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

17609 diff mbox series

The ScopeExitActions class is a simple object that performs
user-provided actions upon destruction. It is meant to simplify cleanup
tasks in error handling paths.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Xavier Roumegue <xavier.roumegue@oss.nxp.com>
---
Changes since v1:

- Rename to ScopeExitActions
- Replace defer() with operator+=()
- Replace clear() with release()
---
 include/libcamera/base/utils.h |  13 ++++
 src/libcamera/base/utils.cpp   | 132 +++++++++++++++++++++++++++++++++
 2 files changed, 145 insertions(+)

Hi Laurent

On Mon, Oct 17, 2022 at 12:02:01AM +0300, Laurent Pinchart via libcamera-devel wrote:
> The ScopeExitActions class is a simple object that performs
> user-provided actions upon destruction. It is meant to simplify cleanup
> tasks in error handling paths.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Xavier Roumegue <xavier.roumegue@oss.nxp.com>
> ---
> Changes since v1:
>
> - Rename to ScopeExitActions

I will avoid bikeshedding on names, all the alternatives I can think
of are not much better

> - Replace defer() with operator+=()
> - Replace clear() with release()
> ---
>  include/libcamera/base/utils.h |  13 ++++
>  src/libcamera/base/utils.cpp   | 132 +++++++++++++++++++++++++++++++++
>  2 files changed, 145 insertions(+)
>
> diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h
> index eb7bcdf4c173..2941d9c45232 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>
> @@ -367,6 +368,18 @@ decltype(auto) abs_diff(const T &a, const T &b)
>  		return a - b;
>  }
>
> +class ScopeExitActions
> +{
> +public:
> +	~ScopeExitActions();
> +
> +	void operator+=(std::function<void()> &&action);
> +	void release();
> +
> +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 6a307940448e..eb91da297c57 100644
> --- a/src/libcamera/base/utils.cpp
> +++ b/src/libcamera/base/utils.cpp
> @@ -463,6 +463,138 @@ std::string toAscii(const std::string &str)
>   * \a b
>   */
>
> +/**
> + * \class ScopeExitActions
> + * \brief An object that performs actions upon destruction
> + *
> + * The ScopeExitActions class is a simple object that performs user-provided
> + * actions upon destruction. 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 = startProducer();
> + * 	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 = startProducer();
> + * 	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 ScopeExitActions class solves these issues by allowing code that
> + * requires cleanup actions to be grouped with its corresponding deferred error
> + * handling code:
> + *
> + * \code{.cpp}
> + * {
> + * 	ScopeExitActions actions;
> + *
> + * 	int ret = allocateMemory();
> + * 	if (ret)
> + * 		return ret;
> + *
> + * 	actions += [&]() { freeMemory(); };
> + *
> + * 	ret = startProducer();
> + * 	if (ret)
> + * 		return ret;
> + *
> + * 	actions += [&]() { stopProducer(); };
> + *
> + * 	ret = startConsumer();
> + * 	if (ret)
> + * 		return ret;
> + *
> + * 	actions.release();
> + * 	return 0;
> + * }
> + * \endcode
> + *
> + * Deferred error handlers are executed when the ScopeExitActions instance is
> + * destroyed, in the reverse order of their addition.
> + */
> +
> +ScopeExitActions::~ScopeExitActions()

Doesn't doxygen complain for the missing documentation ?

> +{
> +	for (const auto &action : utils::reverse(actions_))
> +		action();
> +}
> +
> +/**
> + * \brief Add a deferred action
> + * \param[in] action The action
> + *
> + * Add a deferred action to the ScopeExitActions. Actions will be called upon
> + * destruction in the reverse order of their addition.
> + */
> +void ScopeExitActions::operator+=(std::function<void()> &&action)
> +{
> +	actions_.push_back(std::move(action));

In case of:

        if (error) {
                actions += [&]() { do_something(); };
                return ret;
        }

The lambda expression is wrapped into a temprary std::function, which
is moved into the vector. At "return ret;" the ScopeExitAction is
destroyed and the cleanup actions called in reverse sequence.

In case of the following, instead:

        error = do_first();
        if (error)
                actions += [&]() { clean_first(); };

        error = do_second();
        if (error)
                actions += [&]() { clean_second(); };

        return error;

The lambda functions wrapped by the temporaries std::function<> will
still be valid or do they live in the if (error) { } scope and once we invoke
the action it will point to invalid memory ?

What if we instead copy the std::function<> in the vector of
actions to allow such a pattern ? Would only the wrapper be copied or
the target as well ?

> +}
> +
> +/**
> + * \brief Remove all deferred actions

You seem to have removed defer from the code but kept in
documentation.

Should the usages of "deferred actions" be replaced by "exit actions"
or "cleanup actions", or is it intentional ?

Thanks
   j

> + *
> + * This function should be called in scope exit paths that don't need the
> + * actions to be executed, such as success return paths from a function when
> + * the ScopeExitActions is used for error cleanup.
> + */
> +void ScopeExitActions::release()
> +{
> +	actions_.clear();
> +}
> +
>  } /* namespace utils */
>
>  #ifndef __DOXYGEN__
> --
> Regards,
>
> Laurent Pinchart
>

