[libcamera-devel,1/2] libcamera: utils: Add Cleaner class

17521 diff mbox series

The Cleaner class is a simple object that performs user-provided actions
upon destruction. As its name implies, it is meant to simplify cleanup
tasks in error handling paths.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 include/libcamera/base/utils.h |  13 ++++
 src/libcamera/base/utils.cpp   | 130 +++++++++++++++++++++++++++++++++
 2 files changed, 143 insertions(+)

Hi Laurent,


On 10/4/22 04:18, Laurent Pinchart wrote:
> The Cleaner class is a simple object that performs user-provided actions
> upon destruction. As its name implies, it is meant to simplify cleanup
> tasks in error handling paths.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>   include/libcamera/base/utils.h |  13 ++++
>   src/libcamera/base/utils.cpp   | 130 +++++++++++++++++++++++++++++++++
>   2 files changed, 143 insertions(+)
> 
> diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h
> index ed88b7163770..0de80eb89d15 100644
> --- a/include/libcamera/base/utils.h
> +++ b/include/libcamera/base/utils.h
> @@ -9,6 +9,7 @@
>   
>   #include <algorithm>
>   #include <chrono>
> +#include <functional>
>   #include <iterator>
>   #include <memory>
>   #include <ostream>
> @@ -381,6 +382,18 @@ struct defopt_t {
>   
>   constexpr details::defopt_t defopt;
>   
> +class Cleaner
> +{
> +public:
> +	~Cleaner();
> +
> +	void defer(std::function<void()> &&action);
> +	void clear();
> +
> +private:
> +	std::vector<std::function<void()>> actions_;
> +};
> +
>   } /* namespace utils */
>   
>   #ifndef __DOXYGEN__
> diff --git a/src/libcamera/base/utils.cpp b/src/libcamera/base/utils.cpp
> index 9cd6cb197243..45e115b7031e 100644
> --- a/src/libcamera/base/utils.cpp
> +++ b/src/libcamera/base/utils.cpp
> @@ -484,6 +484,136 @@ std::string toAscii(const std::string &str)
>    * default-constructed T.
>    */
>   
> +/**
> + * \class Cleaner
> + * \brief An object that performs actions upon destruction
> + *
> + * The Cleaner class is a simple object that performs user-provided actions
> + * upon destruction. As its name implies, it is meant to simplify cleanup tasks
> + * in error handling paths.
> + *
> + * When the code flow performs multiple sequential actions that each need a
> + * corresponding cleanup action, error handling quickly become tedious:
> + *
> + * \code{.cpp}
> + * {
> + * 	int ret = allocateMemory();
> + * 	if (ret)
> + * 		return ret;
> + *
> + * 	ret = startProducder();
typo
> + * 	if (ret) {
> + * 		freeMemory();
> + * 		return ret;
> + * 	}
> + *
> + * 	ret = startConsumer();
> + * 	if (ret) {
> + * 		stopProducer();
> + * 		freeMemory();
> + * 		return ret;
> + * 	}
> + *
> + * 	return 0;
> + * }
> + * \endcode
> + *
> + * This is prone to programming mistakes, as cleanup actions can easily be
> + * forgotten or ordered incorrectly. One strategy to simplify error handling is
> + * to use goto statements:
> + *
> + * \code{.cpp}
> + * {
> + * 	int ret = allocateMemory();
> + * 	if (ret)
> + * 		return ret;
> + *
> + * 	ret = startProducder();
typo
> + * 	if (ret)
> + * 		goto error_free;
> + *
> + * 	ret = startConsumer();
> + * 	if (ret)
> + * 		goto error_stop;
> + *
> + * 	return 0;
> + *
> + * error_stop:
> + * 	stopProducer();
> + * error_free:
> + * 	freeMemory();
> + * 	return ret;
> + * }
> + * \endcode
> + *
> + * While this may be considered better, this solution is still quite
> + * error-prone. Beside the risk of picking the wrong error label, the error
> + * handling logic is separated from the normal code flow, which increases the
> + * risk of error when refactoring the code. Additionally, C++ doesn't allow
> + * goto statements to jump over local variable declarations, which can make
> + * usage of this pattern more difficult.
> + *
> + * The Cleaner class solves these issues by allowing code that requires cleanup
> + * actions to be grouped with its corresponding deferred error handling code:
> + *
> + * \code{.cpp}
> + * {
> + * 	Cleaner cleaner;
> + *
> + * 	int ret = allocateMemory();
> + * 	if (ret)
> + * 		return ret;
> + *
> + * 	cleaner.defer([&]() { freeMemory(); });
> + *
> + * 	ret = startProducder();
typo
> + * 	if (ret)
> + * 		return ret;
> + *
> + * 	cleaner.defer([&]() { stopProducer(); });
> + *
> + * 	ret = startConsumer();
> + * 	if (ret)
> + * 		return ret;
> + *
> + * 	cleaner.clear();
> + * 	return 0;
> + * }
> + * \endcode
> + *
> + * Deferred error handlers are executed when the Cleaner instance is destroyed,
> + * in the reverse order of their addition.
> + */
> +
> +Cleaner::~Cleaner()
> +{
> +	for (const auto &action : utils::reverse(actions_))
> +		action();
> +}
> +
> +/**
> + * \brief Add a deferred cleanup action
> + * \param[in] action The cleanup action
> + *
> + * Add a deferred cleanup action to the Cleaner. Cleanup actions will be called
> + * upon destruction in the reverse order of their addition.
> + */
> +void Cleaner::defer(std::function<void()> &&action)
> +{
> +	actions_.push_back(std::move(action));
> +}
> +
> +/**
> + * \brief Clear all deferred cleanup actions
> + *
> + * This function should be called in the success path to clear all deferred
> + * error cleanup actions.
> + */
> +void Cleaner::clear()
> +{
> +	actions_.clear();
> +}
> +
>   } /* namespace utils */
>   
>   #ifndef __DOXYGEN__With those typos fixed:

Reviewed-by: Xavier Roumegue <xavier.roumegue@oss.nxp.com>

Hi Laurent

