[libcamera-devel] [PATCH v2 1/2] libcamera: utils: Add ScopeExitActions class
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Oct 17 11:50:08 CEST 2022
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 at ideasonboard.com>
> > > > Reviewed-by: Xavier Roumegue <xavier.roumegue at 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__
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list