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

14371 diff mbox series

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@ideasonboard.com>
---
 Documentation/coding-style.rst | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)


base-commit: 76bd9f3d80cb99a3391832b644b65a619427ed00

Hi Laurent,


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@ideasonboard.com>

Sounds like a pretty good explanation and worth adding to me.

Reviewed-by: Kieran Bingham <kieran.bingham@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.


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

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@ideasonboard.com>
> 
> Sounds like a pretty good explanation and worth adding to me.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@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