Cover Letter Detail
Show a cover letter.
GET /api/1.1/covers/20758/?format=api
{ "id": 20758, "url": "https://patchwork.libcamera.org/api/1.1/covers/20758/?format=api", "web_url": "https://patchwork.libcamera.org/cover/20758/", "project": { "id": 1, "url": "https://patchwork.libcamera.org/api/1.1/projects/1/?format=api", "name": "libcamera", "link_name": "libcamera", "list_id": "libcamera_core", "list_email": "libcamera-devel@lists.libcamera.org", "web_url": "", "scm_url": "", "webscm_url": "" }, "msgid": "<20240804153106.22167-1-laurent.pinchart@ideasonboard.com>", "date": "2024-08-04T15:31:04", "name": "[v3,0/2] libcamera: Simplify error handling with ScopeExitActions class", "submitter": { "id": 2, "url": "https://patchwork.libcamera.org/api/1.1/people/2/?format=api", "name": "Laurent Pinchart", "email": "laurent.pinchart@ideasonboard.com" }, "mbox": "https://patchwork.libcamera.org/cover/20758/mbox/", "series": [ { "id": 4482, "url": "https://patchwork.libcamera.org/api/1.1/series/4482/?format=api", "web_url": "https://patchwork.libcamera.org/project/libcamera/list/?series=4482", "date": "2024-08-04T15:31:04", "name": "libcamera: Simplify error handling with ScopeExitActions class", "version": 3, "mbox": "https://patchwork.libcamera.org/series/4482/mbox/" } ], "comments": "https://patchwork.libcamera.org/api/covers/20758/comments/", "headers": { "Return-Path": "<libcamera-devel-bounces@lists.libcamera.org>", "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 B7E17C323E\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSun, 4 Aug 2024 15:31:31 +0000 (UTC)", "from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CEF0C63381;\n\tSun, 4 Aug 2024 17:31:30 +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 0BF016192D\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun, 4 Aug 2024 17:31:29 +0200 (CEST)", "from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 03CD71BA\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun, 4 Aug 2024 17:30:37 +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=\"KNHuaRGU\"; dkim-atps=neutral", "DKIM-Signature": "v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1722785438;\n\tbh=3mSO3AP3H99LQi8Rzp2yWPr+zC7RK2pWz55iyKvjmvI=;\n\th=From:To:Subject:Date:From;\n\tb=KNHuaRGUcYNPjZzQALlEfyikYTZrMs9OqioLrReMNbqzDCi2FYcquR1CBBNLBg9aQ\n\tgdRb6Lc29mOmrxux5s/oZF3pItiSQTTIgv3fAIIp5Gt9Q8p6dafzTuo5qx6NyJY/V0\n\tmDZY+M9g0TjbEOVtNd4IwjwnmkpOZVU5/jKIycvc=", "From": "Laurent Pinchart <laurent.pinchart@ideasonboard.com>", "To": "libcamera-devel@lists.libcamera.org", "Subject": "[PATCH v3 0/2] libcamera: Simplify error handling with\n\tScopeExitActions class", "Date": "Sun, 4 Aug 2024 18:31:04 +0300", "Message-ID": "<20240804153106.22167-1-laurent.pinchart@ideasonboard.com>", "X-Mailer": "git-send-email 2.44.2", "MIME-Version": "1.0", "Content-Transfer-Encoding": "8bit", "X-BeenThere": "libcamera-devel@lists.libcamera.org", "X-Mailman-Version": "2.1.29", "Precedence": "list", "List-Id": "<libcamera-devel.lists.libcamera.org>", "List-Unsubscribe": "<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>", "List-Archive": "<https://lists.libcamera.org/pipermail/libcamera-devel/>", "List-Post": "<mailto:libcamera-devel@lists.libcamera.org>", "List-Help": "<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>", "List-Subscribe": "<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>", "Errors-To": "libcamera-devel-bounces@lists.libcamera.org", "Sender": "\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>" }, "content": "Hello,\n\nHere's an attempt to resurect a patch series from nearly two years agoi.\n\nThis small patch series stems from my review of \"[PATCH 13/14]\nlibcamera: pipeline: rkisp1: Factorize errors handling path on start()\".\nWhile I agreed that the current error handling pattern in\nPipelineHandlerRkISP1::start() is fairly bad, I wasn't thrilled by usage\nof goto statements to fix it. This is an alternative proposal.\n\nThe ScopeExitActions class introduced in patch 1/2 took multiple forms\nbefore reaching this one. I first experimented with a class that handled\na single action and thus needed to be instantiated once per action. To\nachieve the equivalent of the ScopeExitActions::release() call I had to\neither add an 'if (ret)' statement to the lambda functions, or pass the\nret variable to the constructor of the ScopeExitActions. I found ideas\nfor syntactic sugar in https://github.com/yuri-kilochek/boost.scope_guard,\nbut in the end, neither option made me very happy.\n\nHandling multiple actions with one ScopeExitActions instance makes the\nuser code (in 2/2) simpler. The main drawback is the use of std::vector\nto store the actions, which requires dynamic allocation (and\nreallocation). I tried to find a different solution. One interesting\napproach used a ScopeExitGroup class instantiated at the beginning of\nthe function, and a ScopeExit class that handled a single action,\ninstantiated multiple times. The ScopeExit class was constructed with a\npointer to the ScopeExitGroup, and only called the action in its\ndestructor if the ScopeExitGroup hadn't been release()d. Unfortunately\nthis didn't work well for patch 2/2, as the\nPipelineHandlerRkISP1::start() function needs to register an action in a\nsub-scope (within a if (...) { ... }). The corresponding ScopeExit\ninstance thus got destroyed when exiting the sub-scope, not at the end\nof the function.\n\nAnother completely different approach to error handling would be to make\nall the cleanup functions in PipelineHandlerRkISP1 idempotent, safe to\nbe called when the corresponding operation hasn't been performed (e.g.\nstreamOff() without a previous streamOn()). This idiom may result to\nbetter error handling through libcamera. If anyone thinks it could be\nbetter than a ScopeExitActions class, that would be enough to convince\nme to give it a try and burry ScopeExitActions.\n\nIn v1 of the series the class had a defer() function (instead of an\noperator+=()), whose name was influenced by an interesting C extension\nproposal (https://hal.inria.fr/hal-03059076/document). It uses the name\n\"guard\" for the conceptual equivalent of the ScopeExitActions class, but\nthat didn't convince me.\n\nI still wish the release() call wouldn't be needed, but I'm not as\nunhappy with it than in v1. This is error-prone as it could be\nforgotten, but the other options I can think of are worse;\n\n- Switch to the opposite, with cleanups disabled by default and an\n explicit call in the error paths. That could also be forgotten,\n resulting in missing cleanups in some error paths. The result would\n likely be worse, as error paths are less tested.\n\n- Require explicit calls in both the success and error paths, and\n complain loudly in the destructor if none of those functions have been\n called. That's equally error-prone due to undertesting of error paths.\n\n- Tying the explicit calls with the return statements, for instance by\n making the user return a Result type (any rust developer here ?), and\n adding two functions to ScopeExitActions, success and error, that\n return a Result. While the Result idiom may be interesting, it would\n be a much broader change.\n\nLaurent Pinchart (2):\n libcamera: utils: Add ScopeExitActions class\n pipeline: rkisp1: Use ScopeExitActions to simplify error handling in\n start\n\n include/libcamera/base/utils.h | 13 +++\n src/libcamera/base/utils.cpp | 132 +++++++++++++++++++++++\n src/libcamera/pipeline/rkisp1/rkisp1.cpp | 31 ++----\n 3 files changed, 156 insertions(+), 20 deletions(-)\n\n\nbase-commit: 19bbca3c0b376ba0183f5db53472c8c46cd402b5" }