Hi Jacopo,

On Mon, Oct 17, 2022 at 10:46:10AM +0200, Jacopo Mondi wrote:
> On Mon, Oct 17, 2022 at 12:02:01AM +0300, Laurent Pinchart via libcamera-devel wrote:
> > The ScopeExitActions class is a simple object that performs
> > user-provided actions upon destruction. It is meant to simplify cleanup
> > tasks in error handling paths.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Reviewed-by: Xavier Roumegue <xavier.roumegue@oss.nxp.com>
> > ---
> > Changes since v1:
> >
> > - Rename to ScopeExitActions
> 
> I will avoid bikeshedding on names, all the alternatives I can think
> of are not much better
> 
> > - Replace defer() with operator+=()
> > - Replace clear() with release()
> > ---
> >  include/libcamera/base/utils.h |  13 ++++
> >  src/libcamera/base/utils.cpp   | 132 +++++++++++++++++++++++++++++++++
> >  2 files changed, 145 insertions(+)
> >
> > diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h
> > index eb7bcdf4c173..2941d9c45232 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>
> > @@ -367,6 +368,18 @@ decltype(auto) abs_diff(const T &a, const T &b)
> >  		return a - b;
> >  }
> >
> > +class ScopeExitActions
> > +{
> > +public:
> > +	~ScopeExitActions();
> > +
> > +	void operator+=(std::function<void()> &&action);
> > +	void release();
> > +
> > +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 6a307940448e..eb91da297c57 100644
> > --- a/src/libcamera/base/utils.cpp
> > +++ b/src/libcamera/base/utils.cpp
> > @@ -463,6 +463,138 @@ std::string toAscii(const std::string &str)
> >   * \a b
> >   */
> >
> > +/**
> > + * \class ScopeExitActions
> > + * \brief An object that performs actions upon destruction
> > + *
> > + * The ScopeExitActions class is a simple object that performs user-provided
> > + * actions upon destruction. 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 = startProducer();
> > + * 	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 = startProducer();
> > + * 	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 ScopeExitActions class solves these issues by allowing code that
> > + * requires cleanup actions to be grouped with its corresponding deferred error
> > + * handling code:
> > + *
> > + * \code{.cpp}
> > + * {
> > + * 	ScopeExitActions actions;
> > + *
> > + * 	int ret = allocateMemory();
> > + * 	if (ret)
> > + * 		return ret;
> > + *
> > + * 	actions += [&]() { freeMemory(); };
> > + *
> > + * 	ret = startProducer();
> > + * 	if (ret)
> > + * 		return ret;
> > + *
> > + * 	actions += [&]() { stopProducer(); };
> > + *
> > + * 	ret = startConsumer();
> > + * 	if (ret)
> > + * 		return ret;
> > + *
> > + * 	actions.release();
> > + * 	return 0;
> > + * }
> > + * \endcode
> > + *
> > + * Deferred error handlers are executed when the ScopeExitActions instance is
> > + * destroyed, in the reverse order of their addition.
> > + */
> > +
> > +ScopeExitActions::~ScopeExitActions()
> 
> Doesn't doxygen complain for the missing documentation ?

Not for destructors, no.

> > +{
> > +	for (const auto &action : utils::reverse(actions_))
> > +		action();
> > +}
> > +
> > +/**
> > + * \brief Add a deferred action
> > + * \param[in] action The action
> > + *
> > + * Add a deferred action to the ScopeExitActions. Actions will be called upon
> > + * destruction in the reverse order of their addition.
> > + */
> > +void ScopeExitActions::operator+=(std::function<void()> &&action)
> > +{
> > +	actions_.push_back(std::move(action));
> 
> In case of:
> 
>         if (error) {
>                 actions += [&]() { do_something(); };
>                 return ret;
>         }
> 
> The lambda expression is wrapped into a temprary std::function, which
> is moved into the vector. At "return ret;" the ScopeExitAction is
> destroyed and the cleanup actions called in reverse sequence.
> 
> In case of the following, instead:
> 
>         error = do_first();
>         if (error)
>                 actions += [&]() { clean_first(); };
> 
>         error = do_second();
>         if (error)
>                 actions += [&]() { clean_second(); };
> 
>         return error;

Note this is not the expected usage pattern. Callers will do

	error = do_first();
	if (error)
		return error;
	actions += [&]() { clean_first(); };

	error = do_second();
	if (error)
		return error;
	actions += [&]() { clean_second(); };

	actions.release();
	return 0;

> The lambda functions wrapped by the temporaries std::function<> will
> still be valid or do they live in the if (error) { } scope and once we invoke
> the action it will point to invalid memory ?

https://en.cppreference.com/w/cpp/language/lambda

ClosureType has copy and move constructors and assignment operators.

> What if we instead copy the std::function<> in the vector of
> actions to allow such a pattern ? Would only the wrapper be copied or
> the target as well ?

I'm not sure to see what difference it would make.

> > +}
> > +
> > +/**
> > + * \brief Remove all deferred actions
> 
> You seem to have removed defer from the code but kept in
> documentation.
> 
> Should the usages of "deferred actions" be replaced by "exit actions"
> or "cleanup actions", or is it intentional ?

I'll fix the leftovers.

> > + *
> > + * This function should be called in scope exit paths that don't need the
> > + * actions to be executed, such as success return paths from a function when
> > + * the ScopeExitActions is used for error cleanup.
> > + */
> > +void ScopeExitActions::release()
> > +{
> > +	actions_.clear();
> > +}
> > +
> >  } /* namespace utils */
> >
> >  #ifndef __DOXYGEN__

