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

[Kieran Bingham]

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.

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.

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

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.



> 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.


> 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 ...
and use something like


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

	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'?

Anyway, overall - I like the idea of a cleanup helper...

--
Kieran


> 
> 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
>