[libcamera-devel] [PATCH v2 0/2] [PATCH 0/2] libcamera: Simplify error handling with ScopeExitActions class

[Laurent Pinchart]

Hello,

This small patch series stems from my review of "[PATCH 13/14]
libcamera: pipeline: rkisp1: Factorize errors handling path on start()".
While I agreed that the current error handling pattern in
PipelineHandlerRkISP1::start() is fairly bad, I wasn't thrilled by usage
of goto statements to fix it. This is an alternative proposal.

The ScopeExitActions class introduced in patch 1/2 took multiple forms
before reaching this one. I first experimented with a class that handled
a single action and thus needed to be instantiated once per action. To
achieve the equivalent of the ScopeExitActions::release() call I had to
either add an 'if (ret)' statement to the lambda functions, or pass the
ret variable to the constructor of the ScopeExitActions. I found ideas
for syntactic sugar in https://github.com/yuri-kilochek/boost.scope_guard,
but in the end, neither option made me very happy.

Handling multiple actions with one ScopeExitActions instance makes the
user code (in 2/2) simpler. The main drawback is the use of std::vector
to store the actions, which requires dynamic allocation (and
reallocation). I tried to find a different solution. One interesting
approach used a ScopeExitGroup class instantiated at the beginning of
the function, and a ScopeExit class that handled a single action,
instantiated multiple times. The ScopeExit class was constructed with a
pointer to the ScopeExitGroup, and only called the action in its
destructor if the ScopeExitGroup hadn't been release()d. Unfortunately
this didn't work well for patch 2/2, as the
PipelineHandlerRkISP1::start() function needs to register an action in a
sub-scope (within a if (...) { ... }). The corresponding ScopeExit
instance thus got destroyed when exiting the sub-scope, not at the end
of the function.

Another completely different approach to error handling would be to make
all the cleanup functions in PipelineHandlerRkISP1 idempotent, safe to
be called when the corresponding operation hasn't been performed (e.g.
streamOff() without a previous streamOn()). This idiom may result to
better error handling through libcamera. If anyone thinks it could be
better than a ScopeExitActions class, that would be enough to convince
me to give it a try and burry ScopeExitActions.

In v1 of the series the class had a defer() function (instead of an
operator+=()), whose name was influenced by an interesting C extension
proposal (https://hal.inria.fr/hal-03059076/document). It uses the name
"guard" for the conceptual equivalent of the ScopeExitActions class, but
that didn't convince me.

I still wish the release() call wouldn't be needed, but I'm not as
unhappy with it than in v1. This is error-prone as it could be
forgotten, but the other options I can think of are worse;

- Switch to the opposite, with cleanups disabled by default and an
  explicit call in the error paths. That could also be forgotten,
  resulting in missing cleanups in some error paths. The result would
  likely be worse, as error paths are less tested.

- Require explicit calls in both the success and error paths, and
  complain loudly in the destructor if none of those functions have been
  called. That's equally error-prone due to undertesting of error paths.

- Tying the explicit calls with the return statements, for instance by
  making the user return a Result type (any rust developer here ?), and
  adding two functions to ScopeExitActions, success and error, that
  return a Result. While the Result idiom may be interesting, it would
  be a much broader change.

Laurent Pinchart (2):
  libcamera: utils: Add ScopeExitActions class
  pipeline: rkisp1: Use ScopeExitActions to simplify error handling in
    start

 include/libcamera/base/utils.h           |  13 +++
 src/libcamera/base/utils.cpp             | 132 +++++++++++++++++++++++
 src/libcamera/pipeline/rkisp1/rkisp1.cpp |  31 ++----
 3 files changed, 156 insertions(+), 20 deletions(-)


base-commit: f4201e9636f4460ad0eacdf32aac43c70c6bf95c
-- 
Regards,

Laurent Pinchart