Hi Laurent

On Mon, Oct 17, 2022 at 11:58:21AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Mon, Oct 17, 2022 at 10:46:10AM +0200, Jacopo Mondi wrote:
> > On Mon, Oct 17, 2022 at 12:02:01AM +0300, Laurent Pinchart via libcamera-devel wrote:
> > > The ScopeExitActions class is a simple object that performs
> > > user-provided actions upon destruction. It is meant to simplify cleanup
> > > tasks in error handling paths.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Reviewed-by: Xavier Roumegue <xavier.roumegue@oss.nxp.com>
> > > ---
> > > Changes since v1:
> > >
> > > - Rename to ScopeExitActions
> >
> > I will avoid bikeshedding on names, all the alternatives I can think
> > of are not much better
> >
> > > - Replace defer() with operator+=()
> > > - Replace clear() with release()
> > > ---
> > >  include/libcamera/base/utils.h |  13 ++++
> > >  src/libcamera/base/utils.cpp   | 132 +++++++++++++++++++++++++++++++++
> > >  2 files changed, 145 insertions(+)
> > >
> > > diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h
> > > index eb7bcdf4c173..2941d9c45232 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>
> > > @@ -367,6 +368,18 @@ decltype(auto) abs_diff(const T &a, const T &b)
> > >  		return a - b;
> > >  }
> > >
> > > +class ScopeExitActions
> > > +{
> > > +public:
> > > +	~ScopeExitActions();
> > > +
> > > +	void operator+=(std::function<void()> &&action);
> > > +	void release();
> > > +
> > > +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 6a307940448e..eb91da297c57 100644
> > > --- a/src/libcamera/base/utils.cpp
> > > +++ b/src/libcamera/base/utils.cpp
> > > @@ -463,6 +463,138 @@ std::string toAscii(const std::string &str)
> > >   * \a b
> > >   */
> > >
> > > +/**
> > > + * \class ScopeExitActions
> > > + * \brief An object that performs actions upon destruction
> > > + *
> > > + * The ScopeExitActions class is a simple object that performs user-provided
> > > + * actions upon destruction. 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 = startProducer();
> > > + * 	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 = startProducer();
> > > + * 	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 ScopeExitActions class solves these issues by allowing code that
> > > + * requires cleanup actions to be grouped with its corresponding deferred error
> > > + * handling code:
> > > + *
> > > + * \code{.cpp}
> > > + * {
> > > + * 	ScopeExitActions actions;
> > > + *
> > > + * 	int ret = allocateMemory();
> > > + * 	if (ret)
> > > + * 		return ret;
> > > + *
> > > + * 	actions += [&]() { freeMemory(); };
> > > + *
> > > + * 	ret = startProducer();
> > > + * 	if (ret)
> > > + * 		return ret;
> > > + *
> > > + * 	actions += [&]() { stopProducer(); };
> > > + *
> > > + * 	ret = startConsumer();
> > > + * 	if (ret)
> > > + * 		return ret;
> > > + *
> > > + * 	actions.release();
> > > + * 	return 0;
> > > + * }
> > > + * \endcode
> > > + *
> > > + * Deferred error handlers are executed when the ScopeExitActions instance is
> > > + * destroyed, in the reverse order of their addition.
> > > + */
> > > +
> > > +ScopeExitActions::~ScopeExitActions()
> >
> > Doesn't doxygen complain for the missing documentation ?
>
> Not for destructors, no.
>
> > > +{
> > > +	for (const auto &action : utils::reverse(actions_))
> > > +		action();
> > > +}
> > > +
> > > +/**
> > > + * \brief Add a deferred action
> > > + * \param[in] action The action
> > > + *
> > > + * Add a deferred action to the ScopeExitActions. Actions will be called upon
> > > + * destruction in the reverse order of their addition.
> > > + */
> > > +void ScopeExitActions::operator+=(std::function<void()> &&action)
> > > +{
> > > +	actions_.push_back(std::move(action));
> >
> > In case of:
> >
> >         if (error) {
> >                 actions += [&]() { do_something(); };
> >                 return ret;
> >         }
> >
> > The lambda expression is wrapped into a temprary std::function, which
> > is moved into the vector. At "return ret;" the ScopeExitAction is
> > destroyed and the cleanup actions called in reverse sequence.
> >
> > In case of the following, instead:
> >
> >         error = do_first();
> >         if (error)
> >                 actions += [&]() { clean_first(); };
> >
> >         error = do_second();
> >         if (error)
> >                 actions += [&]() { clean_second(); };
> >
> >         return error;
>
> Note this is not the expected usage pattern. Callers will do
>
> 	error = do_first();
> 	if (error)
> 		return error;
> 	actions += [&]() { clean_first(); };
>
> 	error = do_second();
> 	if (error)
> 		return error;
> 	actions += [&]() { clean_second(); };
>
> 	actions.release();
> 	return 0;

Why ? What prevents the usage of ScopeExitAction in patterns where
instead of returning immediately, you continue ?

If we introduce a contruct and only prescribe a single usage pattern,
we would have to review it even more carefully.

>
> > The lambda functions wrapped by the temporaries std::function<> will
> > still be valid or do they live in the if (error) { } scope and once we invoke
> > the action it will point to invalid memory ?
>
> https://en.cppreference.com/w/cpp/language/lambda
>
> ClosureType has copy and move constructors and assignment operators.
>
> > What if we instead copy the std::function<> in the vector of
> > actions to allow such a pattern ? Would only the wrapper be copied or
> > the target as well ?
>
> I'm not sure to see what difference it would make.
>

I was wondering if moving the std::function<> moves the target as
well, or it will end up pointing to invalid memory.

In other words, is the above usage pattern valid or will it crash ?

> > > +}
> > > +
> > > +/**
> > > + * \brief Remove all deferred actions
> >
> > You seem to have removed defer from the code but kept in
> > documentation.
> >
> > Should the usages of "deferred actions" be replaced by "exit actions"
> > or "cleanup actions", or is it intentional ?
>
> I'll fix the leftovers.
>
> > > + *
> > > + * This function should be called in scope exit paths that don't need the
> > > + * actions to be executed, such as success return paths from a function when
> > > + * the ScopeExitActions is used for error cleanup.
> > > + */
> > > +void ScopeExitActions::release()
> > > +{
> > > +	actions_.clear();
> > > +}
> > > +
> > >  } /* namespace utils */
> > >
> > >  #ifndef __DOXYGEN__
>
> --
> Regards,
>
> Laurent Pinchart

