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

[Laurent Pinchart]

Hi Kieran,

On Tue, Oct 11, 2022 at 03:51:19PM +0100, Kieran Bingham wrote:
> Quoting Laurent Pinchart via libcamera-devel (2022-10-04 03:18:40)
> > 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.
> 
> I'm not sure I like 'Cleaner' either. To me, this looks very much like
> the scope_exit experimental C++ features:
> 
>  https://en.cppreference.com/w/cpp/experimental/scope_exit
> 
> I like the ability to add multiple cleanups to a single instance though.

Me too. I considered going for something more similar to scope_exit, but
it would have required calling the .release() function on each scope
guard individually at the end, which isn't very nice.

> https://github.com/SergiusTheBest/ScopeExit is another interesting
> implementation, but bases the choice for a Success or Failed return
> using exceptions, so that's not of any use to us.

Same as https://en.cppreference.com/w/cpp/experimental/scope_fail.

> I (almost) like the syntatic sugar that means lambda's are hidden away:
> 
>   https://github.com/SergiusTheBest/ScopeExit/blob/master/include/ScopeExit/ScopeExit.h
> 
>   SCOPE_EXIT{ cout << "hello"; }; // will be called at the scope exit

Almost indeed.

> Which could easily be ScopeExit { }; ... but that's creating a separate
> instance for each declaration which wouldn't be easy to 'clean up' or
> cancel..., so I think that's not any better.

Exactly :-)

> > 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
> 
> Explicitly requiring either Cleaner.success() or Cleaner.fail() with an
> error if one isn't called is interesting, but I am weary that then
> someone will end up simply calling the 'wrong one' on a code path ...
> But maybe that's worrying about 'possible' bugs, rather than actual
> bugs.

That's a possible issue too.

> > 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.
> 
> If there is automatic operations at exit, I'd call it AtScopeExit or
> ScopeExit ...

The name sounds good.

> and use something like
> 
> 
> 	(At?)ScopeExit e;
> 	e.defer([&]{ std::out << "Failed" << std::endl; });

I could also add a constructor that takes a functor, so an instance that
would have a single action wouldn't require a separate call to defer().

> 
> 	if (fail)
> 		return -EINVAL;
> 	
> 	e.cancel();
> 
> 	return 0;
> 
> But it's also just 'deferred work' so perhaps DeferredWorker would be a
> better class name than Cleaner if it is enforced to be called at both
> success/failure paths.
> 
> 
> Remembering to cleanup before a successful exit is a pain, but I bet
> that's easier to catch if it goes wrong than the alternative, as the
> 'success' path is likely to be more often used... So I actually think
> this would be 'safer'?

I think so too.

> Anyway, overall - I like the idea of a cleanup helper...
> 
> > 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