On Tue, Oct 04, 2022 at 05:18:41AM +0300, Laurent Pinchart via libcamera-devel wrote:
> The Cleaner class is a simple object that performs user-provided actions
> upon destruction. As its name implies, it is meant to simplify cleanup
> tasks in error handling paths.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  include/libcamera/base/utils.h |  13 ++++
>  src/libcamera/base/utils.cpp   | 130 +++++++++++++++++++++++++++++++++
>  2 files changed, 143 insertions(+)
>
> diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h
> index ed88b7163770..0de80eb89d15 100644
> --- a/include/libcamera/base/utils.h
> +++ b/include/libcamera/base/utils.h
> @@ -9,6 +9,7 @@
>
>  #include <algorithm>
>  #include <chrono>
> +#include <functional>
>  #include <iterator>
>  #include <memory>
>  #include <ostream>
> @@ -381,6 +382,18 @@ struct defopt_t {
>
>  constexpr details::defopt_t defopt;
>
> +class Cleaner
> +{
> +public:
> +	~Cleaner();
> +
> +	void defer(std::function<void()> &&action);
> +	void clear();
> +
> +private:
> +	std::vector<std::function<void()>> actions_;
> +};
> +
>  } /* namespace utils */
>
>  #ifndef __DOXYGEN__
> diff --git a/src/libcamera/base/utils.cpp b/src/libcamera/base/utils.cpp
> index 9cd6cb197243..45e115b7031e 100644
> --- a/src/libcamera/base/utils.cpp
> +++ b/src/libcamera/base/utils.cpp
> @@ -484,6 +484,136 @@ std::string toAscii(const std::string &str)
>   * default-constructed T.
>   */
>
> +/**
> + * \class Cleaner
> + * \brief An object that performs actions upon destruction
> + *
> + * The Cleaner class is a simple object that performs user-provided actions
> + * upon destruction. As its name implies, it is meant to simplify cleanup tasks
> + * in error handling paths.
> + *
> + * When the code flow performs multiple sequential actions that each need a
> + * corresponding cleanup action, error handling quickly become tedious:
> + *
> + * \code{.cpp}
> + * {
> + * 	int ret = allocateMemory();
> + * 	if (ret)
> + * 		return ret;
> + *
> + * 	ret = startProducder();
> + * 	if (ret) {
> + * 		freeMemory();
> + * 		return ret;
> + * 	}
> + *
> + * 	ret = startConsumer();
> + * 	if (ret) {
> + * 		stopProducer();
> + * 		freeMemory();
> + * 		return ret;
> + * 	}
> + *
> + * 	return 0;
> + * }
> + * \endcode
> + *
> + * This is prone to programming mistakes, as cleanup actions can easily be
> + * forgotten or ordered incorrectly. One strategy to simplify error handling is
> + * to use goto statements:
> + *
> + * \code{.cpp}
> + * {
> + * 	int ret = allocateMemory();
> + * 	if (ret)
> + * 		return ret;
> + *
> + * 	ret = startProducder();
> + * 	if (ret)
> + * 		goto error_free;
> + *
> + * 	ret = startConsumer();
> + * 	if (ret)
> + * 		goto error_stop;
> + *
> + * 	return 0;
> + *
> + * error_stop:
> + * 	stopProducer();
> + * error_free:
> + * 	freeMemory();
> + * 	return ret;
> + * }
> + * \endcode
> + *
> + * While this may be considered better, this solution is still quite
> + * error-prone. Beside the risk of picking the wrong error label, the error
> + * handling logic is separated from the normal code flow, which increases the
> + * risk of error when refactoring the code. Additionally, C++ doesn't allow
> + * goto statements to jump over local variable declarations, which can make
> + * usage of this pattern more difficult.
> + *
> + * The Cleaner class solves these issues by allowing code that requires cleanup
> + * actions to be grouped with its corresponding deferred error handling code:
> + *
> + * \code{.cpp}
> + * {
> + * 	Cleaner cleaner;
> + *
> + * 	int ret = allocateMemory();
> + * 	if (ret)
> + * 		return ret;
> + *
> + * 	cleaner.defer([&]() { freeMemory(); });
> + *
> + * 	ret = startProducder();
> + * 	if (ret)
> + * 		return ret;
> + *
> + * 	cleaner.defer([&]() { stopProducer(); });
> + *
> + * 	ret = startConsumer();
> + * 	if (ret)
> + * 		return ret;
> + *
> + * 	cleaner.clear();
> + * 	return 0;
> + * }
> + * \endcode

I just wonder if

{
	Cleaner cleaner;

	int ret = allocateMemory();
	if (ret)
		return ret;

	cleaner.defer([&]() { freeMemory(); });

	ret = startProducder();
	if (ret) {
                cleaner.clean();
		return ret;
        }

	cleaner.defer([&]() { stopProducer(); });

	ret = startConsumer();
	if (ret) {
                cleaner.clean();
		return ret;
        }

	return 0;
}


IOW make Claner::clean() execute the cleaning actions while
destruction is a no-op.

It just feel more natural to have a clean() call on error paths and
not having to call clean() on a succesfull return.

> + *
> + * Deferred error handlers are executed when the Cleaner instance is destroyed,
> + * in the reverse order of their addition.
> + */
> +
> +Cleaner::~Cleaner()
> +{
> +	for (const auto &action : utils::reverse(actions_))
> +		action();
> +}
> +
> +/**
> + * \brief Add a deferred cleanup action
> + * \param[in] action The cleanup action
> + *
> + * Add a deferred cleanup action to the Cleaner. Cleanup actions will be called
> + * upon destruction in the reverse order of their addition.
> + */
> +void Cleaner::defer(std::function<void()> &&action)
> +{
> +	actions_.push_back(std::move(action));
> +}
> +
> +/**
> + * \brief Clear all deferred cleanup actions
> + *
> + * This function should be called in the success path to clear all deferred
> + * error cleanup actions.
> + */
> +void Cleaner::clear()
> +{
> +	actions_.clear();
> +}
> +
>  } /* namespace utils */
>
>  #ifndef __DOXYGEN__
> --
> Regards,
>
> Laurent Pinchart
>

Hi Jacopo,

