[{"id":25255,"web_url":"https://patchwork.libcamera.org/comment/25255/","msgid":"<703a6a3c-78e4-108d-ad85-740968233930@oss.nxp.com>","date":"2022-10-04T11:35:42","subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: utils: Add Cleaner\n\tclass","submitter":{"id":107,"url":"https://patchwork.libcamera.org/api/people/107/","name":"Xavier Roumegue","email":"xavier.roumegue@oss.nxp.com"},"content":"Hi Laurent,\n\n\nOn 10/4/22 04:18, Laurent Pinchart wrote:\n> The Cleaner class is a simple object that performs user-provided actions\n> upon destruction. As its name implies, it is meant to simplify cleanup\n> tasks in error handling paths.\n> \n> Signed-off-by: Laurent Pinchart \n> ---\n> include/libcamera/base/utils.h | 13 ++++\n> src/libcamera/base/utils.cpp | 130 +++++++++++++++++++++++++++++++++\n> 2 files changed, 143 insertions(+)\n> \n> diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h\n> index ed88b7163770..0de80eb89d15 100644\n> --- a/include/libcamera/base/utils.h\n> +++ b/include/libcamera/base/utils.h\n> @@ -9,6 +9,7 @@\n> \n> #include \n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -381,6 +382,18 @@ struct defopt_t {\n> \n> constexpr details::defopt_t defopt;\n> \n> +class Cleaner\n> +{\n> +public:\n> +\t~Cleaner();\n> +\n> +\tvoid defer(std::function &&action);\n> +\tvoid clear();\n> +\n> +private:\n> +\tstd::vector> actions_;\n> +};\n> +\n> } /* namespace utils */\n> \n> #ifndef __DOXYGEN__\n> diff --git a/src/libcamera/base/utils.cpp b/src/libcamera/base/utils.cpp\n> index 9cd6cb197243..45e115b7031e 100644\n> --- a/src/libcamera/base/utils.cpp\n> +++ b/src/libcamera/base/utils.cpp\n> @@ -484,6 +484,136 @@ std::string toAscii(const std::string &str)\n> * default-constructed T.\n> */\n> \n> +/**\n> + * \\class Cleaner\n> + * \\brief An object that performs actions upon destruction\n> + *\n> + * The Cleaner class is a simple object that performs user-provided actions\n> + * upon destruction. As its name implies, it is meant to simplify cleanup tasks\n> + * in error handling paths.\n> + *\n> + * When the code flow performs multiple sequential actions that each need a\n> + * corresponding cleanup action, error handling quickly become tedious:\n> + *\n> + * \\code{.cpp}\n> + * {\n> + * \tint ret = allocateMemory();\n> + * \tif (ret)\n> + * \t\treturn ret;\n> + *\n> + * \tret = startProducder();\ntypo\n> + * \tif (ret) {\n> + * \t\tfreeMemory();\n> + * \t\treturn ret;\n> + * \t}\n> + *\n> + * \tret = startConsumer();\n> + * \tif (ret) {\n> + * \t\tstopProducer();\n> + * \t\tfreeMemory();\n> + * \t\treturn ret;\n> + * \t}\n> + *\n> + * \treturn 0;\n> + * }\n> + * \\endcode\n> + *\n> + * This is prone to programming mistakes, as cleanup actions can easily be\n> + * forgotten or ordered incorrectly. One strategy to simplify error handling is\n> + * to use goto statements:\n> + *\n> + * \\code{.cpp}\n> + * {\n> + * \tint ret = allocateMemory();\n> + * \tif (ret)\n> + * \t\treturn ret;\n> + *\n> + * \tret = startProducder();\ntypo\n> + * \tif (ret)\n> + * \t\tgoto error_free;\n> + *\n> + * \tret = startConsumer();\n> + * \tif (ret)\n> + * \t\tgoto error_stop;\n> + *\n> + * \treturn 0;\n> + *\n> + * error_stop:\n> + * \tstopProducer();\n> + * error_free:\n> + * \tfreeMemory();\n> + * \treturn ret;\n> + * }\n> + * \\endcode\n> + *\n> + * While this may be considered better, this solution is still quite\n> + * error-prone. Beside the risk of picking the wrong error label, the error\n> + * handling logic is separated from the normal code flow, which increases the\n> + * risk of error when refactoring the code. Additionally, C++ doesn't allow\n> + * goto statements to jump over local variable declarations, which can make\n> + * usage of this pattern more difficult.\n> + *\n> + * The Cleaner class solves these issues by allowing code that requires cleanup\n> + * actions to be grouped with its corresponding deferred error handling code:\n> + *\n> + * \\code{.cpp}\n> + * {\n> + * \tCleaner cleaner;\n> + *\n> + * \tint ret = allocateMemory();\n> + * \tif (ret)\n> + * \t\treturn ret;\n> + *\n> + * \tcleaner.defer([&]() { freeMemory(); });\n> + *\n> + * \tret = startProducder();\ntypo\n> + * \tif (ret)\n> + * \t\treturn ret;\n> + *\n> + * \tcleaner.defer([&]() { stopProducer(); });\n> + *\n> + * \tret = startConsumer();\n> + * \tif (ret)\n> + * \t\treturn ret;\n> + *\n> + * \tcleaner.clear();\n> + * \treturn 0;\n> + * }\n> + * \\endcode\n> + *\n> + * Deferred error handlers are executed when the Cleaner instance is destroyed,\n> + * in the reverse order of their addition.\n> + */\n> +\n> +Cleaner::~Cleaner()\n> +{\n> +\tfor (const auto &action : utils::reverse(actions_))\n> +\t\taction();\n> +}\n> +\n> +/**\n> + * \\brief Add a deferred cleanup action\n> + * \\param[in] action The cleanup action\n> + *\n> + * Add a deferred cleanup action to the Cleaner. Cleanup actions will be called\n> + * upon destruction in the reverse order of their addition.\n> + */\n> +void Cleaner::defer(std::function &&action)\n> +{\n> +\tactions_.push_back(std::move(action));\n> +}\n> +\n> +/**\n> + * \\brief Clear all deferred cleanup actions\n> + *\n> + * This function should be called in the success path to clear all deferred\n> + * error cleanup actions.\n> + */\n> +void Cleaner::clear()\n> +{\n> +\tactions_.clear();\n> +}\n> +\n> } /* namespace utils */\n> \n> #ifndef __DOXYGEN__With those typos fixed:\n\nReviewed-by: Xavier Roumegue ","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 07DECC0DA4\n\tfor ;\n\tTue, 4 Oct 2022 11:35:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2E56760AA1;\n\tTue, 4 Oct 2022 13:35:49 +0200 (CEST)","from EUR01-VE1-obe.outbound.protection.outlook.com\n\t(mail-eopbgr140041.outbound.protection.outlook.com [40.107.14.41])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1BF63601C7\n\tfor ;\n\tTue, 4 Oct 2022 13:35:47 +0200 (CEST)","from PAXPR04MB8703.eurprd04.prod.outlook.com\n\t(2603:10a6:102:21e::22)\n\tby PA4PR04MB9342.eurprd04.prod.outlook.com (2603:10a6:102:2a6::21)\n\twith Microsoft SMTP Server (version=TLS1_2,\n\tcipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.5676.31;\n\tTue, 4 Oct 2022 11:35:44 +0000","from PAXPR04MB8703.eurprd04.prod.outlook.com\n\t([fe80::4f72:a35a:8c60:63f1]) by\n\tPAXPR04MB8703.eurprd04.prod.outlook.com\n\t([fe80::4f72:a35a:8c60:63f1%6]) with mapi id 15.20.5676.028;\n\tTue, 4 Oct 2022 11:35:44 +0000"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1664883349;\n\tbh=hmQKdbXUNLLKfF95Y7Dp6szpfJ2TJHXjiyLOc93NtI8=;\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:\n\tFrom;\n\tb=rzYqIvSXr+Wqx0nI+LA36qJJLPrQjoa3pLceIIjlirJwuSdNPTDfDLVXtCkozF8ai\n\tgtftl8dcef7rOlz+nEJWKPuyNz0u3hKft3K7m2qi6oJSZIRo8KX9Px9E9R/VpuVKee\n\tTnhmwuQTrwJboC3SoXEWi5AooRPnzkx958cdJYo3G5RrhX6cNYJNHcaOPx257rSwZ7\n\t/3RgUA619qFYIlSazlVQQLDNezj1RjkTiMEgtAyCjEnkfuhmh1bQ320PvHQEAw7L9W\n\tGJI2eBqbnzkgeM3tw/ivvOMSmcNl/vuHEjDoOWm5UOaeyVwiAHwFW5kY2/CXcyCJvD\n\t5YigOxOMWU+Tg==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=NXP1.onmicrosoft.com;\n\ts=selector2-NXP1-onmicrosoft-com;\n\th=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck;\n\tbh=M5PGw4OSXWGKPIKq14MWcdqFc8lHVHFntt+7zG51hEE=;\n\tb=VVDPKkXOF3XLP/GUIgod3OWTBu8KBK1LVTvZ88piNBfcwnZGE46Ap5CgAY9rOpPTUOk4qyifHDIjqbNK1P3UVbeH11Ij0NYok+5zsEeKEkvYO95n1d7UNKroovKtQ1zOrTGbWoglzFimh/dCBxBnNNsgSGptQQmLsdWwIQTjbr8="],"Authentication-Results":["lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=NXP1.onmicrosoft.com\n\theader.i=@NXP1.onmicrosoft.com\n\theader.b=\"VVDPKkXO\"; dkim-atps=neutral","dkim=none (message not signed)\n\theader.d=none;dmarc=none action=none header.from=oss.nxp.com;"],"ARC-Seal":"i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none;\n\tb=hqpW74M2ud45nCv6998AtxHLtCWnEJXJDIxtgNTc+rZpdKWhy0TFy7LLbFrKihGPlZqKf5sWr/nWvTnPzzuw1XFaUsWaQRzTz2sILFqI3XmykaZy7wu4f3p+HQTUojmI/HpxJqutuezaAiVREStjp9n0DCUfSHKUtoW57F2fmVU7V20LjileibH+h+S7vEDmVQDnLcFXzBp5v9BU4br2VHvyZauuKqjqrMQgbWSWYarnUph5Sdvq0bLUn/BsHtLpoBlKcZbZc8POEXbHX4yPt6HMDCk5NU/twEoIlm66gTOK3GHjujivODCbu2KHZDeDi5xgfMyZ2eLxGMUrLmzRMA==","ARC-Message-Signature":"i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com;\n\ts=arcselector9901;\n\th=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1;\n\tbh=M5PGw4OSXWGKPIKq14MWcdqFc8lHVHFntt+7zG51hEE=;\n\tb=iSjNKGV5a3kedsG964WdhFcO3OwLjhTkrbbiM1EEqxrWpqGdmVfToHXv+EssxnmKxkCmSb/9D9/CQ2u2jAbIAtUh9O1mxkwQm7PeaD5LrNnYNipb7y+UaWEslFDyG3bdRho4X0/b8vBOc8R8k/12+t48KirMIbYRSHJLOxykNuS7xrQXP5QFwZ4r7e23aUDyNgm9l+GVw0ehL6oUJbIFbP2YkHs3dEM5I5QYg6glR1qa27MFbFJp7bXhTms5RdS9Cz32XUilTXubQcEPxEayRdPFUrgdJurEwFBz1EpGEYPQ0xZkO8ntHUj6p9wI34Jj7s0OmEF5S/XlXdM25ACOtQ==","ARC-Authentication-Results":"i=1; mx.microsoft.com 1; spf=pass\n\tsmtp.mailfrom=oss.nxp.com;\n\tdmarc=pass action=none header.from=oss.nxp.com; \n\tdkim=pass header.d=oss.nxp.com; arc=none","Message-ID":"<703a6a3c-78e4-108d-ad85-740968233930@oss.nxp.com>","Date":"Tue, 4 Oct 2022 13:35:42 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101\n\tThunderbird/102.3.1","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20221004021842.6345-1-laurent.pinchart@ideasonboard.com>\n\t<20221004021842.6345-2-laurent.pinchart@ideasonboard.com>","Content-Language":"en-US, fr","In-Reply-To":"<20221004021842.6345-2-laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","X-ClientProxiedBy":"PAZP264CA0036.FRAP264.PROD.OUTLOOK.COM\n\t(2603:10a6:102:122::23) To PAXPR04MB8703.eurprd04.prod.outlook.com\n\t(2603:10a6:102:21e::22)","MIME-Version":"1.0","X-MS-Exchange-MessageSentRepresentingType":"1","X-MS-PublicTrafficType":"Email","X-MS-TrafficTypeDiagnostic":"PAXPR04MB8703:EE_|PA4PR04MB9342:EE_","X-MS-Office365-Filtering-Correlation-Id":"9288b59d-d3ed-43ea-6312-08daa5fc9286","X-MS-Exchange-SharedMailbox-RoutingAgent-Processed":"True","X-MS-Exchange-SenderADCheck":"1","X-MS-Exchange-AntiSpam-Relay":"0","X-Microsoft-Antispam":"BCL:0;","X-Microsoft-Antispam-Message-Info":"6NAR3ZeC2J2LtQgjbvYG9MHQeFpzxT5ouxDohzC5nwzETaf4UsmLh0onZQhNX3CAYfpzLCDjevYMKzAwaz2oQVCsIQ4tlpYKdr3Kmgqjb7eJOx0KPASQ2lh0WqdfgCcniguueA+iHWOOfD9QOgn7RBkEXSIAKgwatWdRU/HH7PXOlO8iZSXn90xFbJebUjsxyZF97C3/C+0yuYMSPMbPXncg3+jAxINjULRFY5UlYFhorA7SF4NP5Kp83vjsYEUeLElKgAfs/s4mW3segQT5eXwq2vh9Y4WvfNt7UyTll9mz0awy9hpcHoxmvz8OxYXF1gmiCStI/vmXAN7iHsNEi5Gt4rpyMTxOe0f0cLD+P9D9R07CsR/HGXEnCO8TbtO8QLdB8UorPBNo415ia8npLAKxMJYvHUbdKSR1vC/2bSfn+526qYUy7PIRCqGJDx7JRCOu1dP/ki5Kabc65YHW37MZJ5jLogTJeNzGjf2z4ckHvwmhQo59hIMUyz+9OF7L+mKRn+yJk8gCdsFmRfcnb+Qqc8+EUMNRcitrFfTVle8tSaj6JiUkFzmpxh6NvSABRkCyi8Am4orxydClaJytsxJUcTdDW05JwkvM74NoGNQyEqLgEGDE4JOrIR1//P9ioooUvU6WlhfhOxx3E77D+bl8mIt4kQbDvsGhGkne5v+gw7923e/kCJb5aAX/6RmANfJRq2O5G+Iyiii+xt0u4uwKvTVaTo4y+K2tpdgNklYixVhMSM72GIAuLgP8W/OFVS0WUtvPyafMbJMDrmOPJuEFCrnxOoQ/tdv9nWYLN8Q=","X-Forefront-Antispam-Report":"CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:;\n\tIPV:NLI; SFV:NSPM; H:PAXPR04MB8703.eurprd04.prod.outlook.com; PTR:;\n\tCAT:NONE; \n\tSFS:(13230022)(4636009)(136003)(376002)(39860400002)(396003)(346002)(366004)(451199015)(66556008)(66946007)(8676002)(66476007)(53546011)(6512007)(52116002)(6506007)(2616005)(186003)(316002)(2906002)(5660300002)(8936002)(38100700002)(41300700001)(31696002)(86362001)(83380400001)(6486002)(31686004)(478600001)(45980500001)(43740500002);\n\tDIR:OUT; SFP:1101; ","X-MS-Exchange-AntiSpam-MessageData-ChunkCount":"1","X-MS-Exchange-AntiSpam-MessageData-0":"=?utf-8?q?0DdlAwMh7WEfsubZ3/ltRzzif?=\n\t=?utf-8?q?9PTCsoBJdgAFRYRwtLjzX/6xfLI08y6dBPQz++NQcIk4VTuKeKSqHbAb?=\n\t=?utf-8?q?9Uwzy3Y8dgleFrxXLNtzh9JFIwkIDDc3ZxFjqU820NwN49Kwg2OEfy57?=\n\t=?utf-8?q?wLnqwi9MnEKpSzuYVtDexwPdFT4H8YXMMkhhAzMSVZwpOxSfx/71cWCj?=\n\t=?utf-8?q?CsLfIz4ieSj7/pC/OEXmPeFZodJAYLrJlfRXtJy627zsXTgoRlUvLE6W?=\n\t=?utf-8?q?a8wJ0FOEeT6xq2OBWbHBf231fEZjJKcSvOUVAPJbrSjTXwKmye2fQHig?=\n\t=?utf-8?q?Ukqdp+hseWAIS5oN5u4mru6fNXd4pBptlQECo0ycHNBsJWogC/Ijm3UO?=\n\t=?utf-8?q?C6LUvxeLDoYIgc+wdLHc8ZMNvofAXgtd158bXptnR/p5eDinMx0s+MAe?=\n\t=?utf-8?q?U3KBIW7EfvlCbDcL7Fn0BeL95fxfO7w52G3ePHMdmh6Dy2w06O2FNFX1?=\n\t=?utf-8?q?uevjhUHCoBZoamMdMt+YhMiOcs9ahHI87EpAoQsUroA2Fhe23+SYX8yE?=\n\t=?utf-8?q?lr/1h3JqwLFCXtojKWzW6ygQijFVDsBKtcYNwh3o7c+Nq6JqwsDSNlUa?=\n\t=?utf-8?q?MdsrM4Y7QAeB1Ai/YJiHKJCFJmmBl5enaSApx8RmE9Jc5QA3bq/2Ju7q?=\n\t=?utf-8?q?YZakQM77iz7NjT4J0aG+78S427qAALy/rzvHZR92b30T8+sI7A/OQaiA?=\n\t=?utf-8?q?Cz9tSH3JA6nfIbuxrLzXFqQgXmHVnOC7U0ZF9aprDI31dNSJkZitYZJf?=\n\t=?utf-8?q?e5LOKZfONB9uZF/6Wg1tTtljJ+t7tnYgWHO/xlCixyMNdTdl2Zn7Z/VE?=\n\t=?utf-8?q?Eai/O/z+Fgba5Ek5pObELELF5JJPiq0bI9E+Ri2zI7Jq0U6L/wqic4Cb?=\n\t=?utf-8?q?i4MpECM6qX/KvA+Hjtbcjgpb5mRYEZWlZ400UnDU29SBKDW/KdpMDYPI?=\n\t=?utf-8?q?WpFfzbbLO9lMfZbU92l8SZikwSPoqaSbDC6TUzSst6WvT3XS6+SL+uOw?=\n\t=?utf-8?q?U2A09lphjPa7TD9JtDBKf+0cIXyqxyUUJ/321UAVRgBLaX4NfgNjumAB?=\n\t=?utf-8?q?8HejKLCbNgNmUUSZWhUUOwWFDlH2fTyXijlOQx+e3E7EhCIvpwaclWv2?=\n\t=?utf-8?q?9daFtmE9WKjExyRjKBMbJEXqn6x2KKHZTnb+hcxaOTsCL00OgqrYefSu?=\n\t=?utf-8?q?toP3LxztGvVYRm/UEE2UOT5F9Z/SAaxr7vDK2fKyoJlRw4Q8YIebn+TD?=\n\t=?utf-8?q?UnVeuOZLVkIsXwe++BQ3ko9asBFqhnKh7VTaW2NIpsPinv2/8bwVQKN9?=\n\t=?utf-8?q?Gs/CykBR/KdIUxM+HkzXX57ulegYYaxYYklVifUscWvWL79pmpj46uVe?=\n\t=?utf-8?q?DGpyFmB1FOowFaOgyZWEqJko/mHkJrbBP98NBVQFlSf5okrydsIlZ0r/?=\n\t=?utf-8?q?qK3SbDhVyz/BErvhRjKT2pPyiBXVqJ9SSRpAqQwXyTwk4jqPf2eK7KPC?=\n\t=?utf-8?q?CImTlTnA6k8R/C0P5seu0DxE7vanBHy4srVnXUlTSw8fl1i87oJ06Qpm?=\n\t=?utf-8?q?ePrNyq8FPVu2PxWy8tXcGWi49Dk5Z5VRwrGaMw2rkinmVGLzkcJj+CJ4?=\n\t=?utf-8?q?onE6L6RUkBvkAL573JqbpiY9vU+Y4SXAWy34x/asMwMEVkClQPR49esU?=\n\t=?utf-8?q?VocDy6BP0Tdrs81UaOV7Uv5tGMt8tMhoNfltnBZMphFIQ5KkHdu0dfON?=\n\t=?utf-8?q?Iw2thU5S9gYueoe1KS/k74W6FlEUCuIX9LJSA=3D=3D?=","X-OriginatorOrg":"oss.nxp.com","X-MS-Exchange-CrossTenant-Network-Message-Id":"9288b59d-d3ed-43ea-6312-08daa5fc9286","X-MS-Exchange-CrossTenant-AuthSource":"PAXPR04MB8703.eurprd04.prod.outlook.com","X-MS-Exchange-CrossTenant-AuthAs":"Internal","X-MS-Exchange-CrossTenant-OriginalArrivalTime":"04 Oct 2022 11:35:44.1686\n\t(UTC)","X-MS-Exchange-CrossTenant-FromEntityHeader":"Hosted","X-MS-Exchange-CrossTenant-Id":"686ea1d3-bc2b-4c6f-a92c-d99c5c301635","X-MS-Exchange-CrossTenant-MailboxType":"HOSTED","X-MS-Exchange-CrossTenant-UserPrincipalName":"WDu9EfzCkHlFRzFH6O9+iuQucUaqo7/QjW5WHE/O/PIHFLEDNk2+eWvVjJS1SazCpNeZ/gaex4s5DuQwTjA3gw==","X-MS-Exchange-Transport-CrossTenantHeadersStamped":"PA4PR04MB9342","Subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: utils: Add Cleaner\n\tclass","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":"\"Xavier Roumegue \\(OSS\\) via libcamera-devel\"\n\t","Reply-To":"\"Xavier Roumegue \\(OSS\\)\" ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25348,"web_url":"https://patchwork.libcamera.org/comment/25348/","msgid":"<20221007150157.zgq3csc4nafkflyh@uno.localdomain>","date":"2022-10-07T15:01:57","subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: utils: Add Cleaner\n\tclass","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent\n\nOn Tue, Oct 04, 2022 at 05:18:41AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> The Cleaner class is a simple object that performs user-provided actions\n> upon destruction. As its name implies, it is meant to simplify cleanup\n> tasks in error handling paths.\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> include/libcamera/base/utils.h | 13 ++++\n> src/libcamera/base/utils.cpp | 130 +++++++++++++++++++++++++++++++++\n> 2 files changed, 143 insertions(+)\n>\n> diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h\n> index ed88b7163770..0de80eb89d15 100644\n> --- a/include/libcamera/base/utils.h\n> +++ b/include/libcamera/base/utils.h\n> @@ -9,6 +9,7 @@\n>\n> #include \n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -381,6 +382,18 @@ struct defopt_t {\n>\n> constexpr details::defopt_t defopt;\n>\n> +class Cleaner\n> +{\n> +public:\n> +\t~Cleaner();\n> +\n> +\tvoid defer(std::function &&action);\n> +\tvoid clear();\n> +\n> +private:\n> +\tstd::vector> actions_;\n> +};\n> +\n> } /* namespace utils */\n>\n> #ifndef __DOXYGEN__\n> diff --git a/src/libcamera/base/utils.cpp b/src/libcamera/base/utils.cpp\n> index 9cd6cb197243..45e115b7031e 100644\n> --- a/src/libcamera/base/utils.cpp\n> +++ b/src/libcamera/base/utils.cpp\n> @@ -484,6 +484,136 @@ std::string toAscii(const std::string &str)\n> * default-constructed T.\n> */\n>\n> +/**\n> + * \\class Cleaner\n> + * \\brief An object that performs actions upon destruction\n> + *\n> + * The Cleaner class is a simple object that performs user-provided actions\n> + * upon destruction. As its name implies, it is meant to simplify cleanup tasks\n> + * in error handling paths.\n> + *\n> + * When the code flow performs multiple sequential actions that each need a\n> + * corresponding cleanup action, error handling quickly become tedious:\n> + *\n> + * \\code{.cpp}\n> + * {\n> + * \tint ret = allocateMemory();\n> + * \tif (ret)\n> + * \t\treturn ret;\n> + *\n> + * \tret = startProducder();\n> + * \tif (ret) {\n> + * \t\tfreeMemory();\n> + * \t\treturn ret;\n> + * \t}\n> + *\n> + * \tret = startConsumer();\n> + * \tif (ret) {\n> + * \t\tstopProducer();\n> + * \t\tfreeMemory();\n> + * \t\treturn ret;\n> + * \t}\n> + *\n> + * \treturn 0;\n> + * }\n> + * \\endcode\n> + *\n> + * This is prone to programming mistakes, as cleanup actions can easily be\n> + * forgotten or ordered incorrectly. One strategy to simplify error handling is\n> + * to use goto statements:\n> + *\n> + * \\code{.cpp}\n> + * {\n> + * \tint ret = allocateMemory();\n> + * \tif (ret)\n> + * \t\treturn ret;\n> + *\n> + * \tret = startProducder();\n> + * \tif (ret)\n> + * \t\tgoto error_free;\n> + *\n> + * \tret = startConsumer();\n> + * \tif (ret)\n> + * \t\tgoto error_stop;\n> + *\n> + * \treturn 0;\n> + *\n> + * error_stop:\n> + * \tstopProducer();\n> + * error_free:\n> + * \tfreeMemory();\n> + * \treturn ret;\n> + * }\n> + * \\endcode\n> + *\n> + * While this may be considered better, this solution is still quite\n> + * error-prone. Beside the risk of picking the wrong error label, the error\n> + * handling logic is separated from the normal code flow, which increases the\n> + * risk of error when refactoring the code. Additionally, C++ doesn't allow\n> + * goto statements to jump over local variable declarations, which can make\n> + * usage of this pattern more difficult.\n> + *\n> + * The Cleaner class solves these issues by allowing code that requires cleanup\n> + * actions to be grouped with its corresponding deferred error handling code:\n> + *\n> + * \\code{.cpp}\n> + * {\n> + * \tCleaner cleaner;\n> + *\n> + * \tint ret = allocateMemory();\n> + * \tif (ret)\n> + * \t\treturn ret;\n> + *\n> + * \tcleaner.defer([&]() { freeMemory(); });\n> + *\n> + * \tret = startProducder();\n> + * \tif (ret)\n> + * \t\treturn ret;\n> + *\n> + * \tcleaner.defer([&]() { stopProducer(); });\n> + *\n> + * \tret = startConsumer();\n> + * \tif (ret)\n> + * \t\treturn ret;\n> + *\n> + * \tcleaner.clear();\n> + * \treturn 0;\n> + * }\n> + * \\endcode\n\nI just wonder if\n\n{\n\tCleaner cleaner;\n\n\tint ret = allocateMemory();\n\tif (ret)\n\t\treturn ret;\n\n\tcleaner.defer([&]() { freeMemory(); });\n\n\tret = startProducder();\n\tif (ret) {\n cleaner.clean();\n\t\treturn ret;\n }\n\n\tcleaner.defer([&]() { stopProducer(); });\n\n\tret = startConsumer();\n\tif (ret) {\n cleaner.clean();\n\t\treturn ret;\n }\n\n\treturn 0;\n}\n\n\nIOW make Claner::clean() execute the cleaning actions while\ndestruction is a no-op.\n\nIt just feel more natural to have a clean() call on error paths and\nnot having to call clean() on a succesfull return.\n\n> + *\n> + * Deferred error handlers are executed when the Cleaner instance is destroyed,\n> + * in the reverse order of their addition.\n> + */\n> +\n> +Cleaner::~Cleaner()\n> +{\n> +\tfor (const auto &action : utils::reverse(actions_))\n> +\t\taction();\n> +}\n> +\n> +/**\n> + * \\brief Add a deferred cleanup action\n> + * \\param[in] action The cleanup action\n> + *\n> + * Add a deferred cleanup action to the Cleaner. Cleanup actions will be called\n> + * upon destruction in the reverse order of their addition.\n> + */\n> +void Cleaner::defer(std::function &&action)\n> +{\n> +\tactions_.push_back(std::move(action));\n> +}\n> +\n> +/**\n> + * \\brief Clear all deferred cleanup actions\n> + *\n> + * This function should be called in the success path to clear all deferred\n> + * error cleanup actions.\n> + */\n> +void Cleaner::clear()\n> +{\n> +\tactions_.clear();\n> +}\n> +\n> } /* namespace utils */\n>\n> #ifndef __DOXYGEN__\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 2F174C0DA4\n\tfor ;\n\tFri, 7 Oct 2022 15:02:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7639562D27;\n\tFri, 7 Oct 2022 17:02:00 +0200 (CEST)","from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id ACF8D60A88\n\tfor ;\n\tFri, 7 Oct 2022 17:01:59 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 2CD08FF806;\n\tFri, 7 Oct 2022 15:01:58 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1665154920;\n\tbh=cYo53a9XdBBkFvIObMudGyN+Ci5kM249vew1nf+TU6I=;\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=RisHEMdo7uxfC1HVHgClOJSMa/AkAhiux40myK5bgtOixpz4xbxqfNnvj0Qpu4pBn\n\tNA/r3pEejvCO2u6qQy6oj2CyB4xv/XmIG7LRjpDf4BJwYgt+tBU7Mp5sHfNQbp9kLu\n\tebOBmhzOYdyFP/jVmBrrqt5U6NjqoL+pbluYXWFvEafxEw1jgfvxNQT9DYfHHKOPZ5\n\tmGx6WqagHftNLO3atqNo4T94QrYq7hb4Rr2lpTKDNYMK5OM3Tu9KSlnFF+EAgLGl8u\n\t2AB0GddNwU0P7zSBvqYEGZIeNBF23M5o+RLpLi32gGHkCce/OtP/3mvWRcqpe1HbFP\n\tYbMsTTv32880Q==","Date":"Fri, 7 Oct 2022 17:01:57 +0200","To":"Laurent Pinchart ","Message-ID":"<20221007150157.zgq3csc4nafkflyh@uno.localdomain>","References":"<20221004021842.6345-1-laurent.pinchart@ideasonboard.com>\n\t<20221004021842.6345-2-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20221004021842.6345-2-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: utils: Add Cleaner\n\tclass","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":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25349,"web_url":"https://patchwork.libcamera.org/comment/25349/","msgid":"","date":"2022-10-07T15:26:00","subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: utils: Add Cleaner\n\tclass","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Fri, Oct 07, 2022 at 05:01:57PM +0200, Jacopo Mondi wrote:\n> On Tue, Oct 04, 2022 at 05:18:41AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> > The Cleaner class is a simple object that performs user-provided actions\n> > upon destruction. As its name implies, it is meant to simplify cleanup\n> > tasks in error handling paths.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > include/libcamera/base/utils.h | 13 ++++\n> > src/libcamera/base/utils.cpp | 130 +++++++++++++++++++++++++++++++++\n> > 2 files changed, 143 insertions(+)\n> >\n> > diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h\n> > index ed88b7163770..0de80eb89d15 100644\n> > --- a/include/libcamera/base/utils.h\n> > +++ b/include/libcamera/base/utils.h\n> > @@ -9,6 +9,7 @@\n> >\n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -381,6 +382,18 @@ struct defopt_t {\n> >\n> > constexpr details::defopt_t defopt;\n> >\n> > +class Cleaner\n> > +{\n> > +public:\n> > +\t~Cleaner();\n> > +\n> > +\tvoid defer(std::function &&action);\n> > +\tvoid clear();\n> > +\n> > +private:\n> > +\tstd::vector> actions_;\n> > +};\n> > +\n> > } /* namespace utils */\n> >\n> > #ifndef __DOXYGEN__\n> > diff --git a/src/libcamera/base/utils.cpp b/src/libcamera/base/utils.cpp\n> > index 9cd6cb197243..45e115b7031e 100644\n> > --- a/src/libcamera/base/utils.cpp\n> > +++ b/src/libcamera/base/utils.cpp\n> > @@ -484,6 +484,136 @@ std::string toAscii(const std::string &str)\n> > * default-constructed T.\n> > */\n> >\n> > +/**\n> > + * \\class Cleaner\n> > + * \\brief An object that performs actions upon destruction\n> > + *\n> > + * The Cleaner class is a simple object that performs user-provided actions\n> > + * upon destruction. As its name implies, it is meant to simplify cleanup tasks\n> > + * in error handling paths.\n> > + *\n> > + * When the code flow performs multiple sequential actions that each need a\n> > + * corresponding cleanup action, error handling quickly become tedious:\n> > + *\n> > + * \\code{.cpp}\n> > + * {\n> > + * \tint ret = allocateMemory();\n> > + * \tif (ret)\n> > + * \t\treturn ret;\n> > + *\n> > + * \tret = startProducder();\n> > + * \tif (ret) {\n> > + * \t\tfreeMemory();\n> > + * \t\treturn ret;\n> > + * \t}\n> > + *\n> > + * \tret = startConsumer();\n> > + * \tif (ret) {\n> > + * \t\tstopProducer();\n> > + * \t\tfreeMemory();\n> > + * \t\treturn ret;\n> > + * \t}\n> > + *\n> > + * \treturn 0;\n> > + * }\n> > + * \\endcode\n> > + *\n> > + * This is prone to programming mistakes, as cleanup actions can easily be\n> > + * forgotten or ordered incorrectly. One strategy to simplify error handling is\n> > + * to use goto statements:\n> > + *\n> > + * \\code{.cpp}\n> > + * {\n> > + * \tint ret = allocateMemory();\n> > + * \tif (ret)\n> > + * \t\treturn ret;\n> > + *\n> > + * \tret = startProducder();\n> > + * \tif (ret)\n> > + * \t\tgoto error_free;\n> > + *\n> > + * \tret = startConsumer();\n> > + * \tif (ret)\n> > + * \t\tgoto error_stop;\n> > + *\n> > + * \treturn 0;\n> > + *\n> > + * error_stop:\n> > + * \tstopProducer();\n> > + * error_free:\n> > + * \tfreeMemory();\n> > + * \treturn ret;\n> > + * }\n> > + * \\endcode\n> > + *\n> > + * While this may be considered better, this solution is still quite\n> > + * error-prone. Beside the risk of picking the wrong error label, the error\n> > + * handling logic is separated from the normal code flow, which increases the\n> > + * risk of error when refactoring the code. Additionally, C++ doesn't allow\n> > + * goto statements to jump over local variable declarations, which can make\n> > + * usage of this pattern more difficult.\n> > + *\n> > + * The Cleaner class solves these issues by allowing code that requires cleanup\n> > + * actions to be grouped with its corresponding deferred error handling code:\n> > + *\n> > + * \\code{.cpp}\n> > + * {\n> > + * \tCleaner cleaner;\n> > + *\n> > + * \tint ret = allocateMemory();\n> > + * \tif (ret)\n> > + * \t\treturn ret;\n> > + *\n> > + * \tcleaner.defer([&]() { freeMemory(); });\n> > + *\n> > + * \tret = startProducder();\n> > + * \tif (ret)\n> > + * \t\treturn ret;\n> > + *\n> > + * \tcleaner.defer([&]() { stopProducer(); });\n> > + *\n> > + * \tret = startConsumer();\n> > + * \tif (ret)\n> > + * \t\treturn ret;\n> > + *\n> > + * \tcleaner.clear();\n> > + * \treturn 0;\n> > + * }\n> > + * \\endcode\n> \n> I just wonder if\n> \n> {\n> \tCleaner cleaner;\n> \n> \tint ret = allocateMemory();\n> \tif (ret)\n> \t\treturn ret;\n> \n> \tcleaner.defer([&]() { freeMemory(); });\n> \n> \tret = startProducder();\n> \tif (ret) {\n> cleaner.clean();\n> \t\treturn ret;\n> }\n> \n> \tcleaner.defer([&]() { stopProducer(); });\n> \n> \tret = startConsumer();\n> \tif (ret) {\n> cleaner.clean();\n> \t\treturn ret;\n> }\n> \n> \treturn 0;\n> }\n> \n> \n> IOW make Claner::clean() execute the cleaning actions while\n> destruction is a no-op.\n\nI've thought about it, and I like the idea of having no additional\nfunction call in the success path. However, that brings other issues:\n\n- There are usually more error paths than success paths, so there will\n be more clean() calls needed, with more risks of forgetting one of\n them.\n\n- Error paths are less tested. A missing clear() in the success path\n will likely be detected very quickly (as all the deferred cleaner\n actions will be executed), while a missing clean() in one error path\n has a much higher chance to end up not being detected for a long time.\n\nAnother option would be to have dedicated calls for both the error and\nsuccess paths, and warn loudly if the object is destructed without any\nof these functions having been called. It will ensure we get a warning\nthe first time an incorrect error path is taken, but we'll still have\nrisks of missing calls in error paths being detected.\n\nYet another option is to hook up the Cleaner class in the return\nstatement itself, writing\n\n\treturn cleaner.return(ret);\n\nret == 0 would be considered a success and ret != 0 an error. Again,\nnothing will guarantee that a stray\n\n\treturn ret;\n\ngets added to an error path, but it may be easier to visually catch the\noffending code during review (not entirely sure about that).\n\nYet another option would be to change the function to return a Result\ninstead of an int, with the Cleaner::return() (or Cleaner::result(),\nname bikeshading is allowed) function returning a Result. Here again,\nsomeone may create a Result directly, so it may not help much.\n\nIf we were using exceptions the Cleaner destructor could differentiate\nbetween the success and failure cases more easily. I can't see another\nelegant method to achieve the same with error values, but maybe I'm\nmissing something.\n\n> It just feel more natural to have a clean() call on error paths and\n> not having to call clean() on a succesfull return.\n\nNote that it's \"clear\", not \"clean\", in the success path. I think I'll\nrename it to release() to avoid confusion, and match\nhttps://en.cppreference.com/w/cpp/experimental/scope_fail.\n\n> > + *\n> > + * Deferred error handlers are executed when the Cleaner instance is destroyed,\n> > + * in the reverse order of their addition.\n> > + */\n> > +\n> > +Cleaner::~Cleaner()\n> > +{\n> > +\tfor (const auto &action : utils::reverse(actions_))\n> > +\t\taction();\n> > +}\n> > +\n> > +/**\n> > + * \\brief Add a deferred cleanup action\n> > + * \\param[in] action The cleanup action\n> > + *\n> > + * Add a deferred cleanup action to the Cleaner. Cleanup actions will be called\n> > + * upon destruction in the reverse order of their addition.\n> > + */\n> > +void Cleaner::defer(std::function &&action)\n> > +{\n> > +\tactions_.push_back(std::move(action));\n> > +}\n> > +\n> > +/**\n> > + * \\brief Clear all deferred cleanup actions\n> > + *\n> > + * This function should be called in the success path to clear all deferred\n> > + * error cleanup actions.\n> > + */\n> > +void Cleaner::clear()\n> > +{\n> > +\tactions_.clear();\n> > +}\n> > +\n> > } /* namespace utils */\n> >\n> > #ifndef __DOXYGEN__","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 854F3BD16B\n\tfor ;\n\tFri, 7 Oct 2022 15:26:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0651662D28;\n\tFri, 7 Oct 2022 17:26:08 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1446F60A88\n\tfor ;\n\tFri, 7 Oct 2022 17:26:06 +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 6A595BBE;\n\tFri, 7 Oct 2022 17:26:05 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1665156368;\n\tbh=wGhMd5CvJcvC5m9Y2h7zoK9aVV75WoHyoyPzF0CPgig=;\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=TbgkM075F4vrSXsd0CD5qJ0txhLHCTyAfHfu0uPLsRbv10H3fH5RKShmipvTY+Fin\n\tztSf+cdxpZmE7lMaqbI+kb7MPdGpuSXrF5SiiLuJFN03YV0w95rr7LIa7huS/oAfaf\n\tgte3cYF1ua3u/letrPRPSUJz4YrzGzO+6RpDvMIY/bwmgZ2XxeS1YxMo06Cbm/bwT8\n\tdVFfkyialP8IH5q71XS+nGjzQcXLxrsCoueUSWmm18xy7cqZKxH2mpCdJwzKnlPu2F\n\tcm9lirK3DiGhxWTcLXiaoH1Xhb+cuXNIWPGyRTaRE2PuYgdLolmFv5vjc8FMlLJuuy\n\tSyfNf1FSqn1oA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1665156365;\n\tbh=wGhMd5CvJcvC5m9Y2h7zoK9aVV75WoHyoyPzF0CPgig=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=WJKRcUKOQHSDEceSYv0erUuoIzjo2lQRj9Lxz2V9hZF0So1Fq+2A/wC4IzoftjxkM\n\t2yzeBPWA9jN75wV0hvvwEYsyCDCmKclBTt/f/aMyWsKLq4KSSi9hcx+Qnc/CVACGiL\n\tX6g+O0bqluNrXsQE6a//VQ24vkLm3rPOunmeAkFY="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"WJKRcUKO\"; dkim-atps=neutral","Date":"Fri, 7 Oct 2022 18:26:00 +0300","To":"Jacopo Mondi ","Message-ID":"","References":"<20221004021842.6345-1-laurent.pinchart@ideasonboard.com>\n\t<20221004021842.6345-2-laurent.pinchart@ideasonboard.com>\n\t<20221007150157.zgq3csc4nafkflyh@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20221007150157.zgq3csc4nafkflyh@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: utils: Add Cleaner\n\tclass","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\" "}},{"id":25385,"web_url":"https://patchwork.libcamera.org/comment/25385/","msgid":"<166550014862.3760285.13256248044360455389@Monstersaurus>","date":"2022-10-11T14:55:48","subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: utils: Add Cleaner\n\tclass","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Jacopo Mondi via libcamera-devel (2022-10-07 16:01:57)\n> Hi Laurent\n> \n> On Tue, Oct 04, 2022 at 05:18:41AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> > The Cleaner class is a simple object that performs user-provided actions\n> > upon destruction. As its name implies, it is meant to simplify cleanup\n> > tasks in error handling paths.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > include/libcamera/base/utils.h | 13 ++++\n> > src/libcamera/base/utils.cpp | 130 +++++++++++++++++++++++++++++++++\n> > 2 files changed, 143 insertions(+)\n> >\n> > diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h\n> > index ed88b7163770..0de80eb89d15 100644\n> > --- a/include/libcamera/base/utils.h\n> > +++ b/include/libcamera/base/utils.h\n> > @@ -9,6 +9,7 @@\n> >\n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -381,6 +382,18 @@ struct defopt_t {\n> >\n> > constexpr details::defopt_t defopt;\n> >\n> > +class Cleaner\n> > +{\n> > +public:\n> > + ~Cleaner();\n> > +\n> > + void defer(std::function &&action);\n> > + void clear();\n> > +\n> > +private:\n> > + std::vector> actions_;\n> > +};\n> > +\n> > } /* namespace utils */\n> >\n> > #ifndef __DOXYGEN__\n> > diff --git a/src/libcamera/base/utils.cpp b/src/libcamera/base/utils.cpp\n> > index 9cd6cb197243..45e115b7031e 100644\n> > --- a/src/libcamera/base/utils.cpp\n> > +++ b/src/libcamera/base/utils.cpp\n> > @@ -484,6 +484,136 @@ std::string toAscii(const std::string &str)\n> > * default-constructed T.\n> > */\n> >\n> > +/**\n> > + * \\class Cleaner\n> > + * \\brief An object that performs actions upon destruction\n> > + *\n> > + * The Cleaner class is a simple object that performs user-provided actions\n> > + * upon destruction. As its name implies, it is meant to simplify cleanup tasks\n> > + * in error handling paths.\n> > + *\n> > + * When the code flow performs multiple sequential actions that each need a\n> > + * corresponding cleanup action, error handling quickly become tedious:\n> > + *\n> > + * \\code{.cpp}\n> > + * {\n> > + * int ret = allocateMemory();\n> > + * if (ret)\n> > + * return ret;\n> > + *\n> > + * ret = startProducder();\n> > + * if (ret) {\n> > + * freeMemory();\n> > + * return ret;\n> > + * }\n> > + *\n> > + * ret = startConsumer();\n> > + * if (ret) {\n> > + * stopProducer();\n> > + * freeMemory();\n> > + * return ret;\n> > + * }\n> > + *\n> > + * return 0;\n> > + * }\n> > + * \\endcode\n> > + *\n> > + * This is prone to programming mistakes, as cleanup actions can easily be\n> > + * forgotten or ordered incorrectly. One strategy to simplify error handling is\n> > + * to use goto statements:\n> > + *\n> > + * \\code{.cpp}\n> > + * {\n> > + * int ret = allocateMemory();\n> > + * if (ret)\n> > + * return ret;\n> > + *\n> > + * ret = startProducder();\n> > + * if (ret)\n> > + * goto error_free;\n> > + *\n> > + * ret = startConsumer();\n> > + * if (ret)\n> > + * goto error_stop;\n> > + *\n> > + * return 0;\n> > + *\n> > + * error_stop:\n> > + * stopProducer();\n> > + * error_free:\n> > + * freeMemory();\n> > + * return ret;\n> > + * }\n> > + * \\endcode\n> > + *\n> > + * While this may be considered better, this solution is still quite\n> > + * error-prone. Beside the risk of picking the wrong error label, the error\n> > + * handling logic is separated from the normal code flow, which increases the\n> > + * risk of error when refactoring the code. Additionally, C++ doesn't allow\n> > + * goto statements to jump over local variable declarations, which can make\n> > + * usage of this pattern more difficult.\n> > + *\n> > + * The Cleaner class solves these issues by allowing code that requires cleanup\n> > + * actions to be grouped with its corresponding deferred error handling code:\n> > + *\n> > + * \\code{.cpp}\n> > + * {\n> > + * Cleaner cleaner;\n> > + *\n> > + * int ret = allocateMemory();\n> > + * if (ret)\n> > + * return ret;\n> > + *\n> > + * cleaner.defer([&]() { freeMemory(); });\n> > + *\n> > + * ret = startProducder();\n> > + * if (ret)\n> > + * return ret;\n> > + *\n> > + * cleaner.defer([&]() { stopProducer(); });\n> > + *\n> > + * ret = startConsumer();\n> > + * if (ret)\n> > + * return ret;\n> > + *\n> > + * cleaner.clear();\n> > + * return 0;\n> > + * }\n> > + * \\endcode\n> \n> I just wonder if\n> \n> {\n> Cleaner cleaner;\n> \n> int ret = allocateMemory();\n> if (ret)\n> return ret;\n> \n> cleaner.defer([&]() { freeMemory(); });\n> \n> ret = startProducder();\n> if (ret) {\n> cleaner.clean();\n> return ret;\n> }\n> \n> cleaner.defer([&]() { stopProducer(); });\n> \n> ret = startConsumer();\n> if (ret) {\n> cleaner.clean();\n> return ret;\n> }\n> \n> return 0;\n> }\n> \n> \n> IOW make Claner::clean() execute the cleaning actions while\n> destruction is a no-op.\n> \n> It just feel more natural to have a clean() call on error paths and\n> not having to call clean() on a succesfull return.\n\nIf we use the name 'Cleaner' then, I'd actually agree here, as it would\nbe clear we're recording and collecting actions to 'clean', and\nexplicitly specifying 'when' to run them.\n\nBut as I said in the cover letter, I prefer the 'ScopeExit' terms, and I\nthink it's more likely that we'd catch bugs if we forget to 'cancel' a\ncleanup/defer operation, than catch them if we forget to act on the\ncleanups on a less used error path...\n\n\n> > + *\n> > + * Deferred error handlers are executed when the Cleaner instance is destroyed,\n> > + * in the reverse order of their addition.\n> > + */\n> > +\n> > +Cleaner::~Cleaner()\n> > +{\n> > + for (const auto &action : utils::reverse(actions_))\n> > + action();\n> > +}\n> > +\n> > +/**\n> > + * \\brief Add a deferred cleanup action\n> > + * \\param[in] action The cleanup action\n> > + *\n> > + * Add a deferred cleanup action to the Cleaner. Cleanup actions will be called\n> > + * upon destruction in the reverse order of their addition.\n> > + */\n> > +void Cleaner::defer(std::function &&action)\n> > +{\n> > + actions_.push_back(std::move(action));\n> > +}\n> > +\n> > +/**\n> > + * \\brief Clear all deferred cleanup actions\n> > + *\n> > + * This function should be called in the success path to clear all deferred\n> > + * error cleanup actions.\n> > + */\n> > +void Cleaner::clear()\n> > +{\n> > + actions_.clear();\n> > +}\n> > +\n> > } /* namespace utils */\n> >\n> > #ifndef __DOXYGEN__\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 62432BD16B\n\tfor ;\n\tTue, 11 Oct 2022 14:55:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D98E862D7C;\n\tTue, 11 Oct 2022 16:55:53 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 40D1D603F3\n\tfor ;\n\tTue, 11 Oct 2022 16:55:52 +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 C24647F9;\n\tTue, 11 Oct 2022 16:55:51 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1665500153;\n\tbh=2j6+81lcsrJGFJ785ikdwnsTFfkTXZCeBS9sw+SVJRs=;\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:Cc:\n\tFrom;\n\tb=KkygGd9fEQ+Jycy9fEWWqNsYUxF2XL5u+Pt+l7OxwSa6kGQStU3bhB4HFYWbD/pij\n\tcoT0Q+w6JPZQ0L8N81/H25t2Xh2nqwp9HuWE3Gr5S9mmFCaKf7ydTyjduLHG68JzeH\n\tNxwaE56OjaeXV1EXkXwjW54fXeHNPsO4Uk6bhHB4Pxgm01bIs6rRkvkc3tQ6UINdnn\n\tPVj56h87o1NT4FSIol5ZcdAprJJHgP7kyapM1AYB3xMyh8Zdgd8jpZVcsV3b6dTCFB\n\taBY/mroAus7hu3hBcbQk9/9J949Bb1SQ0gn/98jfZO7mzi//xQ9LD1bLrX3Tap3Eo3\n\t8XDbjbnMb7hSw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1665500151;\n\tbh=2j6+81lcsrJGFJ785ikdwnsTFfkTXZCeBS9sw+SVJRs=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=SdsLIRAV3BSYu8xDBSwQcE/cYnEDs3znccxoLR0Dkhk0LbyhDbAkAmS4BK0eQm9Mr\n\t1aaje/knkalrwvqErsLBLnpGwFUVSRZc/Sexs+stFXHKTf9ybnXy6XJcBo7vPc7MW/\n\tZzyLba1cZj/qV+r13mSGoEfsZGzVYpl8y6WfPmk0="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"SdsLIRAV\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20221007150157.zgq3csc4nafkflyh@uno.localdomain>","References":"<20221004021842.6345-1-laurent.pinchart@ideasonboard.com>\n\t<20221004021842.6345-2-laurent.pinchart@ideasonboard.com>\n\t<20221007150157.zgq3csc4nafkflyh@uno.localdomain>","To":"Jacopo Mondi , Jacopo Mondi via libcamera-devel\n\t, \n\tLaurent Pinchart ","Date":"Tue, 11 Oct 2022 15:55:48 +0100","Message-ID":"<166550014862.3760285.13256248044360455389@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: utils: Add Cleaner\n\tclass","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 ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25386,"web_url":"https://patchwork.libcamera.org/comment/25386/","msgid":"<166550060081.3760285.3545335618980566320@Monstersaurus>","date":"2022-10-11T15:03:20","subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: utils: Add Cleaner\n\tclass","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-07 16:26:00)\n> Hi Jacopo,\n> \n> On Fri, Oct 07, 2022 at 05:01:57PM +0200, Jacopo Mondi wrote:\n> > On Tue, Oct 04, 2022 at 05:18:41AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> > > The Cleaner class is a simple object that performs user-provided actions\n> > > upon destruction. As its name implies, it is meant to simplify cleanup\n> > > tasks in error handling paths.\n> > >\n> > > Signed-off-by: Laurent Pinchart \n> > > ---\n> > > include/libcamera/base/utils.h | 13 ++++\n> > > src/libcamera/base/utils.cpp | 130 +++++++++++++++++++++++++++++++++\n> > > 2 files changed, 143 insertions(+)\n> > >\n> > > diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h\n> > > index ed88b7163770..0de80eb89d15 100644\n> > > --- a/include/libcamera/base/utils.h\n> > > +++ b/include/libcamera/base/utils.h\n> > > @@ -9,6 +9,7 @@\n> > >\n> > > #include \n> > > #include \n> > > +#include \n> > > #include \n> > > #include \n> > > #include \n> > > @@ -381,6 +382,18 @@ struct defopt_t {\n> > >\n> > > constexpr details::defopt_t defopt;\n> > >\n> > > +class Cleaner\n> > > +{\n> > > +public:\n> > > + ~Cleaner();\n> > > +\n> > > + void defer(std::function &&action);\n> > > + void clear();\n> > > +\n> > > +private:\n> > > + std::vector> actions_;\n> > > +};\n> > > +\n> > > } /* namespace utils */\n> > >\n> > > #ifndef __DOXYGEN__\n> > > diff --git a/src/libcamera/base/utils.cpp b/src/libcamera/base/utils.cpp\n> > > index 9cd6cb197243..45e115b7031e 100644\n> > > --- a/src/libcamera/base/utils.cpp\n> > > +++ b/src/libcamera/base/utils.cpp\n> > > @@ -484,6 +484,136 @@ std::string toAscii(const std::string &str)\n> > > * default-constructed T.\n> > > */\n> > >\n> > > +/**\n> > > + * \\class Cleaner\n> > > + * \\brief An object that performs actions upon destruction\n> > > + *\n> > > + * The Cleaner class is a simple object that performs user-provided actions\n> > > + * upon destruction. As its name implies, it is meant to simplify cleanup tasks\n> > > + * in error handling paths.\n> > > + *\n> > > + * When the code flow performs multiple sequential actions that each need a\n> > > + * corresponding cleanup action, error handling quickly become tedious:\n> > > + *\n> > > + * \\code{.cpp}\n> > > + * {\n> > > + * int ret = allocateMemory();\n> > > + * if (ret)\n> > > + * return ret;\n> > > + *\n> > > + * ret = startProducder();\n> > > + * if (ret) {\n> > > + * freeMemory();\n> > > + * return ret;\n> > > + * }\n> > > + *\n> > > + * ret = startConsumer();\n> > > + * if (ret) {\n> > > + * stopProducer();\n> > > + * freeMemory();\n> > > + * return ret;\n> > > + * }\n> > > + *\n> > > + * return 0;\n> > > + * }\n> > > + * \\endcode\n> > > + *\n> > > + * This is prone to programming mistakes, as cleanup actions can easily be\n> > > + * forgotten or ordered incorrectly. One strategy to simplify error handling is\n> > > + * to use goto statements:\n> > > + *\n> > > + * \\code{.cpp}\n> > > + * {\n> > > + * int ret = allocateMemory();\n> > > + * if (ret)\n> > > + * return ret;\n> > > + *\n> > > + * ret = startProducder();\n> > > + * if (ret)\n> > > + * goto error_free;\n> > > + *\n> > > + * ret = startConsumer();\n> > > + * if (ret)\n> > > + * goto error_stop;\n> > > + *\n> > > + * return 0;\n> > > + *\n> > > + * error_stop:\n> > > + * stopProducer();\n> > > + * error_free:\n> > > + * freeMemory();\n> > > + * return ret;\n> > > + * }\n> > > + * \\endcode\n> > > + *\n> > > + * While this may be considered better, this solution is still quite\n> > > + * error-prone. Beside the risk of picking the wrong error label, the error\n> > > + * handling logic is separated from the normal code flow, which increases the\n> > > + * risk of error when refactoring the code. Additionally, C++ doesn't allow\n> > > + * goto statements to jump over local variable declarations, which can make\n> > > + * usage of this pattern more difficult.\n> > > + *\n> > > + * The Cleaner class solves these issues by allowing code that requires cleanup\n> > > + * actions to be grouped with its corresponding deferred error handling code:\n> > > + *\n> > > + * \\code{.cpp}\n> > > + * {\n> > > + * Cleaner cleaner;\n> > > + *\n> > > + * int ret = allocateMemory();\n> > > + * if (ret)\n> > > + * return ret;\n> > > + *\n> > > + * cleaner.defer([&]() { freeMemory(); });\n> > > + *\n> > > + * ret = startProducder();\n> > > + * if (ret)\n> > > + * return ret;\n> > > + *\n> > > + * cleaner.defer([&]() { stopProducer(); });\n> > > + *\n> > > + * ret = startConsumer();\n> > > + * if (ret)\n> > > + * return ret;\n> > > + *\n> > > + * cleaner.clear();\n> > > + * return 0;\n> > > + * }\n> > > + * \\endcode\n> > \n> > I just wonder if\n> > \n> > {\n> > Cleaner cleaner;\n> > \n> > int ret = allocateMemory();\n> > if (ret)\n> > return ret;\n> > \n> > cleaner.defer([&]() { freeMemory(); });\n> > \n> > ret = startProducder();\n> > if (ret) {\n> > cleaner.clean();\n> > return ret;\n> > }\n> > \n> > cleaner.defer([&]() { stopProducer(); });\n> > \n> > ret = startConsumer();\n> > if (ret) {\n> > cleaner.clean();\n> > return ret;\n> > }\n> > \n> > return 0;\n> > }\n> > \n> > \n> > IOW make Claner::clean() execute the cleaning actions while\n> > destruction is a no-op.\n> \n> I've thought about it, and I like the idea of having no additional\n> function call in the success path. However, that brings other issues:\n> \n> - There are usually more error paths than success paths, so there will\n> be more clean() calls needed, with more risks of forgetting one of\n> them.\n> \n> - Error paths are less tested. A missing clear() in the success path\n> will likely be detected very quickly (as all the deferred cleaner\n> actions will be executed), while a missing clean() in one error path\n> has a much higher chance to end up not being detected for a long time.\n\nAha, looks like I should have started here. I think both of the above\nare why I prefer automatic cleanup on error paths, and an explicit\ncancellation of deferred work on success. (Assuming a single success\npath).\n\n\n> Another option would be to have dedicated calls for both the error and\n> success paths, and warn loudly if the object is destructed without any\n> of these functions having been called. It will ensure we get a warning\n> the first time an incorrect error path is taken, but we'll still have\n> risks of missing calls in error paths being detected.\n> \n> Yet another option is to hook up the Cleaner class in the return\n> statement itself, writing\n> \n> return cleaner.return(ret);\n> \n> ret == 0 would be considered a success and ret != 0 an error. Again,\n> nothing will guarantee that a stray\n> \n> return ret;\n\nBut that would only work on integer/boolean return function types?\nUnless ... there was a ScopedReturn return type that /enforced/ use of\nan explicit return type whenever a ScopedExit / Cleaner class was in use\nin a function?\n\n(I'm not sure how that would be handled though, and it would be just\nanother wrapper around return types to cause confusion perhaps)\n\n\n> \n> gets added to an error path, but it may be easier to visually catch the\n> offending code during review (not entirely sure about that).\n> \n> Yet another option would be to change the function to return a Result\n> instead of an int, with the Cleaner::return() (or Cleaner::result(),\n> name bikeshading is allowed) function returning a Result. Here again,\n> someone may create a Result directly, so it may not help much.\n\nDamnit ... You have the same ideas before I get there...\n\n\n> If we were using exceptions the Cleaner destructor could differentiate\n> between the success and failure cases more easily. I can't see another\n> elegant method to achieve the same with error values, but maybe I'm\n> missing something.\n> \n> > It just feel more natural to have a clean() call on error paths and\n> > not having to call clean() on a succesfull return.\n> \n> Note that it's \"clear\", not \"clean\", in the success path. I think I'll\n> rename it to release() to avoid confusion, and match\n> https://en.cppreference.com/w/cpp/experimental/scope_fail.\n\nHrm... release() makes sense too, though for some reason I still think\nof it as a cancellation ... .cancel() ... but I'm not opposed to a\n.release().\n\n\n\n> \n> > > + *\n> > > + * Deferred error handlers are executed when the Cleaner instance is destroyed,\n> > > + * in the reverse order of their addition.\n> > > + */\n> > > +\n> > > +Cleaner::~Cleaner()\n> > > +{\n> > > + for (const auto &action : utils::reverse(actions_))\n> > > + action();\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Add a deferred cleanup action\n> > > + * \\param[in] action The cleanup action\n> > > + *\n> > > + * Add a deferred cleanup action to the Cleaner. Cleanup actions will be called\n> > > + * upon destruction in the reverse order of their addition.\n> > > + */\n> > > +void Cleaner::defer(std::function &&action)\n> > > +{\n> > > + actions_.push_back(std::move(action));\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Clear all deferred cleanup actions\n> > > + *\n> > > + * This function should be called in the success path to clear all deferred\n> > > + * error cleanup actions.\n> > > + */\n> > > +void Cleaner::clear()\n> > > +{\n> > > + actions_.clear();\n> > > +}\n> > > +\n> > > } /* namespace utils */\n> > >\n> > > #ifndef __DOXYGEN__\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","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 999E7C0DA4\n\tfor ;\n\tTue, 11 Oct 2022 15:03:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D35FE62D7E;\n\tTue, 11 Oct 2022 17:03:24 +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 9DA7B603F3\n\tfor ;\n\tTue, 11 Oct 2022 17:03:23 +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 0E0B2A42;\n\tTue, 11 Oct 2022 17:03:23 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1665500604;\n\tbh=XoCFJOps6UJmR9kMz+l1SRz6VeJK1SYBGxb+B7vkU2o=;\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:Cc:\n\tFrom;\n\tb=I64jWFgEy5QAtRuMI1kHOrZQk6QJXqwl3RY8BcetEj/qaTQtrDYSdcMZ6D6opUucb\n\tqFQ9XFADP3ZiivND1sYdu6jzGntNiKmzk0mkwmTsvlAIdI1PhaT1xGk7USsc0ClfKb\n\tj2ZL8JuAsv9T4gDuSqMuGhBHZAdR5+UIlUwHr8HWSH//SWQRW06ctR6b9qv+rKOLEm\n\tYpMBZAbtepqiLBCyg5R+HkD1mYRWyJ2koKVvSRkCfBIPLnVK/dpxWnDcuoBTAjCGib\n\tHiI8UF9XQYlUteeLF70Fyv9EGXF31j7nfxozT0uztuAunzzr9VtjyadNa+wlSqLQ/v\n\tILNjQ+Tt4smiw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1665500603;\n\tbh=XoCFJOps6UJmR9kMz+l1SRz6VeJK1SYBGxb+B7vkU2o=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=OZh9Q8L6PC3s8evdCjwO3EBlDHtE3FA+0EJSKl6bVfnkiIHXZ4pLD5Nt8TmdJ8IWX\n\tKU39+Coy51lGbGWTH5HMan/TLLUEnkF9y4HZQ/EGuqseq5dDudphfEipyIOMySCEZf\n\tgO7rj4/1KgUPsgyBx7wK5mkOMLMWjmZk1QFUF448="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"OZh9Q8L6\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20221004021842.6345-1-laurent.pinchart@ideasonboard.com>\n\t<20221004021842.6345-2-laurent.pinchart@ideasonboard.com>\n\t<20221007150157.zgq3csc4nafkflyh@uno.localdomain>\n\t","To":"Jacopo Mondi ,\n\tLaurent Pinchart ,\n\tLaurent Pinchart via libcamera-devel\n\t","Date":"Tue, 11 Oct 2022 16:03:20 +0100","Message-ID":"<166550060081.3760285.3545335618980566320@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: utils: Add Cleaner\n\tclass","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 ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25388,"web_url":"https://patchwork.libcamera.org/comment/25388/","msgid":"","date":"2022-10-11T20:29:37","subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: utils: Add Cleaner\n\tclass","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 04:03:20PM +0100, Kieran Bingham wrote:\n> Quoting Laurent Pinchart via libcamera-devel (2022-10-07 16:26:00)\n> > On Fri, Oct 07, 2022 at 05:01:57PM +0200, Jacopo Mondi wrote:\n> > > On Tue, Oct 04, 2022 at 05:18:41AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> > > > The Cleaner class is a simple object that performs user-provided actions\n> > > > upon destruction. As its name implies, it is meant to simplify cleanup\n> > > > tasks in error handling paths.\n> > > >\n> > > > Signed-off-by: Laurent Pinchart \n> > > > ---\n> > > > include/libcamera/base/utils.h | 13 ++++\n> > > > src/libcamera/base/utils.cpp | 130 +++++++++++++++++++++++++++++++++\n> > > > 2 files changed, 143 insertions(+)\n> > > >\n> > > > diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h\n> > > > index ed88b7163770..0de80eb89d15 100644\n> > > > --- a/include/libcamera/base/utils.h\n> > > > +++ b/include/libcamera/base/utils.h\n> > > > @@ -9,6 +9,7 @@\n> > > >\n> > > > #include \n> > > > #include \n> > > > +#include \n> > > > #include \n> > > > #include \n> > > > #include \n> > > > @@ -381,6 +382,18 @@ struct defopt_t {\n> > > >\n> > > > constexpr details::defopt_t defopt;\n> > > >\n> > > > +class Cleaner\n> > > > +{\n> > > > +public:\n> > > > + ~Cleaner();\n> > > > +\n> > > > + void defer(std::function &&action);\n> > > > + void clear();\n> > > > +\n> > > > +private:\n> > > > + std::vector> actions_;\n> > > > +};\n> > > > +\n> > > > } /* namespace utils */\n> > > >\n> > > > #ifndef __DOXYGEN__\n> > > > diff --git a/src/libcamera/base/utils.cpp b/src/libcamera/base/utils.cpp\n> > > > index 9cd6cb197243..45e115b7031e 100644\n> > > > --- a/src/libcamera/base/utils.cpp\n> > > > +++ b/src/libcamera/base/utils.cpp\n> > > > @@ -484,6 +484,136 @@ std::string toAscii(const std::string &str)\n> > > > * default-constructed T.\n> > > > */\n> > > >\n> > > > +/**\n> > > > + * \\class Cleaner\n> > > > + * \\brief An object that performs actions upon destruction\n> > > > + *\n> > > > + * The Cleaner class is a simple object that performs user-provided actions\n> > > > + * upon destruction. As its name implies, it is meant to simplify cleanup tasks\n> > > > + * in error handling paths.\n> > > > + *\n> > > > + * When the code flow performs multiple sequential actions that each need a\n> > > > + * corresponding cleanup action, error handling quickly become tedious:\n> > > > + *\n> > > > + * \\code{.cpp}\n> > > > + * {\n> > > > + * int ret = allocateMemory();\n> > > > + * if (ret)\n> > > > + * return ret;\n> > > > + *\n> > > > + * ret = startProducder();\n> > > > + * if (ret) {\n> > > > + * freeMemory();\n> > > > + * return ret;\n> > > > + * }\n> > > > + *\n> > > > + * ret = startConsumer();\n> > > > + * if (ret) {\n> > > > + * stopProducer();\n> > > > + * freeMemory();\n> > > > + * return ret;\n> > > > + * }\n> > > > + *\n> > > > + * return 0;\n> > > > + * }\n> > > > + * \\endcode\n> > > > + *\n> > > > + * This is prone to programming mistakes, as cleanup actions can easily be\n> > > > + * forgotten or ordered incorrectly. One strategy to simplify error handling is\n> > > > + * to use goto statements:\n> > > > + *\n> > > > + * \\code{.cpp}\n> > > > + * {\n> > > > + * int ret = allocateMemory();\n> > > > + * if (ret)\n> > > > + * return ret;\n> > > > + *\n> > > > + * ret = startProducder();\n> > > > + * if (ret)\n> > > > + * goto error_free;\n> > > > + *\n> > > > + * ret = startConsumer();\n> > > > + * if (ret)\n> > > > + * goto error_stop;\n> > > > + *\n> > > > + * return 0;\n> > > > + *\n> > > > + * error_stop:\n> > > > + * stopProducer();\n> > > > + * error_free:\n> > > > + * freeMemory();\n> > > > + * return ret;\n> > > > + * }\n> > > > + * \\endcode\n> > > > + *\n> > > > + * While this may be considered better, this solution is still quite\n> > > > + * error-prone. Beside the risk of picking the wrong error label, the error\n> > > > + * handling logic is separated from the normal code flow, which increases the\n> > > > + * risk of error when refactoring the code. Additionally, C++ doesn't allow\n> > > > + * goto statements to jump over local variable declarations, which can make\n> > > > + * usage of this pattern more difficult.\n> > > > + *\n> > > > + * The Cleaner class solves these issues by allowing code that requires cleanup\n> > > > + * actions to be grouped with its corresponding deferred error handling code:\n> > > > + *\n> > > > + * \\code{.cpp}\n> > > > + * {\n> > > > + * Cleaner cleaner;\n> > > > + *\n> > > > + * int ret = allocateMemory();\n> > > > + * if (ret)\n> > > > + * return ret;\n> > > > + *\n> > > > + * cleaner.defer([&]() { freeMemory(); });\n> > > > + *\n> > > > + * ret = startProducder();\n> > > > + * if (ret)\n> > > > + * return ret;\n> > > > + *\n> > > > + * cleaner.defer([&]() { stopProducer(); });\n> > > > + *\n> > > > + * ret = startConsumer();\n> > > > + * if (ret)\n> > > > + * return ret;\n> > > > + *\n> > > > + * cleaner.clear();\n> > > > + * return 0;\n> > > > + * }\n> > > > + * \\endcode\n> > > \n> > > I just wonder if\n> > > \n> > > {\n> > > Cleaner cleaner;\n> > > \n> > > int ret = allocateMemory();\n> > > if (ret)\n> > > return ret;\n> > > \n> > > cleaner.defer([&]() { freeMemory(); });\n> > > \n> > > ret = startProducder();\n> > > if (ret) {\n> > > cleaner.clean();\n> > > return ret;\n> > > }\n> > > \n> > > cleaner.defer([&]() { stopProducer(); });\n> > > \n> > > ret = startConsumer();\n> > > if (ret) {\n> > > cleaner.clean();\n> > > return ret;\n> > > }\n> > > \n> > > return 0;\n> > > }\n> > > \n> > > \n> > > IOW make Claner::clean() execute the cleaning actions while\n> > > destruction is a no-op.\n> > \n> > I've thought about it, and I like the idea of having no additional\n> > function call in the success path. However, that brings other issues:\n> > \n> > - There are usually more error paths than success paths, so there will\n> > be more clean() calls needed, with more risks of forgetting one of\n> > them.\n> > \n> > - Error paths are less tested. A missing clear() in the success path\n> > will likely be detected very quickly (as all the deferred cleaner\n> > actions will be executed), while a missing clean() in one error path\n> > has a much higher chance to end up not being detected for a long time.\n> \n> Aha, looks like I should have started here. I think both of the above\n> are why I prefer automatic cleanup on error paths, and an explicit\n> cancellation of deferred work on success. (Assuming a single success\n> path).\n> \n> > Another option would be to have dedicated calls for both the error and\n> > success paths, and warn loudly if the object is destructed without any\n> > of these functions having been called. It will ensure we get a warning\n> > the first time an incorrect error path is taken, but we'll still have\n> > risks of missing calls in error paths being detected.\n> > \n> > Yet another option is to hook up the Cleaner class in the return\n> > statement itself, writing\n> > \n> > return cleaner.return(ret);\n> > \n> > ret == 0 would be considered a success and ret != 0 an error. Again,\n> > nothing will guarantee that a stray\n> > \n> > return ret;\n> \n> But that would only work on integer/boolean return function types?\n> Unless ... there was a ScopedReturn return type that /enforced/ use of\n> an explicit return type whenever a ScopedExit / Cleaner class was in use\n> in a function?\n> \n> (I'm not sure how that would be handled though, and it would be just\n> another wrapper around return types to cause confusion perhaps)\n> \n> > gets added to an error path, but it may be easier to visually catch the\n> > offending code during review (not entirely sure about that).\n> > \n> > Yet another option would be to change the function to return a Result\n> > instead of an int, with the Cleaner::return() (or Cleaner::result(),\n> > name bikeshading is allowed) function returning a Result. Here again,\n> > someone may create a Result directly, so it may not help much.\n> \n> Damnit ... You have the same ideas before I get there...\n\nWe're getting dangerously close to rust ;-)\n\n> > If we were using exceptions the Cleaner destructor could differentiate\n> > between the success and failure cases more easily. I can't see another\n> > elegant method to achieve the same with error values, but maybe I'm\n> > missing something.\n> > \n> > > It just feel more natural to have a clean() call on error paths and\n> > > not having to call clean() on a succesfull return.\n> > \n> > Note that it's \"clear\", not \"clean\", in the success path. I think I'll\n> > rename it to release() to avoid confusion, and match\n> > https://en.cppreference.com/w/cpp/experimental/scope_fail.\n> \n> Hrm... release() makes sense too, though for some reason I still think\n> of it as a cancellation ... .cancel() ... but I'm not opposed to a\n> .release().\n\nI like .cancel() as well, but .release() being more standard, I think it\nhas my preference.\n\n> > > > + *\n> > > > + * Deferred error handlers are executed when the Cleaner instance is destroyed,\n> > > > + * in the reverse order of their addition.\n> > > > + */\n> > > > +\n> > > > +Cleaner::~Cleaner()\n> > > > +{\n> > > > + for (const auto &action : utils::reverse(actions_))\n> > > > + action();\n> > > > +}\n> > > > +\n> > > > +/**\n> > > > + * \\brief Add a deferred cleanup action\n> > > > + * \\param[in] action The cleanup action\n> > > > + *\n> > > > + * Add a deferred cleanup action to the Cleaner. Cleanup actions will be called\n> > > > + * upon destruction in the reverse order of their addition.\n> > > > + */\n> > > > +void Cleaner::defer(std::function &&action)\n> > > > +{\n> > > > + actions_.push_back(std::move(action));\n> > > > +}\n> > > > +\n> > > > +/**\n> > > > + * \\brief Clear all deferred cleanup actions\n> > > > + *\n> > > > + * This function should be called in the success path to clear all deferred\n> > > > + * error cleanup actions.\n> > > > + */\n> > > > +void Cleaner::clear()\n> > > > +{\n> > > > + actions_.clear();\n> > > > +}\n> > > > +\n> > > > } /* namespace utils */\n> > > >\n> > > > #ifndef __DOXYGEN__","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 4590FBD16B\n\tfor ;\n\tTue, 11 Oct 2022 20:29:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BBEFB62D80;\n\tTue, 11 Oct 2022 22:29:45 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 71330603F3\n\tfor ;\n\tTue, 11 Oct 2022 22:29:44 +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 BC86F907;\n\tTue, 11 Oct 2022 22:29:43 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1665520185;\n\tbh=KMkOQ77T/2bCQMV9R3df8POA/0d49hZ5WqK4F2gRm5E=;\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=jJHxcGJcUNIMLbTsNDbXMagKTyHgTCs8+KUdJb7ZzUGom0rn6qIUdtRy7FyQtimCJ\n\t6mmt/xRs/z98ap4jYHXVMw8oQtxLXCwe7Edf+3wz4dyh2uGSalE1wolVECJon4Lbwv\n\tXfriTw+L6XmM6m19+fjJar6Dg6/3NwVRJaGbppeuJSskwRteOc6M6Hbm3XyTFegXGc\n\trJAWd5YS4VwBHZ/UUbLGdXNmGMgTde9W9oRUZBccoWK5+gXxzDZFxiFknmER42fpiW\n\tGRiMKXTpOnR26B+ki/ZYAXdg5lWLR8JRpORW839DCyytjOEb5Ns159QQ0Xw5zQTYvz\n\tC/bzyYPzEA29g==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1665520184;\n\tbh=KMkOQ77T/2bCQMV9R3df8POA/0d49hZ5WqK4F2gRm5E=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=NaSai8bCpFciNzP1UX0lHGVpvti40zfsaCXzVERs+x5lP+XxNbq5JlKi3AWyPMGut\n\tHkH+wuhqb0A9KGZvFUK+IyyJFLB+kGXwAnjJq7PbJMQqa6vSiFppuMOjTryWrdVJVE\n\tmLU3kNuRPS5UMP67JL1yy9qjDfMMSxbk0HtPA1D0="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"NaSai8bC\"; dkim-atps=neutral","Date":"Tue, 11 Oct 2022 23:29:37 +0300","To":"Kieran Bingham ","Message-ID":"","References":"<20221004021842.6345-1-laurent.pinchart@ideasonboard.com>\n\t<20221004021842.6345-2-laurent.pinchart@ideasonboard.com>\n\t<20221007150157.zgq3csc4nafkflyh@uno.localdomain>\n\t\n\t<166550060081.3760285.3545335618980566320@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<166550060081.3760285.3545335618980566320@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: utils: Add Cleaner\n\tclass","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":"Laurent Pinchart via libcamera-devel\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25485,"web_url":"https://patchwork.libcamera.org/comment/25485/","msgid":"<20221020054627.GA3874866@pyrite.rasen.tech>","date":"2022-10-20T05:46:27","subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: utils: Add Cleaner\n\tclass","submitter":{"id":97,"url":"https://patchwork.libcamera.org/api/people/97/","name":"Nicolas Dufresne via libcamera-devel","email":"libcamera-devel@lists.libcamera.org"},"content":"On Fri, Oct 07, 2022 at 06:26:00PM +0300, Laurent Pinchart via libcamera-devel wrote:\n> Hi Jacopo,\n> \n> On Fri, Oct 07, 2022 at 05:01:57PM +0200, Jacopo Mondi wrote:\n> > On Tue, Oct 04, 2022 at 05:18:41AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> > > The Cleaner class is a simple object that performs user-provided actions\n> > > upon destruction. As its name implies, it is meant to simplify cleanup\n> > > tasks in error handling paths.\n> > >\n> > > Signed-off-by: Laurent Pinchart \n> > > ---\n> > > include/libcamera/base/utils.h | 13 ++++\n> > > src/libcamera/base/utils.cpp | 130 +++++++++++++++++++++++++++++++++\n> > > 2 files changed, 143 insertions(+)\n> > >\n> > > diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h\n> > > index ed88b7163770..0de80eb89d15 100644\n> > > --- a/include/libcamera/base/utils.h\n> > > +++ b/include/libcamera/base/utils.h\n> > > @@ -9,6 +9,7 @@\n> > >\n> > > #include \n> > > #include \n> > > +#include \n> > > #include \n> > > #include \n> > > #include \n> > > @@ -381,6 +382,18 @@ struct defopt_t {\n> > >\n> > > constexpr details::defopt_t defopt;\n> > >\n> > > +class Cleaner\n> > > +{\n> > > +public:\n> > > +\t~Cleaner();\n> > > +\n> > > +\tvoid defer(std::function &&action);\n> > > +\tvoid clear();\n> > > +\n> > > +private:\n> > > +\tstd::vector> actions_;\n> > > +};\n> > > +\n> > > } /* namespace utils */\n> > >\n> > > #ifndef __DOXYGEN__\n> > > diff --git a/src/libcamera/base/utils.cpp b/src/libcamera/base/utils.cpp\n> > > index 9cd6cb197243..45e115b7031e 100644\n> > > --- a/src/libcamera/base/utils.cpp\n> > > +++ b/src/libcamera/base/utils.cpp\n> > > @@ -484,6 +484,136 @@ std::string toAscii(const std::string &str)\n> > > * default-constructed T.\n> > > */\n> > >\n> > > +/**\n> > > + * \\class Cleaner\n> > > + * \\brief An object that performs actions upon destruction\n> > > + *\n> > > + * The Cleaner class is a simple object that performs user-provided actions\n> > > + * upon destruction. As its name implies, it is meant to simplify cleanup tasks\n> > > + * in error handling paths.\n> > > + *\n> > > + * When the code flow performs multiple sequential actions that each need a\n> > > + * corresponding cleanup action, error handling quickly become tedious:\n> > > + *\n> > > + * \\code{.cpp}\n> > > + * {\n> > > + * \tint ret = allocateMemory();\n> > > + * \tif (ret)\n> > > + * \t\treturn ret;\n> > > + *\n> > > + * \tret = startProducder();\n> > > + * \tif (ret) {\n> > > + * \t\tfreeMemory();\n> > > + * \t\treturn ret;\n> > > + * \t}\n> > > + *\n> > > + * \tret = startConsumer();\n> > > + * \tif (ret) {\n> > > + * \t\tstopProducer();\n> > > + * \t\tfreeMemory();\n> > > + * \t\treturn ret;\n> > > + * \t}\n> > > + *\n> > > + * \treturn 0;\n> > > + * }\n> > > + * \\endcode\n> > > + *\n> > > + * This is prone to programming mistakes, as cleanup actions can easily be\n> > > + * forgotten or ordered incorrectly. One strategy to simplify error handling is\n> > > + * to use goto statements:\n> > > + *\n> > > + * \\code{.cpp}\n> > > + * {\n> > > + * \tint ret = allocateMemory();\n> > > + * \tif (ret)\n> > > + * \t\treturn ret;\n> > > + *\n> > > + * \tret = startProducder();\n> > > + * \tif (ret)\n> > > + * \t\tgoto error_free;\n> > > + *\n> > > + * \tret = startConsumer();\n> > > + * \tif (ret)\n> > > + * \t\tgoto error_stop;\n> > > + *\n> > > + * \treturn 0;\n> > > + *\n> > > + * error_stop:\n> > > + * \tstopProducer();\n> > > + * error_free:\n> > > + * \tfreeMemory();\n> > > + * \treturn ret;\n> > > + * }\n> > > + * \\endcode\n> > > + *\n> > > + * While this may be considered better, this solution is still quite\n> > > + * error-prone. Beside the risk of picking the wrong error label, the error\n> > > + * handling logic is separated from the normal code flow, which increases the\n> > > + * risk of error when refactoring the code. Additionally, C++ doesn't allow\n> > > + * goto statements to jump over local variable declarations, which can make\n> > > + * usage of this pattern more difficult.\n> > > + *\n> > > + * The Cleaner class solves these issues by allowing code that requires cleanup\n> > > + * actions to be grouped with its corresponding deferred error handling code:\n> > > + *\n> > > + * \\code{.cpp}\n> > > + * {\n> > > + * \tCleaner cleaner;\n> > > + *\n> > > + * \tint ret = allocateMemory();\n> > > + * \tif (ret)\n> > > + * \t\treturn ret;\n> > > + *\n> > > + * \tcleaner.defer([&]() { freeMemory(); });\n> > > + *\n> > > + * \tret = startProducder();\n> > > + * \tif (ret)\n> > > + * \t\treturn ret;\n> > > + *\n> > > + * \tcleaner.defer([&]() { stopProducer(); });\n> > > + *\n> > > + * \tret = startConsumer();\n> > > + * \tif (ret)\n> > > + * \t\treturn ret;\n> > > + *\n> > > + * \tcleaner.clear();\n> > > + * \treturn 0;\n> > > + * }\n> > > + * \\endcode\n> > \n> > I just wonder if\n> > \n> > {\n> > \tCleaner cleaner;\n> > \n> > \tint ret = allocateMemory();\n> > \tif (ret)\n> > \t\treturn ret;\n> > \n> > \tcleaner.defer([&]() { freeMemory(); });\n> > \n> > \tret = startProducder();\n> > \tif (ret) {\n> > cleaner.clean();\n> > \t\treturn ret;\n> > }\n> > \n> > \tcleaner.defer([&]() { stopProducer(); });\n> > \n> > \tret = startConsumer();\n> > \tif (ret) {\n> > cleaner.clean();\n> > \t\treturn ret;\n> > }\n> > \n> > \treturn 0;\n> > }\n> > \n> > \n> > IOW make Claner::clean() execute the cleaning actions while\n> > destruction is a no-op.\n> \n> I've thought about it, and I like the idea of having no additional\n> function call in the success path. However, that brings other issues:\n> \n> - There are usually more error paths than success paths, so there will\n> be more clean() calls needed, with more risks of forgetting one of\n> them.\n> \n> - Error paths are less tested. A missing clear() in the success path\n> will likely be detected very quickly (as all the deferred cleaner\n> actions will be executed), while a missing clean() in one error path\n> has a much higher chance to end up not being detected for a long time.\n> \n> Another option would be to have dedicated calls for both the error and\n> success paths, and warn loudly if the object is destructed without any\n> of these functions having been called. It will ensure we get a warning\n> the first time an incorrect error path is taken, but we'll still have\n> risks of missing calls in error paths being detected.\n> \n> Yet another option is to hook up the Cleaner class in the return\n> statement itself, writing\n> \n> \treturn cleaner.return(ret);\n\nI was thinking this too, given that cleaner.clean() that Jacopo proposed\nabove adds more fluff. But then again, error paths are less tested, and\nare more frequent, so I think that having those auto-clean on scope exit\nis better.\n\nExplicit cleanup on success I guess is an acceptable side-product of\nthis protection.\n\n\nReviewed-by: Paul Elder \n\n> \n> ret == 0 would be considered a success and ret != 0 an error. Again,\n> nothing will guarantee that a stray\n> \n> \treturn ret;\n> \n> gets added to an error path, but it may be easier to visually catch the\n> offending code during review (not entirely sure about that).\n> \n> Yet another option would be to change the function to return a Result\n> instead of an int, with the Cleaner::return() (or Cleaner::result(),\n> name bikeshading is allowed) function returning a Result. Here again,\n> someone may create a Result directly, so it may not help much.\n> \n> If we were using exceptions the Cleaner destructor could differentiate\n> between the success and failure cases more easily. I can't see another\n> elegant method to achieve the same with error values, but maybe I'm\n> missing something.\n> \n> > It just feel more natural to have a clean() call on error paths and\n> > not having to call clean() on a succesfull return.\n> \n> Note that it's \"clear\", not \"clean\", in the success path. I think I'll\n> rename it to release() to avoid confusion, and match\n> https://en.cppreference.com/w/cpp/experimental/scope_fail.\n> \n> > > + *\n> > > + * Deferred error handlers are executed when the Cleaner instance is destroyed,\n> > > + * in the reverse order of their addition.\n> > > + */\n> > > +\n> > > +Cleaner::~Cleaner()\n> > > +{\n> > > +\tfor (const auto &action : utils::reverse(actions_))\n> > > +\t\taction();\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Add a deferred cleanup action\n> > > + * \\param[in] action The cleanup action\n> > > + *\n> > > + * Add a deferred cleanup action to the Cleaner. Cleanup actions will be called\n> > > + * upon destruction in the reverse order of their addition.\n> > > + */\n> > > +void Cleaner::defer(std::function &&action)\n> > > +{\n> > > +\tactions_.push_back(std::move(action));\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Clear all deferred cleanup actions\n> > > + *\n> > > + * This function should be called in the success path to clear all deferred\n> > > + * error cleanup actions.\n> > > + */\n> > > +void Cleaner::clear()\n> > > +{\n> > > +\tactions_.clear();\n> > > +}\n> > > +\n> > > } /* namespace utils */\n> > >\n> > > #ifndef __DOXYGEN__\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","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 18AA2C0DA4\n\tfor ;\n\tThu, 20 Oct 2022 05:46:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6A03962E8C;\n\tThu, 20 Oct 2022 07:46:37 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id ECF8262E75\n\tfor ;\n\tThu, 20 Oct 2022 07:46:35 +0200 (CEST)","from pyrite.rasen.tech (h175-177-042-159.catv02.itscom.jp\n\t[175.177.42.159])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id F0B76570;\n\tThu, 20 Oct 2022 07:46:33 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1666244797;\n\tbh=T3YtLW059JpYyLRUeHnIu3ovR8rzkrrA4y194kCsJwk=;\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=Bpz8TFygLPb7N4L1pdc0ovSMVwvk6W6kcouYORANle3V111APjDyzaNSzqbcb2qVl\n\tSzP2O0VuFXsIN0HBkVx5+mH+/IKNBZkbWEHXBi5msxAsV295Q1+I6gucxYvat7cunw\n\tftIPntP60AkuStiZ3rqcspbs7wKk+gzAlarims7vOMb8BKwfLM1HVQJnh248TOYY7T\n\tJhokB+RPMIaWIXOBiTNiPraD0q65ajINtNVJLAXHBFSVTL4MjA0dYILiCKNlcxkx0t\n\tVfoQVHVNo+NXyj7JuqIu/hgBpYk9DA8jE3gS2Kl2yXAGfhy/2kQVpzP1vO1Uej9nzU\n\tC5W3vV8tv2CRA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1666244795;\n\tbh=T3YtLW059JpYyLRUeHnIu3ovR8rzkrrA4y194kCsJwk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=U7DdANXH+o6t/DXZ0KuBP3ad7no2Eti8ihZWrH5fh+BG4GJBdjQEVKPTciTSEgk4V\n\taCtOOv2X1W0qj7ydf55r4BuJDhvBqakwQM9eE5WtqBKiL4V5iP35o3/LhdeN54Zm79\n\tFq9I+B96dGVQJpB2EpAxpqOnCVGwbxDijt4r5mG4="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"U7DdANXH\"; dkim-atps=neutral","Date":"Thu, 20 Oct 2022 14:46:27 +0900","To":"Laurent Pinchart ","Message-ID":"<20221020054627.GA3874866@pyrite.rasen.tech>","References":"<20221004021842.6345-1-laurent.pinchart@ideasonboard.com>\n\t<20221004021842.6345-2-laurent.pinchart@ideasonboard.com>\n\t<20221007150157.zgq3csc4nafkflyh@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: utils: Add Cleaner\n\tclass","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":"Paul Elder via libcamera-devel ","Reply-To":"paul.elder@ideasonboard.com","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]