[libcamera-devel] [PATCH 1/2] libcamera: utils: Add Cleaner class
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Oct 11 22:29:37 CEST 2022
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 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.
>
> 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__
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list