[{"id":20639,"web_url":"https://patchwork.libcamera.org/comment/20639/","msgid":"<163551780413.143624.9009934762160877382@Monstersaurus>","date":"2021-10-29T14:30:04","subject":"Re: [libcamera-devel] [PATCH] Documentation: coding-style: Document\n\terror handling rules","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\n\nQuoting Laurent Pinchart (2021-10-27 23:59:51)\n> Following a conversation on the mailing list about the use of\n> assertions, document the error handling strategy in libcamera. This is\n> an initial set of rules that are expected be extended and detailed in\n> the future.\n> \n> Signed-off-by: Laurent Pinchart \n\nSounds like a pretty good explanation and worth adding to me.\n\nReviewed-by: Kieran Bingham \n\n\nI wonder if we might expand that we consider even LOG(Fatal) should\nreturn rather than continue as it may not fire an assertion in release\nbuilds.\n\nBut that's not a specific worry right now.\n\n\n> ---\n> Documentation/coding-style.rst | 30 ++++++++++++++++++++++++++++++\n> 1 file changed, 30 insertions(+)\n> \n> diff --git a/Documentation/coding-style.rst b/Documentation/coding-style.rst\n> index ef3a0d1714ab..4e8d6988fef8 100644\n> --- a/Documentation/coding-style.rst\n> +++ b/Documentation/coding-style.rst\n> @@ -217,6 +217,36 @@ global variable, should run a minimal amount of code in the constructor and\n> destructor, and code that contains dependencies should be moved to a later\n> point in time. \n> \n> +Error Handling\n> +~~~~~~~~~~~~~~\n> +\n> +Proper error handling is crucial to the stability of libcamera. The project\n> +follows a set of high-level rules:\n> +\n> +* Make errors impossible through API design. The best way to handle errors is\n> + to prevent them from happening in the first place. The preferred option is\n> + thus to prevent error conditions at the API design stage when possible.\n> +* Detect errors at compile time. Compile-test checking of errors not only\n> + reduces the runtime complexity, but also ensures that errors are caught early\n> + on during development instead of during testing or, worse, in production. The\n> + static_assert() declaration should be used where possible for this purpose.\n> +* Validate all external API contracts. Explicit pre-condition checks shall be\n> + used to validate API contracts. Whenever possible, appropriate errors should\n> + be returned directly. As libcamera doesn't use exceptions, errors detected in\n> + constructors shall result in the constructed object being marked as invalid,\n> + with a public member function available to check validity. The checks should\n> + be thorough for the public API, and may be lighter for internal APIs when\n> + pre-conditions can reasonably be considered to be met through other means.\n> +* Use assertions for fatal issues only. The ASSERT() macro causes a program\n> + abort when compiled in debug mode, and is a no-op otherwise. It is useful to\n> + abort execution synchronously with the error check instead of letting the\n> + error cause problems (such as segmentation faults) later, and to provide a\n> + detailed backtrace. Assertions shall only be used to catch conditions that are\n> + never supposed to happen without a serious bug in libcamera that would prevent\n> + safe recovery. They shall never be used to validate API contracts. The\n> + assertion conditions shall not cause any side effect as they are compiled out\n> + in non-debug mode.\n> +\n> C Compatibility Headers\n> ~~~~~~~~~~~~~~~~~~~~~~~\n> \n> \n> base-commit: 76bd9f3d80cb99a3391832b644b65a619427ed00\n> -- \n> Regards,\n> \n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 3017EBDB1C\n\tfor ;\n\tFri, 29 Oct 2021 14:30:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 41A8F600BD;\n\tFri, 29 Oct 2021 16:30:09 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 287C0600B8\n\tfor ;\n\tFri, 29 Oct 2021 16:30:07 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BB3D8881;\n\tFri, 29 Oct 2021 16:30:06 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"UaIT6ZJi\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1635517806;\n\tbh=+G2MOptxIlVTfWl8/TO1NMOal/JVHILSSSfYFR07zLs=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=UaIT6ZJiNFA4oU8jgd49sEudHlu8g6YBft74zHWgyHEvlZII8S7mzMKWLf/0TMXC8\n\tI/3bDEay72qxeIaGJsRIKqKdo17w4I1icyZJznrLPDq0XAMG0pMuWZGKqA8QYTFaQg\n\terqzRzt2QkcWjHnOVWiZcwKA9ztoPcOrKUYufQQk=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211027225951.10578-1-laurent.pinchart@ideasonboard.com>","References":"<20211027225951.10578-1-laurent.pinchart@ideasonboard.com>","From":"Kieran Bingham ","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Fri, 29 Oct 2021 15:30:04 +0100","Message-ID":"<163551780413.143624.9009934762160877382@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [PATCH] Documentation: coding-style: Document\n\terror handling rules","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20643,"web_url":"https://patchwork.libcamera.org/comment/20643/","msgid":"","date":"2021-10-30T14:39:49","subject":"Re: [libcamera-devel] [PATCH] Documentation: coding-style: Document\n\terror handling rules","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Fri, Oct 29, 2021 at 03:30:04PM +0100, Kieran Bingham wrote:\n> Quoting Laurent Pinchart (2021-10-27 23:59:51)\n> > Following a conversation on the mailing list about the use of\n> > assertions, document the error handling strategy in libcamera. This is\n> > an initial set of rules that are expected be extended and detailed in\n> > the future.\n> > \n> > Signed-off-by: Laurent Pinchart \n> \n> Sounds like a pretty good explanation and worth adding to me.\n> \n> Reviewed-by: Kieran Bingham \n> \n> \n> I wonder if we might expand that we consider even LOG(Fatal) should\n> return rather than continue as it may not fire an assertion in release\n> builds.\n> \n> But that's not a specific worry right now.\n\nI haven't done so because we haven't decided what to do in this area.\nLet's sort it out, reach an agreement, and then document it :-)\n\n> > ---\n> > Documentation/coding-style.rst | 30 ++++++++++++++++++++++++++++++\n> > 1 file changed, 30 insertions(+)\n> > \n> > diff --git a/Documentation/coding-style.rst b/Documentation/coding-style.rst\n> > index ef3a0d1714ab..4e8d6988fef8 100644\n> > --- a/Documentation/coding-style.rst\n> > +++ b/Documentation/coding-style.rst\n> > @@ -217,6 +217,36 @@ global variable, should run a minimal amount of code in the constructor and\n> > destructor, and code that contains dependencies should be moved to a later\n> > point in time. \n> > \n> > +Error Handling\n> > +~~~~~~~~~~~~~~\n> > +\n> > +Proper error handling is crucial to the stability of libcamera. The project\n> > +follows a set of high-level rules:\n> > +\n> > +* Make errors impossible through API design. The best way to handle errors is\n> > + to prevent them from happening in the first place. The preferred option is\n> > + thus to prevent error conditions at the API design stage when possible.\n> > +* Detect errors at compile time. Compile-test checking of errors not only\n> > + reduces the runtime complexity, but also ensures that errors are caught early\n> > + on during development instead of during testing or, worse, in production. The\n> > + static_assert() declaration should be used where possible for this purpose.\n> > +* Validate all external API contracts. Explicit pre-condition checks shall be\n> > + used to validate API contracts. Whenever possible, appropriate errors should\n> > + be returned directly. As libcamera doesn't use exceptions, errors detected in\n> > + constructors shall result in the constructed object being marked as invalid,\n> > + with a public member function available to check validity. The checks should\n> > + be thorough for the public API, and may be lighter for internal APIs when\n> > + pre-conditions can reasonably be considered to be met through other means.\n> > +* Use assertions for fatal issues only. The ASSERT() macro causes a program\n> > + abort when compiled in debug mode, and is a no-op otherwise. It is useful to\n> > + abort execution synchronously with the error check instead of letting the\n> > + error cause problems (such as segmentation faults) later, and to provide a\n> > + detailed backtrace. Assertions shall only be used to catch conditions that are\n> > + never supposed to happen without a serious bug in libcamera that would prevent\n> > + safe recovery. They shall never be used to validate API contracts. The\n> > + assertion conditions shall not cause any side effect as they are compiled out\n> > + in non-debug mode.\n> > +\n> > C Compatibility Headers\n> > ~~~~~~~~~~~~~~~~~~~~~~~\n> > \n> > \n> > base-commit: 76bd9f3d80cb99a3391832b644b65a619427ed00","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id BCDD9BDB1C\n\tfor ;\n\tSat, 30 Oct 2021 14:40:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2F3AF600C2;\n\tSat, 30 Oct 2021 16:40:28 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E5AE8600B6\n\tfor ;\n\tSat, 30 Oct 2021 16:40:26 +0200 (CEST)","from pendragon.ideasonboard.com (85-76-103-215-nat.elisa-mobile.fi\n\t[85.76.103.215])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EAB04881;\n\tSat, 30 Oct 2021 16:40:23 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"KxusJb36\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1635604826;\n\tbh=y/enN+xgBshvwoigebRH6j6qaRYlEQn5EXCWZQlR9Lk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=KxusJb36jaJDBXJnrd36yRhpXA6ha2qlVtXmrSalfuXLYkfYEyHgDAuUPiixXBEvA\n\t6ES+URW1O0bVnY12IymGxFxUI2qM5+4B+0FLPQpKoMlQ9LdeLP3DuMo/rdpw5RRGcY\n\tLfikXrv9g8iaKDQnwnkWJqU3WWwJn2ycddqNyS9w=","Date":"Sat, 30 Oct 2021 17:39:49 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20211027225951.10578-1-laurent.pinchart@ideasonboard.com>\n\t<163551780413.143624.9009934762160877382@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<163551780413.143624.9009934762160877382@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH] Documentation: coding-style: Document\n\terror handling rules","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]