On Fri, Oct 07, 2022 at 05:01:57PM +0200, Jacopo Mondi wrote:
> On Tue, Oct 04, 2022 at 05:18:41AM +0300, Laurent Pinchart via libcamera-devel wrote:
> > The Cleaner class is a simple object that performs user-provided actions
> > upon destruction. As its name implies, it is meant to simplify cleanup
> > tasks in error handling paths.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  include/libcamera/base/utils.h |  13 ++++
> >  src/libcamera/base/utils.cpp   | 130 +++++++++++++++++++++++++++++++++
> >  2 files changed, 143 insertions(+)
> >
> > diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h
> > index ed88b7163770..0de80eb89d15 100644
> > --- a/include/libcamera/base/utils.h
> > +++ b/include/libcamera/base/utils.h
> > @@ -9,6 +9,7 @@
> >
> >  #include <algorithm>
> >  #include <chrono>
> > +#include <functional>
> >  #include <iterator>
> >  #include <memory>
> >  #include <ostream>
> > @@ -381,6 +382,18 @@ struct defopt_t {
> >
> >  constexpr details::defopt_t defopt;
> >
> > +class Cleaner
> > +{
> > +public:
> > +	~Cleaner();
> > +
> > +	void defer(std::function<void()> &&action);
> > +	void clear();
> > +
> > +private:
> > +	std::vector<std::function<void()>> actions_;
> > +};
> > +
> >  } /* namespace utils */
> >
> >  #ifndef __DOXYGEN__
> > diff --git a/src/libcamera/base/utils.cpp b/src/libcamera/base/utils.cpp
> > index 9cd6cb197243..45e115b7031e 100644
> > --- a/src/libcamera/base/utils.cpp
> > +++ b/src/libcamera/base/utils.cpp
> > @@ -484,6 +484,136 @@ std::string toAscii(const std::string &str)
> >   * default-constructed T.
> >   */
> >
> > +/**
> > + * \class Cleaner
> > + * \brief An object that performs actions upon destruction
> > + *
> > + * The Cleaner class is a simple object that performs user-provided actions
> > + * upon destruction. As its name implies, it is meant to simplify cleanup tasks
> > + * in error handling paths.
> > + *
> > + * When the code flow performs multiple sequential actions that each need a
> > + * corresponding cleanup action, error handling quickly become tedious:
> > + *
> > + * \code{.cpp}
> > + * {
> > + * 	int ret = allocateMemory();
> > + * 	if (ret)
> > + * 		return ret;
> > + *
> > + * 	ret = startProducder();
> > + * 	if (ret) {
> > + * 		freeMemory();
> > + * 		return ret;
> > + * 	}
> > + *
> > + * 	ret = startConsumer();
> > + * 	if (ret) {
> > + * 		stopProducer();
> > + * 		freeMemory();
> > + * 		return ret;
> > + * 	}
> > + *
> > + * 	return 0;
> > + * }
> > + * \endcode
> > + *
> > + * This is prone to programming mistakes, as cleanup actions can easily be
> > + * forgotten or ordered incorrectly. One strategy to simplify error handling is
> > + * to use goto statements:
> > + *
> > + * \code{.cpp}
> > + * {
> > + * 	int ret = allocateMemory();
> > + * 	if (ret)
> > + * 		return ret;
> > + *
> > + * 	ret = startProducder();
> > + * 	if (ret)
> > + * 		goto error_free;
> > + *
> > + * 	ret = startConsumer();
> > + * 	if (ret)
> > + * 		goto error_stop;
> > + *
> > + * 	return 0;
> > + *
> > + * error_stop:
> > + * 	stopProducer();
> > + * error_free:
> > + * 	freeMemory();
> > + * 	return ret;
> > + * }
> > + * \endcode
> > + *
> > + * While this may be considered better, this solution is still quite
> > + * error-prone. Beside the risk of picking the wrong error label, the error
> > + * handling logic is separated from the normal code flow, which increases the
> > + * risk of error when refactoring the code. Additionally, C++ doesn't allow
> > + * goto statements to jump over local variable declarations, which can make
> > + * usage of this pattern more difficult.
> > + *
> > + * The Cleaner class solves these issues by allowing code that requires cleanup
> > + * actions to be grouped with its corresponding deferred error handling code:
> > + *
> > + * \code{.cpp}
> > + * {
> > + * 	Cleaner cleaner;
> > + *
> > + * 	int ret = allocateMemory();
> > + * 	if (ret)
> > + * 		return ret;
> > + *
> > + * 	cleaner.defer([&]() { freeMemory(); });
> > + *
> > + * 	ret = startProducder();
> > + * 	if (ret)
> > + * 		return ret;
> > + *
> > + * 	cleaner.defer([&]() { stopProducer(); });
> > + *
> > + * 	ret = startConsumer();
> > + * 	if (ret)
> > + * 		return ret;
> > + *
> > + * 	cleaner.clear();
> > + * 	return 0;
> > + * }
> > + * \endcode
> 
> I just wonder if
> 
> {
> 	Cleaner cleaner;
> 
> 	int ret = allocateMemory();
> 	if (ret)
> 		return ret;
> 
> 	cleaner.defer([&]() { freeMemory(); });
> 
> 	ret = startProducder();
> 	if (ret) {
>                 cleaner.clean();
> 		return ret;
>         }
> 
> 	cleaner.defer([&]() { stopProducer(); });
> 
> 	ret = startConsumer();
> 	if (ret) {
>                 cleaner.clean();
> 		return ret;
>         }
> 
> 	return 0;
> }
> 
> 
> IOW make Claner::clean() execute the cleaning actions while
> destruction is a no-op.

I've thought about it, and I like the idea of having no additional
function call in the success path. However, that brings other issues:

- There are usually more error paths than success paths, so there will
  be more clean() calls needed, with more risks of forgetting one of
  them.

- Error paths are less tested. A missing clear() in the success path
  will likely be detected very quickly (as all the deferred cleaner
  actions will be executed), while a missing clean() in one error path
  has a much higher chance to end up not being detected for a long time.

Another option would be to have dedicated calls for both the error and
success paths, and warn loudly if the object is destructed without any
of these functions having been called. It will ensure we get a warning
the first time an incorrect error path is taken, but we'll still have
risks of missing calls in error paths being detected.

Yet another option is to hook up the Cleaner class in the return
statement itself, writing

	return cleaner.return(ret);

ret == 0 would be considered a success and ret != 0 an error. Again,
nothing will guarantee that a stray

	return ret;

gets added to an error path, but it may be easier to visually catch the
offending code during review (not entirely sure about that).

Yet another option would be to change the function to return a Result
instead of an int, with the Cleaner::return() (or Cleaner::result(),
name bikeshading is allowed) function returning a Result. Here again,
someone may create a Result directly, so it may not help much.

If we were using exceptions the Cleaner destructor could differentiate
between the success and failure cases more easily. I can't see another
elegant method to achieve the same with error values, but maybe I'm
missing something.

> It just feel more natural to have a clean() call on error paths and
> not having to call clean() on a succesfull return.

Note that it's "clear", not "clean", in the success path. I think I'll
rename it to release() to avoid confusion, and match
https://en.cppreference.com/w/cpp/experimental/scope_fail.

> > + *
> > + * Deferred error handlers are executed when the Cleaner instance is destroyed,
> > + * in the reverse order of their addition.
> > + */
> > +
> > +Cleaner::~Cleaner()
> > +{
> > +	for (const auto &action : utils::reverse(actions_))
> > +		action();
> > +}
> > +
> > +/**
> > + * \brief Add a deferred cleanup action
> > + * \param[in] action The cleanup action
> > + *
> > + * Add a deferred cleanup action to the Cleaner. Cleanup actions will be called
> > + * upon destruction in the reverse order of their addition.
> > + */
> > +void Cleaner::defer(std::function<void()> &&action)
> > +{
> > +	actions_.push_back(std::move(action));
> > +}
> > +
> > +/**
> > + * \brief Clear all deferred cleanup actions
> > + *
> > + * This function should be called in the success path to clear all deferred
> > + * error cleanup actions.
> > + */
> > +void Cleaner::clear()
> > +{
> > +	actions_.clear();
> > +}
> > +
> >  } /* namespace utils */
> >
> >  #ifndef __DOXYGEN__