Hi Jacopo,

On Mon, Oct 17, 2022 at 11:28:51AM +0200, Jacopo Mondi wrote:
> On Mon, Oct 17, 2022 at 11:58:21AM +0300, Laurent Pinchart wrote:
> > On Mon, Oct 17, 2022 at 10:46:10AM +0200, Jacopo Mondi wrote:
> > > On Mon, Oct 17, 2022 at 12:02:01AM +0300, Laurent Pinchart via libcamera-devel wrote:
> > > > The ScopeExitActions class is a simple object that performs
> > > > user-provided actions upon destruction. It is meant to simplify cleanup
> > > > tasks in error handling paths.
> > > >
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > Reviewed-by: Xavier Roumegue <xavier.roumegue@oss.nxp.com>
> > > > ---
> > > > Changes since v1:
> > > >
> > > > - Rename to ScopeExitActions
> > >
> > > I will avoid bikeshedding on names, all the alternatives I can think
> > > of are not much better
> > >
> > > > - Replace defer() with operator+=()
> > > > - Replace clear() with release()
> > > > ---
> > > >  include/libcamera/base/utils.h |  13 ++++
> > > >  src/libcamera/base/utils.cpp   | 132 +++++++++++++++++++++++++++++++++
> > > >  2 files changed, 145 insertions(+)
> > > >
> > > > diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h
> > > > index eb7bcdf4c173..2941d9c45232 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>
> > > > @@ -367,6 +368,18 @@ decltype(auto) abs_diff(const T &a, const T &b)
> > > >  		return a - b;
> > > >  }
> > > >
> > > > +class ScopeExitActions
> > > > +{
> > > > +public:
> > > > +	~ScopeExitActions();
> > > > +
> > > > +	void operator+=(std::function<void()> &&action);
> > > > +	void release();
> > > > +
> > > > +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 6a307940448e..eb91da297c57 100644
> > > > --- a/src/libcamera/base/utils.cpp
> > > > +++ b/src/libcamera/base/utils.cpp
> > > > @@ -463,6 +463,138 @@ std::string toAscii(const std::string &str)
> > > >   * \a b
> > > >   */
> > > >
> > > > +/**
> > > > + * \class ScopeExitActions
> > > > + * \brief An object that performs actions upon destruction
> > > > + *
> > > > + * The ScopeExitActions class is a simple object that performs user-provided
> > > > + * actions upon destruction. 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 = startProducer();
> > > > + * 	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 = startProducer();
> > > > + * 	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 ScopeExitActions class solves these issues by allowing code that
> > > > + * requires cleanup actions to be grouped with its corresponding deferred error
> > > > + * handling code:
> > > > + *
> > > > + * \code{.cpp}
> > > > + * {
> > > > + * 	ScopeExitActions actions;
> > > > + *
> > > > + * 	int ret = allocateMemory();
> > > > + * 	if (ret)
> > > > + * 		return ret;
> > > > + *
> > > > + * 	actions += [&]() { freeMemory(); };
> > > > + *
> > > > + * 	ret = startProducer();
> > > > + * 	if (ret)
> > > > + * 		return ret;
> > > > + *
> > > > + * 	actions += [&]() { stopProducer(); };
> > > > + *
> > > > + * 	ret = startConsumer();
> > > > + * 	if (ret)
> > > > + * 		return ret;
> > > > + *
> > > > + * 	actions.release();
> > > > + * 	return 0;
> > > > + * }
> > > > + * \endcode
> > > > + *
> > > > + * Deferred error handlers are executed when the ScopeExitActions instance is
> > > > + * destroyed, in the reverse order of their addition.
> > > > + */
> > > > +
> > > > +ScopeExitActions::~ScopeExitActions()
> > >
> > > Doesn't doxygen complain for the missing documentation ?
> >
> > Not for destructors, no.
> >
> > > > +{
> > > > +	for (const auto &action : utils::reverse(actions_))
> > > > +		action();
> > > > +}
> > > > +
> > > > +/**
> > > > + * \brief Add a deferred action
> > > > + * \param[in] action The action
> > > > + *
> > > > + * Add a deferred action to the ScopeExitActions. Actions will be called upon
> > > > + * destruction in the reverse order of their addition.
> > > > + */
> > > > +void ScopeExitActions::operator+=(std::function<void()> &&action)
> > > > +{
> > > > +	actions_.push_back(std::move(action));
> > >
> > > In case of:
> > >
> > >         if (error) {
> > >                 actions += [&]() { do_something(); };
> > >                 return ret;
> > >         }
> > >
> > > The lambda expression is wrapped into a temprary std::function, which
> > > is moved into the vector. At "return ret;" the ScopeExitAction is
> > > destroyed and the cleanup actions called in reverse sequence.
> > >
> > > In case of the following, instead:
> > >
> > >         error = do_first();
> > >         if (error)
> > >                 actions += [&]() { clean_first(); };
> > >
> > >         error = do_second();
> > >         if (error)
> > >                 actions += [&]() { clean_second(); };
> > >
> > >         return error;
> >
> > Note this is not the expected usage pattern. Callers will do
> >
> > 	error = do_first();
> > 	if (error)
> > 		return error;
> > 	actions += [&]() { clean_first(); };
> >
> > 	error = do_second();
> > 	if (error)
> > 		return error;
> > 	actions += [&]() { clean_second(); };
> >
> > 	actions.release();
> > 	return 0;
> 
> Why ? What prevents the usage of ScopeExitAction in patterns where
> instead of returning immediately, you continue ?
> 
> If we introduce a contruct and only prescribe a single usage pattern,
> we would have to review it even more carefully.

It's not that something *prevents* it, I'm just not sure to see what the
code above is supposed to do. In any case, calling operator+=() in
sub-scope is absolutely valid, there's an example in patch 2/2.

> > > The lambda functions wrapped by the temporaries std::function<> will
> > > still be valid or do they live in the if (error) { } scope and once we invoke
> > > the action it will point to invalid memory ?
> >
> > https://en.cppreference.com/w/cpp/language/lambda
> >
> > ClosureType has copy and move constructors and assignment operators.
> >
> > > What if we instead copy the std::function<> in the vector of
> > > actions to allow such a pattern ? Would only the wrapper be copied or
> > > the target as well ?
> >
> > I'm not sure to see what difference it would make.
> 
> I was wondering if moving the std::function<> moves the target as
> well, or it will end up pointing to invalid memory.
> 
> In other words, is the above usage pattern valid or will it crash ?

As far as I can tell, it will not crash.

> > > > +}
> > > > +
> > > > +/**
> > > > + * \brief Remove all deferred actions
> > >
> > > You seem to have removed defer from the code but kept in
> > > documentation.
> > >
> > > Should the usages of "deferred actions" be replaced by "exit actions"
> > > or "cleanup actions", or is it intentional ?
> >
> > I'll fix the leftovers.
> >
> > > > + *
> > > > + * This function should be called in scope exit paths that don't need the
> > > > + * actions to be executed, such as success return paths from a function when
> > > > + * the ScopeExitActions is used for error cleanup.
> > > > + */
> > > > +void ScopeExitActions::release()
> > > > +{
> > > > +	actions_.clear();
> > > > +}
> > > > +
> > > >  } /* namespace utils */
> > > >
> > > >  #ifndef __DOXYGEN__