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

[Jacopo Mondi]

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