Quoting Jacopo Mondi via libcamera-devel (2022-10-07 16:01:57)
> Hi Laurent
> 
> On Tue, Oct 04, 2022 at 05:18:41AM +0300, Laurent Pinchart via libcamera-devel wrote:
> > The Cleaner class is a simple object that performs user-provided actions
> > upon destruction. As its name implies, it is meant to simplify cleanup
> > tasks in error handling paths.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  include/libcamera/base/utils.h |  13 ++++
> >  src/libcamera/base/utils.cpp   | 130 +++++++++++++++++++++++++++++++++
> >  2 files changed, 143 insertions(+)
> >
> > diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h
> > index ed88b7163770..0de80eb89d15 100644
> > --- a/include/libcamera/base/utils.h
> > +++ b/include/libcamera/base/utils.h
> > @@ -9,6 +9,7 @@
> >
> >  #include <algorithm>
> >  #include <chrono>
> > +#include <functional>
> >  #include <iterator>
> >  #include <memory>
> >  #include <ostream>
> > @@ -381,6 +382,18 @@ struct defopt_t {
> >
> >  constexpr details::defopt_t defopt;
> >
> > +class Cleaner
> > +{
> > +public:
> > +     ~Cleaner();
> > +
> > +     void defer(std::function<void()> &&action);
> > +     void clear();
> > +
> > +private:
> > +     std::vector<std::function<void()>> actions_;
> > +};
> > +
> >  } /* namespace utils */
> >
> >  #ifndef __DOXYGEN__
> > diff --git a/src/libcamera/base/utils.cpp b/src/libcamera/base/utils.cpp
> > index 9cd6cb197243..45e115b7031e 100644
> > --- a/src/libcamera/base/utils.cpp
> > +++ b/src/libcamera/base/utils.cpp
> > @@ -484,6 +484,136 @@ std::string toAscii(const std::string &str)
> >   * default-constructed T.
> >   */
> >
> > +/**
> > + * \class Cleaner
> > + * \brief An object that performs actions upon destruction
> > + *
> > + * The Cleaner class is a simple object that performs user-provided actions
> > + * upon destruction. As its name implies, it is meant to simplify cleanup tasks
> > + * in error handling paths.
> > + *
> > + * When the code flow performs multiple sequential actions that each need a
> > + * corresponding cleanup action, error handling quickly become tedious:
> > + *
> > + * \code{.cpp}
> > + * {
> > + *   int ret = allocateMemory();
> > + *   if (ret)
> > + *           return ret;
> > + *
> > + *   ret = startProducder();
> > + *   if (ret) {
> > + *           freeMemory();
> > + *           return ret;
> > + *   }
> > + *
> > + *   ret = startConsumer();
> > + *   if (ret) {
> > + *           stopProducer();
> > + *           freeMemory();
> > + *           return ret;
> > + *   }
> > + *
> > + *   return 0;
> > + * }
> > + * \endcode
> > + *
> > + * This is prone to programming mistakes, as cleanup actions can easily be
> > + * forgotten or ordered incorrectly. One strategy to simplify error handling is
> > + * to use goto statements:
> > + *
> > + * \code{.cpp}
> > + * {
> > + *   int ret = allocateMemory();
> > + *   if (ret)
> > + *           return ret;
> > + *
> > + *   ret = startProducder();
> > + *   if (ret)
> > + *           goto error_free;
> > + *
> > + *   ret = startConsumer();
> > + *   if (ret)
> > + *           goto error_stop;
> > + *
> > + *   return 0;
> > + *
> > + * error_stop:
> > + *   stopProducer();
> > + * error_free:
> > + *   freeMemory();
> > + *   return ret;
> > + * }
> > + * \endcode
> > + *
> > + * While this may be considered better, this solution is still quite
> > + * error-prone. Beside the risk of picking the wrong error label, the error
> > + * handling logic is separated from the normal code flow, which increases the
> > + * risk of error when refactoring the code. Additionally, C++ doesn't allow
> > + * goto statements to jump over local variable declarations, which can make
> > + * usage of this pattern more difficult.
> > + *
> > + * The Cleaner class solves these issues by allowing code that requires cleanup
> > + * actions to be grouped with its corresponding deferred error handling code:
> > + *
> > + * \code{.cpp}
> > + * {
> > + *   Cleaner cleaner;
> > + *
> > + *   int ret = allocateMemory();
> > + *   if (ret)
> > + *           return ret;
> > + *
> > + *   cleaner.defer([&]() { freeMemory(); });
> > + *
> > + *   ret = startProducder();
> > + *   if (ret)
> > + *           return ret;
> > + *
> > + *   cleaner.defer([&]() { stopProducer(); });
> > + *
> > + *   ret = startConsumer();
> > + *   if (ret)
> > + *           return ret;
> > + *
> > + *   cleaner.clear();
> > + *   return 0;
> > + * }
> > + * \endcode
> 
> I just wonder if
> 
> {
>         Cleaner cleaner;
> 
>         int ret = allocateMemory();
>         if (ret)
>                 return ret;
> 
>         cleaner.defer([&]() { freeMemory(); });
> 
>         ret = startProducder();
>         if (ret) {
>                 cleaner.clean();
>                 return ret;
>         }
> 
>         cleaner.defer([&]() { stopProducer(); });
> 
>         ret = startConsumer();
>         if (ret) {
>                 cleaner.clean();
>                 return ret;
>         }
> 
>         return 0;
> }
> 
> 
> IOW make Claner::clean() execute the cleaning actions while
> destruction is a no-op.
> 
> It just feel more natural to have a clean() call on error paths and
> not having to call clean() on a succesfull return.

If we use the name 'Cleaner' then, I'd actually agree here, as it would
be clear we're recording and collecting actions to 'clean', and
explicitly specifying 'when' to run them.

But as I said in the cover letter, I prefer the 'ScopeExit' terms, and I
think it's more likely that we'd catch bugs if we forget to 'cancel' a
cleanup/defer operation, than catch them if we forget to act on the
cleanups on a less used error path...


> > + *
> > + * Deferred error handlers are executed when the Cleaner instance is destroyed,
> > + * in the reverse order of their addition.
> > + */
> > +
> > +Cleaner::~Cleaner()
> > +{
> > +     for (const auto &action : utils::reverse(actions_))
> > +             action();
> > +}
> > +
> > +/**
> > + * \brief Add a deferred cleanup action
> > + * \param[in] action The cleanup action
> > + *
> > + * Add a deferred cleanup action to the Cleaner. Cleanup actions will be called
> > + * upon destruction in the reverse order of their addition.
> > + */
> > +void Cleaner::defer(std::function<void()> &&action)
> > +{
> > +     actions_.push_back(std::move(action));
> > +}
> > +
> > +/**
> > + * \brief Clear all deferred cleanup actions
> > + *
> > + * This function should be called in the success path to clear all deferred
> > + * error cleanup actions.
> > + */
> > +void Cleaner::clear()
> > +{
> > +     actions_.clear();
> > +}
> > +
> >  } /* namespace utils */
> >
> >  #ifndef __DOXYGEN__
> > --
> > Regards,
> >
> > Laurent Pinchart
> >

