[libcamera-devel] [PATCH 1/2] libcamera: utils: Add Cleaner class
paul.elder at ideasonboard.com
paul.elder at ideasonboard.com
Thu Oct 20 07:46:27 CEST 2022
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 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.
>
> 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 at 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
More information about the libcamera-devel
mailing list