[{"id":25384,"web_url":"https://patchwork.libcamera.org/comment/25384/","msgid":"<166549987924.3760285.5982782385649249504@Monstersaurus>","date":"2022-10-11T14:51:19","subject":"Re: [libcamera-devel] [PATCH 0/2] libcamera: Simplify error\n\thandling with Cleaner class","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart via libcamera-devel (2022-10-04 03:18:40)\n> Hello,\n> \n> This small patch series stems from my review of \"[PATCH 13/14]\n> libcamera: pipeline: rkisp1: Factorize errors handling path on start()\".\n> While I agreed that the current error handling pattern in\n> PipelineHandlerRkISP1::start() is fairly bad, I wasn't thrilled by usage\n> of goto statements to fix it. This is an alternative proposal.\n> \n> The Cleaner class introduced in patch 1/2 took multiple forms before\n> reaching this one. I first experimented with a class that handled a\n> single action and thus needed to be instantiated once per action. To\n> achieve the equivalent of the Cleaner::clear() call I had to either add\n> an 'if (ret)' statement to the lambda functions, or pass the ret\n> variable to the constructor of the Cleaner. I found ideas for syntactic\n> sugar in https://github.com/yuri-kilochek/boost.scope_guard, but in the\n> end, neither option made me very happy.\n> \n> Handling multiple deferred cleanup actions with one Cleaner instance\n> makes the user code (in 2/2) simpler. The \"defer\" name for the\n> Cleaner::defer() function was influenced by an interesting C extension\n> proposal (https://hal.inria.fr/hal-03059076/document). It uses the name\n> \"guard\" for the conceptual equivalent of the Cleaner class, but that\n> didn't convince me. I'm not sure to really like the \"Cleaner\" name\n> though, alternative naming proposals are welcome.\n\nI'm not sure I like 'Cleaner' either. To me, this looks very much like\nthe scope_exit experimental C++ features:\n\n https://en.cppreference.com/w/cpp/experimental/scope_exit\n\nI like the ability to add multiple cleanups to a single instance though.\n\nhttps://github.com/SergiusTheBest/ScopeExit is another interesting\nimplementation, but bases the choice for a Success or Failed return\nusing exceptions, so that's not of any use to us.\n\nI (almost) like the syntatic sugar that means lambda's are hidden away:\n\n https://github.com/SergiusTheBest/ScopeExit/blob/master/include/ScopeExit/ScopeExit.h\n\n SCOPE_EXIT{ cout << \"hello\"; }; // will be called at the scope exit\n\nWhich could easily be ScopeExit { }; ... but that's creating a separate\ninstance for each declaration which wouldn't be easy to 'clean up' or\ncancel..., so I think that's not any better.\n\n\n\n> One point I'm not happy with is the need to call Cleaner::clear() in the\n> success path. This is error-prone as it could be forgotten. We could\n> switch to the opposite, with cleanups disabled by default and an\n> explicit call in the error paths, but that could also be forgotten,\n> resulting in missing cleanups in some error paths. The result would\n> possibly be worse, as error paths are less tested. Another option would\n> be to 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. I'm not sure to be convinced it would be the best idea, although\n\nExplicitly requiring either Cleaner.success() or Cleaner.fail() with an\nerror if one isn't called is interesting, but I am weary that then\nsomeone will end up simply calling the 'wrong one' on a code path ...\nBut maybe that's worrying about 'possible' bugs, rather than actual\nbugs.\n\n\n> finding good names for the success/error functions may help. Alternative\n> ideas are welcome too, as well as opinions regarding whether this is\n> worth it or not.\n\nIf there is automatic operations at exit, I'd call it AtScopeExit or\nScopeExit ...\nand use something like\n\n\n\t(At?)ScopeExit e;\n\te.defer([&]{ std::out << \"Failed\" << std::endl; });\n\n\tif (fail)\n\t\treturn -EINVAL;\n\t\n\te.cancel();\n\n\treturn 0;\n\nBut it's also just 'deferred work' so perhaps DeferredWorker would be a\nbetter class name than Cleaner if it is enforced to be called at both\nsuccess/failure paths.\n\n\nRemembering to cleanup before a successful exit is a pain, but I bet\nthat's easier to catch if it goes wrong than the alternative, as the\n'success' path is likely to be more often used... So I actually think\nthis would be 'safer'?\n\nAnyway, overall - I like the idea of a cleanup helper...\n\n--\nKieran\n\n\n> \n> Laurent Pinchart (2):\n> libcamera: utils: Add Cleaner class\n> pipeline: rkisp1: Use utils::Cleaner to simplify error handling in\n> start\n> \n> include/libcamera/base/utils.h | 13 +++\n> src/libcamera/base/utils.cpp | 130 +++++++++++++++++++++++\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 31 ++----\n> 3 files changed, 154 insertions(+), 20 deletions(-)\n> \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 C837FC0DA4\n\tfor ;\n\tTue, 11 Oct 2022 14:51:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 23CF5603F3;\n\tTue, 11 Oct 2022 16:51:24 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DCFE6603F3\n\tfor ;\n\tTue, 11 Oct 2022 16:51:22 +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 5961F907;\n\tTue, 11 Oct 2022 16:51:22 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1665499884;\n\tbh=47hV5ltD7PWxQM0attwZJ02Ax0xUSiBsxyMcp3/68T0=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=tiAt7Kis/AmYDjmfUkTCe0ZbGLYkrt6SVfkArBynl2b8/jwKsvgizX5K1YwT1HLe8\n\tu7O1OoofmqZ9xZeta4AfBO9pWnZ5Pt4CvoKWiuT3mlYs5iAjlYStX9lAhxe8xh1Jeu\n\ttidw/p1HrxqUOFa/m0CN4/tQ/iWqZZZyesYg8xmn6w35dyyUKhMzC0SpzaXuo2KSxS\n\tLQe+Ttp2MH9qr5fYtFqVzu4BNZD3DeXlVPM2mNFzN5Zo2QpR7jnIHTSvYAkEL6gyjd\n\ttFGelYImXBGyEACfgIlnTOYyqvY1qCUxMguvNH18/wkL7kcCtYZpNJivlfdPnvRvxW\n\tK8HQqpSTKIyLQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1665499882;\n\tbh=47hV5ltD7PWxQM0attwZJ02Ax0xUSiBsxyMcp3/68T0=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=qYiq09CTjBCw9VwRuAJ3Xu6HryrNycj0hJ8vnhCRlh2vTq85GdPgwR59bKaNP9pnl\n\tHq00glIdaAeymheJhCakeJrvzz7LhDxFZ6hCV13glM++F7ZLRJtdljVW5UxELwhDzk\n\tpQLptnAp45rZkIyTtmhUaEH1wkYHO8+cc/Vmv5Ys="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"qYiq09CT\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20221004021842.6345-1-laurent.pinchart@ideasonboard.com>","References":"<20221004021842.6345-1-laurent.pinchart@ideasonboard.com>","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 11 Oct 2022 15:51:19 +0100","Message-ID":"<166549987924.3760285.5982782385649249504@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH 0/2] libcamera: Simplify error\n\thandling with Cleaner class","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","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25387,"web_url":"https://patchwork.libcamera.org/comment/25387/","msgid":"","date":"2022-10-11T20:27:22","subject":"Re: [libcamera-devel] [PATCH 0/2] libcamera: Simplify error\n\thandling with Cleaner class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Tue, Oct 11, 2022 at 03:51:19PM +0100, Kieran Bingham wrote:\n> Quoting Laurent Pinchart via libcamera-devel (2022-10-04 03:18:40)\n> > Hello,\n> > \n> > This small patch series stems from my review of \"[PATCH 13/14]\n> > libcamera: pipeline: rkisp1: Factorize errors handling path on start()\".\n> > While I agreed that the current error handling pattern in\n> > PipelineHandlerRkISP1::start() is fairly bad, I wasn't thrilled by usage\n> > of goto statements to fix it. This is an alternative proposal.\n> > \n> > The Cleaner class introduced in patch 1/2 took multiple forms before\n> > reaching this one. I first experimented with a class that handled a\n> > single action and thus needed to be instantiated once per action. To\n> > achieve the equivalent of the Cleaner::clear() call I had to either add\n> > an 'if (ret)' statement to the lambda functions, or pass the ret\n> > variable to the constructor of the Cleaner. I found ideas for syntactic\n> > sugar in https://github.com/yuri-kilochek/boost.scope_guard, but in the\n> > end, neither option made me very happy.\n> > \n> > Handling multiple deferred cleanup actions with one Cleaner instance\n> > makes the user code (in 2/2) simpler. The \"defer\" name for the\n> > Cleaner::defer() function was influenced by an interesting C extension\n> > proposal (https://hal.inria.fr/hal-03059076/document). It uses the name\n> > \"guard\" for the conceptual equivalent of the Cleaner class, but that\n> > didn't convince me. I'm not sure to really like the \"Cleaner\" name\n> > though, alternative naming proposals are welcome.\n> \n> I'm not sure I like 'Cleaner' either. To me, this looks very much like\n> the scope_exit experimental C++ features:\n> \n> https://en.cppreference.com/w/cpp/experimental/scope_exit\n> \n> I like the ability to add multiple cleanups to a single instance though.\n\nMe too. I considered going for something more similar to scope_exit, but\nit would have required calling the .release() function on each scope\nguard individually at the end, which isn't very nice.\n\n> https://github.com/SergiusTheBest/ScopeExit is another interesting\n> implementation, but bases the choice for a Success or Failed return\n> using exceptions, so that's not of any use to us.\n\nSame as https://en.cppreference.com/w/cpp/experimental/scope_fail.\n\n> I (almost) like the syntatic sugar that means lambda's are hidden away:\n> \n> https://github.com/SergiusTheBest/ScopeExit/blob/master/include/ScopeExit/ScopeExit.h\n> \n> SCOPE_EXIT{ cout << \"hello\"; }; // will be called at the scope exit\n\nAlmost indeed.\n\n> Which could easily be ScopeExit { }; ... but that's creating a separate\n> instance for each declaration which wouldn't be easy to 'clean up' or\n> cancel..., so I think that's not any better.\n\nExactly :-)\n\n> > One point I'm not happy with is the need to call Cleaner::clear() in the\n> > success path. This is error-prone as it could be forgotten. We could\n> > switch to the opposite, with cleanups disabled by default and an\n> > explicit call in the error paths, but that could also be forgotten,\n> > resulting in missing cleanups in some error paths. The result would\n> > possibly be worse, as error paths are less tested. Another option would\n> > be to 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. I'm not sure to be convinced it would be the best idea, although\n> \n> Explicitly requiring either Cleaner.success() or Cleaner.fail() with an\n> error if one isn't called is interesting, but I am weary that then\n> someone will end up simply calling the 'wrong one' on a code path ...\n> But maybe that's worrying about 'possible' bugs, rather than actual\n> bugs.\n\nThat's a possible issue too.\n\n> > finding good names for the success/error functions may help. Alternative\n> > ideas are welcome too, as well as opinions regarding whether this is\n> > worth it or not.\n> \n> If there is automatic operations at exit, I'd call it AtScopeExit or\n> ScopeExit ...\n\nThe name sounds good.\n\n> and use something like\n> \n> \n> \t(At?)ScopeExit e;\n> \te.defer([&]{ std::out << \"Failed\" << std::endl; });\n\nI could also add a constructor that takes a functor, so an instance that\nwould have a single action wouldn't require a separate call to defer().\n\n> \n> \tif (fail)\n> \t\treturn -EINVAL;\n> \t\n> \te.cancel();\n> \n> \treturn 0;\n> \n> But it's also just 'deferred work' so perhaps DeferredWorker would be a\n> better class name than Cleaner if it is enforced to be called at both\n> success/failure paths.\n> \n> \n> Remembering to cleanup before a successful exit is a pain, but I bet\n> that's easier to catch if it goes wrong than the alternative, as the\n> 'success' path is likely to be more often used... So I actually think\n> this would be 'safer'?\n\nI think so too.\n\n> Anyway, overall - I like the idea of a cleanup helper...\n> \n> > Laurent Pinchart (2):\n> > libcamera: utils: Add Cleaner class\n> > pipeline: rkisp1: Use utils::Cleaner to simplify error handling in\n> > start\n> > \n> > include/libcamera/base/utils.h | 13 +++\n> > src/libcamera/base/utils.cpp | 130 +++++++++++++++++++++++\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 31 ++----\n> > 3 files changed, 154 insertions(+), 20 deletions(-)","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 B2BC7BD16B\n\tfor ;\n\tTue, 11 Oct 2022 20:27:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CA96062D7F;\n\tTue, 11 Oct 2022 22:27:31 +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 C2114603F3\n\tfor ;\n\tTue, 11 Oct 2022 22:27:29 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 083C2907;\n\tTue, 11 Oct 2022 22:27:28 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1665520051;\n\tbh=p7NLCzGlqffZfc1tk4qiQc7dG2SxveVaGY+GMuKIeFA=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=ElnCwr+1zbo+KjtHeX7thghqam5t3w70OrNoF+Zw8v38QToCa35B/BnVc9bQZEpH2\n\tTQOIvoGnMHULO9jZKzLcpuY4QL5MiHEqQO/PhXadiVuxJ4bMH5IpdxoS4Ywa7gVo1I\n\tETYJFSqWh3egepkRxEwtLIL15l355IWq2pVLNYZKnk4xpaB50Y3R7Z8f9QSGHDSkHw\n\tzr+oA89shE0DWXdGnFstAv4dQoo3RZgnqKDa1IWdmsGeL21uIJ15LIv5KK8gdB14Pr\n\tGs4YKeKkAEY4AYZ11A7dJ+M9FajeG4/uW2BfaYBd68lZ+sPJaO65ByNF/91jHdipBr\n\tn2h1UIB2h1xIg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1665520049;\n\tbh=p7NLCzGlqffZfc1tk4qiQc7dG2SxveVaGY+GMuKIeFA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ZaNLM1T04celtRoL/37HgSg+9tleure/mmvbVaPMTUKPbxnV+2+PsUasK141UBtuv\n\t0hf8y7TBrLhurPEWlwUP6/uJ75ro2XPiKxwLS+h5tfAeVncybrnqjoHWhVB4jNpDKM\n\taRRYmZDwOuHLKkAjrQN4/+8339RVOf+h71v3U2s4="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"ZaNLM1T0\"; dkim-atps=neutral","Date":"Tue, 11 Oct 2022 23:27:22 +0300","To":"Kieran Bingham ","Message-ID":"","References":"<20221004021842.6345-1-laurent.pinchart@ideasonboard.com>\n\t<166549987924.3760285.5982782385649249504@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<166549987924.3760285.5982782385649249504@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH 0/2] libcamera: Simplify error\n\thandling with Cleaner class","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]