From patchwork Sun Aug 4 15:31:05 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 20759 Return-Path: X-Original-To: parsemail@patchwork.libcamera.org Delivered-To: parsemail@patchwork.libcamera.org Received: from lancelot.ideasonboard.com (lancelot.ideasonboard.com [92.243.16.209]) by patchwork.libcamera.org (Postfix) with ESMTPS id C5AC0C323E for ; Sun, 4 Aug 2024 15:31:33 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 5E03563385; Sun, 4 Aug 2024 17:31:31 +0200 (CEST) Authentication-Results: lancelot.ideasonboard.com; dkim=pass (1024-bit key; unprotected) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="cdbK7NBo"; dkim-atps=neutral Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 41A526195E for ; Sun, 4 Aug 2024 17:31:30 +0200 (CEST) Received: from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi [81.175.209.231]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 65B121BA for ; Sun, 4 Aug 2024 17:30:39 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1722785439; bh=R+GOanGW5/bMQAkPJJK1gUQdEya0hpEBmmdRoccSpv8=; h=From:To:Subject:Date:In-Reply-To:References:From; b=cdbK7NBoX43Pg76l93bOAIlDq5yjCxWndeaX3UoZsNFiqj/8028vcFgKudGbSy10m P6ZkhmZ6QAtY7hi6SA+63Y1Uok0NfS2rzoby489aDu6LQXA3C5q3AJ7pup7u08IizJ tyqN7BAvRYSf08QdUS4FPsKI8E4mis2PNffV/52M= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Subject: [PATCH v3 1/2] libcamera: utils: Add ScopeExitActions class Date: Sun, 4 Aug 2024 18:31:05 +0300 Message-ID: <20240804153106.22167-2-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.44.2 In-Reply-To: <20240804153106.22167-1-laurent.pinchart@ideasonboard.com> References: <20240804153106.22167-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" The ScopeExitActions class is a simple object that performs user-provided actions upon destruction. It is meant to simplify cleanup tasks in error handling paths. Signed-off-by: Laurent Pinchart Reviewed-by: Xavier Roumegue Reviewed-by: Paul Elder Reviewed-by: Umang Jain --- Changes since v2: - Reaplace the last references to "defer" Changes since v1: - Rename to ScopeExitActions - Replace defer() with operator+=() - Replace clear() with release() --- include/libcamera/base/utils.h | 13 ++++ src/libcamera/base/utils.cpp | 132 +++++++++++++++++++++++++++++++++ 2 files changed, 145 insertions(+) diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h index 734ff81e2860..b87f9b0ade7a 100644 --- a/include/libcamera/base/utils.h +++ b/include/libcamera/base/utils.h @@ -9,6 +9,7 @@ #include #include +#include #include #include #include @@ -399,6 +400,18 @@ constexpr std::underlying_type_t to_underlying(Enum e) noexcept return static_cast>(e); } +class ScopeExitActions +{ +public: + ~ScopeExitActions(); + + void operator+=(std::function &&action); + void release(); + +private: + std::vector> actions_; +}; + } /* namespace utils */ #ifndef __DOXYGEN__ diff --git a/src/libcamera/base/utils.cpp b/src/libcamera/base/utils.cpp index ccb31063b922..67e5a8964680 100644 --- a/src/libcamera/base/utils.cpp +++ b/src/libcamera/base/utils.cpp @@ -531,6 +531,138 @@ double strtod(const char *__restrict nptr, char **__restrict endptr) * \return The value of e converted to its underlying type */ +/** + * \class ScopeExitActions + * \brief An object that performs actions upon destruction + * + * The ScopeExitActions class is a simple object that performs user-provided + * actions upon destruction. It is meant to simplify cleanup tasks in error + * handling paths. + * + * When the code flow performs multiple sequential actions that each need a + * corresponding cleanup action, error handling quickly become tedious: + * + * \code{.cpp} + * { + * int ret = allocateMemory(); + * if (ret) + * return ret; + * + * ret = startProducer(); + * if (ret) { + * freeMemory(); + * return ret; + * } + * + * ret = startConsumer(); + * if (ret) { + * stopProducer(); + * freeMemory(); + * return ret; + * } + * + * return 0; + * } + * \endcode + * + * This is prone to programming mistakes, as cleanup actions can easily be + * forgotten or ordered incorrectly. One strategy to simplify error handling is + * to use goto statements: + * + * \code{.cpp} + * { + * int ret = allocateMemory(); + * if (ret) + * return ret; + * + * ret = startProducer(); + * if (ret) + * goto error_free; + * + * ret = startConsumer(); + * if (ret) + * goto error_stop; + * + * return 0; + * + * error_stop: + * stopProducer(); + * error_free: + * freeMemory(); + * return ret; + * } + * \endcode + * + * While this may be considered better, this solution is still quite + * error-prone. Beside the risk of picking the wrong error label, the error + * handling logic is separated from the normal code flow, which increases the + * risk of error when refactoring the code. Additionally, C++ doesn't allow + * goto statements to jump over local variable declarations, which can make + * usage of this pattern more difficult. + * + * The ScopeExitActions class solves these issues by allowing code that + * requires cleanup actions to be grouped with its corresponding error handling + * code: + * + * \code{.cpp} + * { + * ScopeExitActions actions; + * + * int ret = allocateMemory(); + * if (ret) + * return ret; + * + * actions += [&]() { freeMemory(); }; + * + * ret = startProducer(); + * if (ret) + * return ret; + * + * actions += [&]() { stopProducer(); }; + * + * ret = startConsumer(); + * if (ret) + * return ret; + * + * actions.release(); + * return 0; + * } + * \endcode + * + * Error handlers are executed when the ScopeExitActions instance is destroyed, + * in the reverse order of their addition. + */ + +ScopeExitActions::~ScopeExitActions() +{ + for (const auto &action : utils::reverse(actions_)) + action(); +} + +/** + * \brief Add an exit action + * \param[in] action The action + * + * Add an exit action to the ScopeExitActions. Actions will be called upon + * destruction in the reverse order of their addition. + */ +void ScopeExitActions::operator+=(std::function &&action) +{ + actions_.push_back(std::move(action)); +} + +/** + * \brief Remove all exit actions + * + * This function should be called in scope exit paths that don't need the + * actions to be executed, such as success return paths from a function when + * the ScopeExitActions is used for error cleanup. + */ +void ScopeExitActions::release() +{ + actions_.clear(); +} + } /* namespace utils */ #ifndef __DOXYGEN__ From patchwork Sun Aug 4 15:31:06 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 20760 Return-Path: X-Original-To: parsemail@patchwork.libcamera.org Delivered-To: parsemail@patchwork.libcamera.org Received: from lancelot.ideasonboard.com (lancelot.ideasonboard.com [92.243.16.209]) by patchwork.libcamera.org (Postfix) with ESMTPS id A8C68C323E for ; Sun, 4 Aug 2024 15:31:36 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 5F4AA63381; Sun, 4 Aug 2024 17:31:36 +0200 (CEST) Authentication-Results: lancelot.ideasonboard.com; dkim=pass (1024-bit key; unprotected) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="lw1FdZ9J"; dkim-atps=neutral Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 048DA63383 for ; Sun, 4 Aug 2024 17:31:31 +0200 (CEST) Received: from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi [81.175.209.231]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id D0D531BA for ; Sun, 4 Aug 2024 17:30:40 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1722785441; bh=wIACAUXXXLbrXeOiDyZ7kanLNWrKtlHp0IjmV9DANuc=; h=From:To:Subject:Date:In-Reply-To:References:From; b=lw1FdZ9JGC3r3t2WF6tPPkaLTdf9oZ/c8Vjcw88RMuZLsPoOXy8YsFxxdBwNdH/l2 LGEdIfkNV7ly7MRfr6/c7x7QVm4Oak6ykRMM2knRptaIbu6I20tnZsH/HTsAnMAHZk C7EJ58f38j2CXOZetguzIF+4AoI1w+fYHmBYqDc4= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Subject: [PATCH v3 2/2] pipeline: rkisp1: Use ScopeExitActions to simplify error handling in start Date: Sun, 4 Aug 2024 18:31:06 +0300 Message-ID: <20240804153106.22167-3-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.44.2 In-Reply-To: <20240804153106.22167-1-laurent.pinchart@ideasonboard.com> References: <20240804153106.22167-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" Error handling in the PipelineHandlerRkISP1::start() function is cumbersome. Simplify it using the utils::ScopeExitActions class. Signed-off-by: Laurent Pinchart Reviewed-by: Xavier Roumegue Reviewed-by: Paul Elder Reviewed-by: Umang Jain --- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 31 +++++++++--------------- 1 file changed, 11 insertions(+), 20 deletions(-) diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp index eec5bf949bed..f7ca7025f95b 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp @@ -915,71 +915,62 @@ int PipelineHandlerRkISP1::freeBuffers(Camera *camera) int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlList *controls) { RkISP1CameraData *data = cameraData(camera); + utils::ScopeExitActions actions; int ret; /* Allocate buffers for internal pipeline usage. */ ret = allocateBuffers(camera); if (ret) return ret; + actions += [&]() { freeBuffers(camera); }; ret = data->ipa_->start(); if (ret) { - freeBuffers(camera); LOG(RkISP1, Error) << "Failed to start IPA " << camera->id(); return ret; } + actions += [&]() { data->ipa_->stop(); }; data->frame_ = 0; if (!isRaw_) { ret = param_->streamOn(); if (ret) { - data->ipa_->stop(); - freeBuffers(camera); LOG(RkISP1, Error) << "Failed to start parameters " << camera->id(); return ret; } + actions += [&]() { param_->streamOff(); }; ret = stat_->streamOn(); if (ret) { - param_->streamOff(); - data->ipa_->stop(); - freeBuffers(camera); LOG(RkISP1, Error) << "Failed to start statistics " << camera->id(); return ret; } + actions += [&]() { stat_->streamOff(); }; } if (data->mainPath_->isEnabled()) { ret = mainPath_.start(); - if (ret) { - param_->streamOff(); - stat_->streamOff(); - data->ipa_->stop(); - freeBuffers(camera); + if (ret) return ret; - } + actions += [&]() { mainPath_.stop(); }; } if (hasSelfPath_ && data->selfPath_->isEnabled()) { ret = selfPath_.start(); - if (ret) { - mainPath_.stop(); - param_->streamOff(); - stat_->streamOff(); - data->ipa_->stop(); - freeBuffers(camera); + if (ret) return ret; - } } isp_->setFrameStartEnabled(true); activeCamera_ = camera; - return ret; + + actions.release(); + return 0; } void PipelineHandlerRkISP1::stopDevice(Camera *camera)