Quoting Laurent Pinchart via libcamera-devel (2022-10-07 16:26:00)
> Hi Jacopo,
> 
> On Fri, Oct 07, 2022 at 05:01:57PM +0200, Jacopo Mondi wrote:
> > On Tue, Oct 04, 2022 at 05:18:41AM +0300, Laurent Pinchart via libcamera-devel wrote:
> > > The Cleaner class is a simple object that performs user-provided actions
> > > upon destruction. As its name implies, it is meant to simplify cleanup
> > > tasks in error handling paths.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > >  include/libcamera/base/utils.h |  13 ++++
> > >  src/libcamera/base/utils.cpp   | 130 +++++++++++++++++++++++++++++++++
> > >  2 files changed, 143 insertions(+)
> > >
> > > diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h
> > > index ed88b7163770..0de80eb89d15 100644
> > > --- a/include/libcamera/base/utils.h
> > > +++ b/include/libcamera/base/utils.h
> > > @@ -9,6 +9,7 @@
> > >
> > >  #include <algorithm>
> > >  #include <chrono>
> > > +#include <functional>
> > >  #include <iterator>
> > >  #include <memory>
> > >  #include <ostream>
> > > @@ -381,6 +382,18 @@ struct defopt_t {
> > >
> > >  constexpr details::defopt_t defopt;
> > >
> > > +class Cleaner
> > > +{
> > > +public:
> > > +   ~Cleaner();
> > > +
> > > +   void defer(std::function<void()> &&action);
> > > +   void clear();
> > > +
> > > +private:
> > > +   std::vector<std::function<void()>> actions_;
> > > +};
> > > +
> > >  } /* namespace utils */
> > >
> > >  #ifndef __DOXYGEN__
> > > diff --git a/src/libcamera/base/utils.cpp b/src/libcamera/base/utils.cpp
> > > index 9cd6cb197243..45e115b7031e 100644
> > > --- a/src/libcamera/base/utils.cpp
> > > +++ b/src/libcamera/base/utils.cpp
> > > @@ -484,6 +484,136 @@ std::string toAscii(const std::string &str)
> > >   * default-constructed T.
> > >   */
> > >
> > > +/**
> > > + * \class Cleaner
> > > + * \brief An object that performs actions upon destruction
> > > + *
> > > + * The Cleaner class is a simple object that performs user-provided actions
> > > + * upon destruction. As its name implies, it is meant to simplify cleanup tasks
> > > + * in error handling paths.
> > > + *
> > > + * When the code flow performs multiple sequential actions that each need a
> > > + * corresponding cleanup action, error handling quickly become tedious:
> > > + *
> > > + * \code{.cpp}
> > > + * {
> > > + *         int ret = allocateMemory();
> > > + *         if (ret)
> > > + *                 return ret;
> > > + *
> > > + *         ret = startProducder();
> > > + *         if (ret) {
> > > + *                 freeMemory();
> > > + *                 return ret;
> > > + *         }
> > > + *
> > > + *         ret = startConsumer();
> > > + *         if (ret) {
> > > + *                 stopProducer();
> > > + *                 freeMemory();
> > > + *                 return ret;
> > > + *         }
> > > + *
> > > + *         return 0;
> > > + * }
> > > + * \endcode
> > > + *
> > > + * This is prone to programming mistakes, as cleanup actions can easily be
> > > + * forgotten or ordered incorrectly. One strategy to simplify error handling is
> > > + * to use goto statements:
> > > + *
> > > + * \code{.cpp}
> > > + * {
> > > + *         int ret = allocateMemory();
> > > + *         if (ret)
> > > + *                 return ret;
> > > + *
> > > + *         ret = startProducder();
> > > + *         if (ret)
> > > + *                 goto error_free;
> > > + *
> > > + *         ret = startConsumer();
> > > + *         if (ret)
> > > + *                 goto error_stop;
> > > + *
> > > + *         return 0;
> > > + *
> > > + * error_stop:
> > > + *         stopProducer();
> > > + * error_free:
> > > + *         freeMemory();
> > > + *         return ret;
> > > + * }
> > > + * \endcode
> > > + *
> > > + * While this may be considered better, this solution is still quite
> > > + * error-prone. Beside the risk of picking the wrong error label, the error
> > > + * handling logic is separated from the normal code flow, which increases the
> > > + * risk of error when refactoring the code. Additionally, C++ doesn't allow
> > > + * goto statements to jump over local variable declarations, which can make
> > > + * usage of this pattern more difficult.
> > > + *
> > > + * The Cleaner class solves these issues by allowing code that requires cleanup
> > > + * actions to be grouped with its corresponding deferred error handling code:
> > > + *
> > > + * \code{.cpp}
> > > + * {
> > > + *         Cleaner cleaner;
> > > + *
> > > + *         int ret = allocateMemory();
> > > + *         if (ret)
> > > + *                 return ret;
> > > + *
> > > + *         cleaner.defer([&]() { freeMemory(); });
> > > + *
> > > + *         ret = startProducder();
> > > + *         if (ret)
> > > + *                 return ret;
> > > + *
> > > + *         cleaner.defer([&]() { stopProducer(); });
> > > + *
> > > + *         ret = startConsumer();
> > > + *         if (ret)
> > > + *                 return ret;
> > > + *
> > > + *         cleaner.clear();
> > > + *         return 0;
> > > + * }
> > > + * \endcode
> > 
> > I just wonder if
> > 
> > {
> >       Cleaner cleaner;
> > 
> >       int ret = allocateMemory();
> >       if (ret)
> >               return ret;
> > 
> >       cleaner.defer([&]() { freeMemory(); });
> > 
> >       ret = startProducder();
> >       if (ret) {
> >                 cleaner.clean();
> >               return ret;
> >         }
> > 
> >       cleaner.defer([&]() { stopProducer(); });
> > 
> >       ret = startConsumer();
> >       if (ret) {
> >                 cleaner.clean();
> >               return ret;
> >         }
> > 
> >       return 0;
> > }
> > 
> > 
> > IOW make Claner::clean() execute the cleaning actions while
> > destruction is a no-op.
> 
> I've thought about it, and I like the idea of having no additional
> function call in the success path. However, that brings other issues:
> 
> - There are usually more error paths than success paths, so there will
>   be more clean() calls needed, with more risks of forgetting one of
>   them.
> 
> - Error paths are less tested. A missing clear() in the success path
>   will likely be detected very quickly (as all the deferred cleaner
>   actions will be executed), while a missing clean() in one error path
>   has a much higher chance to end up not being detected for a long time.

Aha, looks like I should have started here. I think both of the above
are why I prefer automatic cleanup on error paths, and an explicit
cancellation of deferred work on success. (Assuming a single success
path).


> Another option would be to have dedicated calls for both the error and
> success paths, and warn loudly if the object is destructed without any
> of these functions having been called. It will ensure we get a warning
> the first time an incorrect error path is taken, but we'll still have
> risks of missing calls in error paths being detected.
> 
> Yet another option is to hook up the Cleaner class in the return
> statement itself, writing
> 
>         return cleaner.return(ret);
> 
> ret == 0 would be considered a success and ret != 0 an error. Again,
> nothing will guarantee that a stray
> 
>         return ret;

But that would only work on integer/boolean return function types?
Unless ... there was a ScopedReturn return type that /enforced/ use of
an explicit return type whenever a ScopedExit / Cleaner class was in use
in a function?

(I'm not sure how that would be handled though, and it would be just
another wrapper around return types to cause confusion perhaps)


> 
> gets added to an error path, but it may be easier to visually catch the
> offending code during review (not entirely sure about that).
> 
> Yet another option would be to change the function to return a Result
> instead of an int, with the Cleaner::return() (or Cleaner::result(),
> name bikeshading is allowed) function returning a Result. Here again,
> someone may create a Result directly, so it may not help much.

Damnit ... You have the same ideas before I get there...


> If we were using exceptions the Cleaner destructor could differentiate
> between the success and failure cases more easily. I can't see another
> elegant method to achieve the same with error values, but maybe I'm
> missing something.
> 
> > It just feel more natural to have a clean() call on error paths and
> > not having to call clean() on a succesfull return.
> 
> Note that it's "clear", not "clean", in the success path. I think I'll
> rename it to release() to avoid confusion, and match
> https://en.cppreference.com/w/cpp/experimental/scope_fail.

Hrm... release() makes sense too, though for some reason I still think
of it as a cancellation ... .cancel() ... but I'm not opposed to a
.release().



> 
> > > + *
> > > + * Deferred error handlers are executed when the Cleaner instance is destroyed,
> > > + * in the reverse order of their addition.
> > > + */
> > > +
> > > +Cleaner::~Cleaner()
> > > +{
> > > +   for (const auto &action : utils::reverse(actions_))
> > > +           action();
> > > +}
> > > +
> > > +/**
> > > + * \brief Add a deferred cleanup action
> > > + * \param[in] action The cleanup action
> > > + *
> > > + * Add a deferred cleanup action to the Cleaner. Cleanup actions will be called
> > > + * upon destruction in the reverse order of their addition.
> > > + */
> > > +void Cleaner::defer(std::function<void()> &&action)
> > > +{
> > > +   actions_.push_back(std::move(action));
> > > +}
> > > +
> > > +/**
> > > + * \brief Clear all deferred cleanup actions
> > > + *
> > > + * This function should be called in the success path to clear all deferred
> > > + * error cleanup actions.
> > > + */
> > > +void Cleaner::clear()
> > > +{
> > > +   actions_.clear();
> > > +}
> > > +
> > >  } /* namespace utils */
> > >
> > >  #ifndef __DOXYGEN__
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Hi Kieran,

