[libcamera-devel] [PATCH 0/2] libcamera: Simplify error handling with Cleaner 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 Cleaner 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 Cleaner::clear() call I had to either add
an 'if (ret)' statement to the lambda functions, or pass the ret
variable to the constructor of the Cleaner. 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 deferred cleanup actions with one Cleaner instance
makes the user code (in 2/2) simpler. The "defer" name for the
Cleaner::defer() function 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 Cleaner class, but that
didn't convince me. I'm not sure to really like the "Cleaner" name
though, alternative naming proposals are welcome.

One point I'm not happy with is the need to call Cleaner::clear() in the
success path. This is error-prone as it could be forgotten. We could
switch to the opposite, with cleanups disabled by default and an
explicit call in the error paths, but that could also be forgotten,
resulting in missing cleanups in some error paths. The result would
possibly be worse, as error paths are less tested. Another option would
be to require explicit calls in both the success and error paths, and
complain loudly in the destructor if none of those functions have been
called. I'm not sure to be convinced it would be the best idea, although
finding good names for the success/error functions may help. Alternative
ideas are welcome too, as well as opinions regarding whether this is
worth it or not.

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

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

-- 
Regards,

Laurent Pinchart