[libcamera-devel] [PATCH] Documentation: coding-style: Document error handling rules

[Laurent Pinchart]

Hi Kieran,

On Fri, Oct 29, 2021 at 03:30:04PM +0100, Kieran Bingham wrote:
> Quoting Laurent Pinchart (2021-10-27 23:59:51)
> > Following a conversation on the mailing list about the use of
> > assertions, document the error handling strategy in libcamera. This is
> > an initial set of rules that are expected be extended and detailed in
> > the future.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> Sounds like a pretty good explanation and worth adding to me.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> 
> I wonder if we might expand that we consider even LOG(Fatal) should
> return rather than continue as it may not fire an assertion in release
> builds.
> 
> But that's not a specific worry right now.

I haven't done so because we haven't decided what to do in this area.
Let's sort it out, reach an agreement, and then document it :-)

> > ---
> >  Documentation/coding-style.rst | 30 ++++++++++++++++++++++++++++++
> >  1 file changed, 30 insertions(+)
> > 
> > diff --git a/Documentation/coding-style.rst b/Documentation/coding-style.rst
> > index ef3a0d1714ab..4e8d6988fef8 100644
> > --- a/Documentation/coding-style.rst
> > +++ b/Documentation/coding-style.rst
> > @@ -217,6 +217,36 @@ global variable, should run a minimal amount of code in the constructor and
> >  destructor, and code that contains dependencies should be moved to a later
> >  point in time. 
> >  
> > +Error Handling
> > +~~~~~~~~~~~~~~
> > +
> > +Proper error handling is crucial to the stability of libcamera. The project
> > +follows a set of high-level rules:
> > +
> > +* Make errors impossible through API design. The best way to handle errors is
> > +  to prevent them from happening in the first place. The preferred option is
> > +  thus to prevent error conditions at the API design stage when possible.
> > +* Detect errors at compile time. Compile-test checking of errors not only
> > +  reduces the runtime complexity, but also ensures that errors are caught early
> > +  on during development instead of during testing or, worse, in production. The
> > +  static_assert() declaration should be used where possible for this purpose.
> > +* Validate all external API contracts. Explicit pre-condition checks shall be
> > +  used to validate API contracts. Whenever possible, appropriate errors should
> > +  be returned directly. As libcamera doesn't use exceptions, errors detected in
> > +  constructors shall result in the constructed object being marked as invalid,
> > +  with a public member function available to check validity. The checks should
> > +  be thorough for the public API, and may be lighter for internal APIs when
> > +  pre-conditions can reasonably be considered to be met through other means.
> > +* Use assertions for fatal issues only. The ASSERT() macro causes a program
> > +  abort when compiled in debug mode, and is a no-op otherwise. It is useful to
> > +  abort execution synchronously with the error check instead of letting the
> > +  error cause problems (such as segmentation faults) later, and to provide a
> > +  detailed backtrace. Assertions shall only be used to catch conditions that are
> > +  never supposed to happen without a serious bug in libcamera that would prevent
> > +  safe recovery. They shall never be used to validate API contracts. The
> > +  assertion conditions shall not cause any side effect as they are compiled out
> > +  in non-debug mode.
> > +
> >  C Compatibility Headers
> >  ~~~~~~~~~~~~~~~~~~~~~~~
> >  
> > 
> > base-commit: 76bd9f3d80cb99a3391832b644b65a619427ed00

-- 
Regards,

Laurent Pinchart