On Tue, Oct 11, 2022 at 04:03:20PM +0100, Kieran Bingham wrote:
> Quoting Laurent Pinchart via libcamera-devel (2022-10-07 16:26:00)
> > On Fri, Oct 07, 2022 at 05:01:57PM +0200, Jacopo Mondi wrote:
> > > On Tue, Oct 04, 2022 at 05:18:41AM +0300, Laurent Pinchart via libcamera-devel wrote:
> > > > The Cleaner class is a simple object that performs user-provided actions
> > > > upon destruction. As its name implies, it is meant to simplify cleanup
> > > > tasks in error handling paths.
> > > >
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > ---
> > > >  include/libcamera/base/utils.h |  13 ++++
> > > >  src/libcamera/base/utils.cpp   | 130 +++++++++++++++++++++++++++++++++
> > > >  2 files changed, 143 insertions(+)
> > > >
> > > > diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h
> > > > index ed88b7163770..0de80eb89d15 100644
> > > > --- a/include/libcamera/base/utils.h
> > > > +++ b/include/libcamera/base/utils.h
> > > > @@ -9,6 +9,7 @@
> > > >
> > > >  #include <algorithm>
> > > >  #include <chrono>
> > > > +#include <functional>
> > > >  #include <iterator>
> > > >  #include <memory>
> > > >  #include <ostream>
> > > > @@ -381,6 +382,18 @@ struct defopt_t {
> > > >
> > > >  constexpr details::defopt_t defopt;
> > > >
> > > > +class Cleaner
> > > > +{
> > > > +public:
> > > > +   ~Cleaner();
> > > > +
> > > > +   void defer(std::function<void()> &&action);
> > > > +   void clear();
> > > > +
> > > > +private:
> > > > +   std::vector<std::function<void()>> actions_;
> > > > +};
> > > > +
> > > >  } /* namespace utils */
> > > >
> > > >  #ifndef __DOXYGEN__
> > > > diff --git a/src/libcamera/base/utils.cpp b/src/libcamera/base/utils.cpp
> > > > index 9cd6cb197243..45e115b7031e 100644
> > > > --- a/src/libcamera/base/utils.cpp
> > > > +++ b/src/libcamera/base/utils.cpp
> > > > @@ -484,6 +484,136 @@ std::string toAscii(const std::string &str)
> > > >   * default-constructed T.
> > > >   */
> > > >
> > > > +/**
> > > > + * \class Cleaner
> > > > + * \brief An object that performs actions upon destruction
> > > > + *
> > > > + * The Cleaner class is a simple object that performs user-provided actions
> > > > + * upon destruction. As its name implies, it is meant to simplify cleanup tasks
> > > > + * in error handling paths.
> > > > + *
> > > > + * When the code flow performs multiple sequential actions that each need a
> > > > + * corresponding cleanup action, error handling quickly become tedious:
> > > > + *
> > > > + * \code{.cpp}
> > > > + * {
> > > > + *         int ret = allocateMemory();
> > > > + *         if (ret)
> > > > + *                 return ret;
> > > > + *
> > > > + *         ret = startProducder();
> > > > + *         if (ret) {
> > > > + *                 freeMemory();
> > > > + *                 return ret;
> > > > + *         }
> > > > + *
> > > > + *         ret = startConsumer();
> > > > + *         if (ret) {
> > > > + *                 stopProducer();
> > > > + *                 freeMemory();
> > > > + *                 return ret;
> > > > + *         }
> > > > + *
> > > > + *         return 0;
> > > > + * }
> > > > + * \endcode
> > > > + *
> > > > + * This is prone to programming mistakes, as cleanup actions can easily be
> > > > + * forgotten or ordered incorrectly. One strategy to simplify error handling is
> > > > + * to use goto statements:
> > > > + *
> > > > + * \code{.cpp}
> > > > + * {
> > > > + *         int ret = allocateMemory();
> > > > + *         if (ret)
> > > > + *                 return ret;
> > > > + *
> > > > + *         ret = startProducder();
> > > > + *         if (ret)
> > > > + *                 goto error_free;
> > > > + *
> > > > + *         ret = startConsumer();
> > > > + *         if (ret)
> > > > + *                 goto error_stop;
> > > > + *
> > > > + *         return 0;
> > > > + *
> > > > + * error_stop:
> > > > + *         stopProducer();
> > > > + * error_free:
> > > > + *         freeMemory();
> > > > + *         return ret;
> > > > + * }
> > > > + * \endcode
> > > > + *
> > > > + * While this may be considered better, this solution is still quite
> > > > + * error-prone. Beside the risk of picking the wrong error label, the error
> > > > + * handling logic is separated from the normal code flow, which increases the
> > > > + * risk of error when refactoring the code. Additionally, C++ doesn't allow
> > > > + * goto statements to jump over local variable declarations, which can make
> > > > + * usage of this pattern more difficult.
> > > > + *
> > > > + * The Cleaner class solves these issues by allowing code that requires cleanup
> > > > + * actions to be grouped with its corresponding deferred error handling code:
> > > > + *
> > > > + * \code{.cpp}
> > > > + * {
> > > > + *         Cleaner cleaner;
> > > > + *
> > > > + *         int ret = allocateMemory();
> > > > + *         if (ret)
> > > > + *                 return ret;
> > > > + *
> > > > + *         cleaner.defer([&]() { freeMemory(); });
> > > > + *
> > > > + *         ret = startProducder();
> > > > + *         if (ret)
> > > > + *                 return ret;
> > > > + *
> > > > + *         cleaner.defer([&]() { stopProducer(); });
> > > > + *
> > > > + *         ret = startConsumer();
> > > > + *         if (ret)
> > > > + *                 return ret;
> > > > + *
> > > > + *         cleaner.clear();
> > > > + *         return 0;
> > > > + * }
> > > > + * \endcode
> > > 
> > > I just wonder if
> > > 
> > > {
> > >       Cleaner cleaner;
> > > 
> > >       int ret = allocateMemory();
> > >       if (ret)
> > >               return ret;
> > > 
> > >       cleaner.defer([&]() { freeMemory(); });
> > > 
> > >       ret = startProducder();
> > >       if (ret) {
> > >                 cleaner.clean();
> > >               return ret;
> > >         }
> > > 
> > >       cleaner.defer([&]() { stopProducer(); });
> > > 
> > >       ret = startConsumer();
> > >       if (ret) {
> > >                 cleaner.clean();
> > >               return ret;
> > >         }
> > > 
> > >       return 0;
> > > }
> > > 
> > > 
> > > IOW make Claner::clean() execute the cleaning actions while
> > > destruction is a no-op.
> > 
> > I've thought about it, and I like the idea of having no additional
> > function call in the success path. However, that brings other issues:
> > 
> > - There are usually more error paths than success paths, so there will
> >   be more clean() calls needed, with more risks of forgetting one of
> >   them.
> > 
> > - Error paths are less tested. A missing clear() in the success path
> >   will likely be detected very quickly (as all the deferred cleaner
> >   actions will be executed), while a missing clean() in one error path
> >   has a much higher chance to end up not being detected for a long time.
> 
> Aha, looks like I should have started here. I think both of the above
> are why I prefer automatic cleanup on error paths, and an explicit
> cancellation of deferred work on success. (Assuming a single success
> path).
> 
> > Another option would be to have dedicated calls for both the error and
> > success paths, and warn loudly if the object is destructed without any
> > of these functions having been called. It will ensure we get a warning
> > the first time an incorrect error path is taken, but we'll still have
> > risks of missing calls in error paths being detected.
> > 
> > Yet another option is to hook up the Cleaner class in the return
> > statement itself, writing
> > 
> >         return cleaner.return(ret);
> > 
> > ret == 0 would be considered a success and ret != 0 an error. Again,
> > nothing will guarantee that a stray
> > 
> >         return ret;
> 
> But that would only work on integer/boolean return function types?
> Unless ... there was a ScopedReturn return type that /enforced/ use of
> an explicit return type whenever a ScopedExit / Cleaner class was in use
> in a function?
> 
> (I'm not sure how that would be handled though, and it would be just
> another wrapper around return types to cause confusion perhaps)
> 
> > gets added to an error path, but it may be easier to visually catch the
> > offending code during review (not entirely sure about that).
> > 
> > Yet another option would be to change the function to return a Result
> > instead of an int, with the Cleaner::return() (or Cleaner::result(),
> > name bikeshading is allowed) function returning a Result. Here again,
> > someone may create a Result directly, so it may not help much.
> 
> Damnit ... You have the same ideas before I get there...

We're getting dangerously close to rust ;-)

> > If we were using exceptions the Cleaner destructor could differentiate
> > between the success and failure cases more easily. I can't see another
> > elegant method to achieve the same with error values, but maybe I'm
> > missing something.
> > 
> > > It just feel more natural to have a clean() call on error paths and
> > > not having to call clean() on a succesfull return.
> > 
> > Note that it's "clear", not "clean", in the success path. I think I'll
> > rename it to release() to avoid confusion, and match
> > https://en.cppreference.com/w/cpp/experimental/scope_fail.
> 
> Hrm... release() makes sense too, though for some reason I still think
> of it as a cancellation ... .cancel() ... but I'm not opposed to a
> .release().

I like .cancel() as well, but .release() being more standard, I think it
has my preference.

> > > > + *
> > > > + * Deferred error handlers are executed when the Cleaner instance is destroyed,
> > > > + * in the reverse order of their addition.
> > > > + */
> > > > +
> > > > +Cleaner::~Cleaner()
> > > > +{
> > > > +   for (const auto &action : utils::reverse(actions_))
> > > > +           action();
> > > > +}
> > > > +
> > > > +/**
> > > > + * \brief Add a deferred cleanup action
> > > > + * \param[in] action The cleanup action
> > > > + *
> > > > + * Add a deferred cleanup action to the Cleaner. Cleanup actions will be called
> > > > + * upon destruction in the reverse order of their addition.
> > > > + */
> > > > +void Cleaner::defer(std::function<void()> &&action)
> > > > +{
> > > > +   actions_.push_back(std::move(action));
> > > > +}
> > > > +
> > > > +/**
> > > > + * \brief Clear all deferred cleanup actions
> > > > + *
> > > > + * This function should be called in the success path to clear all deferred
> > > > + * error cleanup actions.
> > > > + */
> > > > +void Cleaner::clear()
> > > > +{
> > > > +   actions_.clear();
> > > > +}
> > > > +
> > > >  } /* namespace utils */
> > > >
> > > >  #ifndef __DOXYGEN__

On Fri, Oct 07, 2022 at 06:26:00PM +0300, Laurent Pinchart via libcamera-devel wrote:
> Hi Jacopo,
> 
> On Fri, Oct 07, 2022 at 05:01:57PM +0200, Jacopo Mondi wrote:
> > On Tue, Oct 04, 2022 at 05:18:41AM +0300, Laurent Pinchart via libcamera-devel wrote:
> > > The Cleaner class is a simple object that performs user-provided actions
> > > upon destruction. As its name implies, it is meant to simplify cleanup
> > > tasks in error handling paths.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > >  include/libcamera/base/utils.h |  13 ++++
> > >  src/libcamera/base/utils.cpp   | 130 +++++++++++++++++++++++++++++++++
> > >  2 files changed, 143 insertions(+)
> > >
> > > diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h
> > > index ed88b7163770..0de80eb89d15 100644
> > > --- a/include/libcamera/base/utils.h
> > > +++ b/include/libcamera/base/utils.h
> > > @@ -9,6 +9,7 @@
> > >
> > >  #include <algorithm>
> > >  #include <chrono>
> > > +#include <functional>
> > >  #include <iterator>
> > >  #include <memory>
> > >  #include <ostream>
> > > @@ -381,6 +382,18 @@ struct defopt_t {
> > >
> > >  constexpr details::defopt_t defopt;
> > >
> > > +class Cleaner
> > > +{
> > > +public:
> > > +	~Cleaner();
> > > +
> > > +	void defer(std::function<void()> &&action);
> > > +	void clear();
> > > +
> > > +private:
> > > +	std::vector<std::function<void()>> actions_;
> > > +};
> > > +
> > >  } /* namespace utils */
> > >
> > >  #ifndef __DOXYGEN__
> > > diff --git a/src/libcamera/base/utils.cpp b/src/libcamera/base/utils.cpp
> > > index 9cd6cb197243..45e115b7031e 100644
> > > --- a/src/libcamera/base/utils.cpp
> > > +++ b/src/libcamera/base/utils.cpp
> > > @@ -484,6 +484,136 @@ std::string toAscii(const std::string &str)
> > >   * default-constructed T.
> > >   */
> > >
> > > +/**
> > > + * \class Cleaner
> > > + * \brief An object that performs actions upon destruction
> > > + *
> > > + * The Cleaner class is a simple object that performs user-provided actions
> > > + * upon destruction. As its name implies, it is meant to simplify cleanup tasks
> > > + * in error handling paths.
> > > + *
> > > + * When the code flow performs multiple sequential actions that each need a
> > > + * corresponding cleanup action, error handling quickly become tedious:
> > > + *
> > > + * \code{.cpp}
> > > + * {
> > > + * 	int ret = allocateMemory();
> > > + * 	if (ret)
> > > + * 		return ret;
> > > + *
> > > + * 	ret = startProducder();
> > > + * 	if (ret) {
> > > + * 		freeMemory();
> > > + * 		return ret;
> > > + * 	}
> > > + *
> > > + * 	ret = startConsumer();
> > > + * 	if (ret) {
> > > + * 		stopProducer();
> > > + * 		freeMemory();
> > > + * 		return ret;
> > > + * 	}
> > > + *
> > > + * 	return 0;
> > > + * }
> > > + * \endcode
> > > + *
> > > + * This is prone to programming mistakes, as cleanup actions can easily be
> > > + * forgotten or ordered incorrectly. One strategy to simplify error handling is
> > > + * to use goto statements:
> > > + *
> > > + * \code{.cpp}
> > > + * {
> > > + * 	int ret = allocateMemory();
> > > + * 	if (ret)
> > > + * 		return ret;
> > > + *
> > > + * 	ret = startProducder();
> > > + * 	if (ret)
> > > + * 		goto error_free;
> > > + *
> > > + * 	ret = startConsumer();
> > > + * 	if (ret)
> > > + * 		goto error_stop;
> > > + *
> > > + * 	return 0;
> > > + *
> > > + * error_stop:
> > > + * 	stopProducer();
> > > + * error_free:
> > > + * 	freeMemory();
> > > + * 	return ret;
> > > + * }
> > > + * \endcode
> > > + *
> > > + * While this may be considered better, this solution is still quite
> > > + * error-prone. Beside the risk of picking the wrong error label, the error
> > > + * handling logic is separated from the normal code flow, which increases the
> > > + * risk of error when refactoring the code. Additionally, C++ doesn't allow
> > > + * goto statements to jump over local variable declarations, which can make
> > > + * usage of this pattern more difficult.
> > > + *
> > > + * The Cleaner class solves these issues by allowing code that requires cleanup
> > > + * actions to be grouped with its corresponding deferred error handling code:
> > > + *
> > > + * \code{.cpp}
> > > + * {
> > > + * 	Cleaner cleaner;
> > > + *
> > > + * 	int ret = allocateMemory();
> > > + * 	if (ret)
> > > + * 		return ret;
> > > + *
> > > + * 	cleaner.defer([&]() { freeMemory(); });
> > > + *
> > > + * 	ret = startProducder();
> > > + * 	if (ret)
> > > + * 		return ret;
> > > + *
> > > + * 	cleaner.defer([&]() { stopProducer(); });
> > > + *
> > > + * 	ret = startConsumer();
> > > + * 	if (ret)
> > > + * 		return ret;
> > > + *
> > > + * 	cleaner.clear();
> > > + * 	return 0;
> > > + * }
> > > + * \endcode
> > 
> > I just wonder if
> > 
> > {
> > 	Cleaner cleaner;
> > 
> > 	int ret = allocateMemory();
> > 	if (ret)
> > 		return ret;
> > 
> > 	cleaner.defer([&]() { freeMemory(); });
> > 
> > 	ret = startProducder();
> > 	if (ret) {
> >                 cleaner.clean();
> > 		return ret;
> >         }
> > 
> > 	cleaner.defer([&]() { stopProducer(); });
> > 
> > 	ret = startConsumer();
> > 	if (ret) {
> >                 cleaner.clean();
> > 		return ret;
> >         }
> > 
> > 	return 0;
> > }
> > 
> > 
> > IOW make Claner::clean() execute the cleaning actions while
> > destruction is a no-op.
> 
> I've thought about it, and I like the idea of having no additional
> function call in the success path. However, that brings other issues:
> 
> - There are usually more error paths than success paths, so there will
>   be more clean() calls needed, with more risks of forgetting one of
>   them.
> 
> - Error paths are less tested. A missing clear() in the success path
>   will likely be detected very quickly (as all the deferred cleaner
>   actions will be executed), while a missing clean() in one error path
>   has a much higher chance to end up not being detected for a long time.
> 
> Another option would be to have dedicated calls for both the error and
> success paths, and warn loudly if the object is destructed without any
> of these functions having been called. It will ensure we get a warning
> the first time an incorrect error path is taken, but we'll still have
> risks of missing calls in error paths being detected.
> 
> Yet another option is to hook up the Cleaner class in the return
> statement itself, writing
> 
> 	return cleaner.return(ret);

I was thinking this too, given that cleaner.clean() that Jacopo proposed
above adds more fluff. But then again, error paths are less tested, and
are more frequent, so I think that having those auto-clean on scope exit
is better.

Explicit cleanup on success I guess is an acceptable side-product of
this protection.


Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> 
> ret == 0 would be considered a success and ret != 0 an error. Again,
> nothing will guarantee that a stray
> 
> 	return ret;
> 
> gets added to an error path, but it may be easier to visually catch the
> offending code during review (not entirely sure about that).
> 
> Yet another option would be to change the function to return a Result
> instead of an int, with the Cleaner::return() (or Cleaner::result(),
> name bikeshading is allowed) function returning a Result. Here again,
> someone may create a Result directly, so it may not help much.
> 
> If we were using exceptions the Cleaner destructor could differentiate
> between the success and failure cases more easily. I can't see another
> elegant method to achieve the same with error values, but maybe I'm
> missing something.
> 
> > It just feel more natural to have a clean() call on error paths and
> > not having to call clean() on a succesfull return.
> 
> Note that it's "clear", not "clean", in the success path. I think I'll
> rename it to release() to avoid confusion, and match
> https://en.cppreference.com/w/cpp/experimental/scope_fail.
> 
> > > + *
> > > + * Deferred error handlers are executed when the Cleaner instance is destroyed,
> > > + * in the reverse order of their addition.
> > > + */
> > > +
> > > +Cleaner::~Cleaner()
> > > +{
> > > +	for (const auto &action : utils::reverse(actions_))
> > > +		action();
> > > +}
> > > +
> > > +/**
> > > + * \brief Add a deferred cleanup action
> > > + * \param[in] action The cleanup action
> > > + *
> > > + * Add a deferred cleanup action to the Cleaner. Cleanup actions will be called
> > > + * upon destruction in the reverse order of their addition.
> > > + */
> > > +void Cleaner::defer(std::function<void()> &&action)
> > > +{
> > > +	actions_.push_back(std::move(action));
> > > +}
> > > +
> > > +/**
> > > + * \brief Clear all deferred cleanup actions
> > > + *
> > > + * This function should be called in the success path to clear all deferred
> > > + * error cleanup actions.
> > > + */
> > > +void Cleaner::clear()
> > > +{
> > > +	actions_.clear();
> > > +}
> > > +
> > >  } /* namespace utils */
> > >
> > >  #ifndef __DOXYGEN__
> 
> -- 
> Regards,
> 
> Laurent Pinchart