From patchwork Tue Oct 4 02:18:40 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 17520 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 6B144BD16B for ; Tue, 4 Oct 2022 02:18:47 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 8655F60A7D; Tue, 4 Oct 2022 04:18:46 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org; s=mail; t=1664849926; bh=drx5bktiN07LT2venuVspMSEJM8klFbNTZ6lAMnBex8=; h=To:Date:Subject:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:From; b=x+U7O68OUNqmujA8sjH0xBb4WAllkFhnXc/VCP1Jszw981f+Jwla+ycwqts6sBaH7 x1WjijqiA4I+ZuiTYkerdCLzt+C83azMlUHcBrHllLi6Vc1m9MPG6ElYo/EdakkjVg NCerMqgm3zAymeO3VMfZG/1tKnLW4XcU17EBORCZfDqgG58s8zI9SCbuW88Fk5tmTR ZhukcFEgFFI/zn4Mpda55kt+brb71owGBYwChxShTXEr+dbdLFivi0P20R9jUw/Y4B 6jT4woG9T+umDZCpOB9rEHTcqHcj90dLjotyyyzEF0TtfbTUEcO76sT2AKnJ3EyKPu p1iwCIClyqGMw== 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 A4A97603F6 for ; Tue, 4 Oct 2022 04:18:45 +0200 (CEST) Authentication-Results: lancelot.ideasonboard.com; dkim=pass (1024-bit key; unprotected) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="bScxrRXV"; dkim-atps=neutral Received: from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi [62.78.145.57]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id F2A57517; Tue, 4 Oct 2022 04:18:44 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1664849925; bh=drx5bktiN07LT2venuVspMSEJM8klFbNTZ6lAMnBex8=; h=From:To:Cc:Subject:Date:From; b=bScxrRXVGqfFPE9jufbqkZPWlYGH3nH5AtxSjMXxqxf4Q3XKJXsUiZCIlDE7rfk8m gnTIla/jA996yJ+G9bgG8T8lJibblNaHB7oHfaqkBb0/No6C1QDl7QqsEcHy4vv42q 1aB+FOA8hMzSbbIOlhfsPmKaSVemb00oExpeUjyQ= To: libcamera-devel@lists.libcamera.org Date: Tue, 4 Oct 2022 05:18:40 +0300 Message-Id: <20221004021842.6345-1-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.35.1 MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH 0/2] libcamera: Simplify error handling with Cleaner class 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: , X-Patchwork-Original-From: Laurent Pinchart via libcamera-devel From: Laurent Pinchart Reply-To: Laurent Pinchart Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" Hello, This small patch series stems from my review of "[PATCH 13/14] libcamera: pipeline: rkisp1: Factorize errors handling path on start()". While I agreed that the current error handling pattern in PipelineHandlerRkISP1::start() is fairly bad, I wasn't thrilled by usage of goto statements to fix it. This is an alternative proposal. The Cleaner class introduced in patch 1/2 took multiple forms before reaching this one. I first experimented with a class that handled a single action and thus needed to be instantiated once per action. To achieve the equivalent of the Cleaner::clear() call I had to either add an 'if (ret)' statement to the lambda functions, or pass the ret variable to the constructor of the Cleaner. I found ideas for syntactic sugar in https://github.com/yuri-kilochek/boost.scope_guard, but in the end, neither option made me very happy. Handling multiple deferred cleanup actions with one Cleaner instance makes the user code (in 2/2) simpler. The "defer" name for the Cleaner::defer() function was influenced by an interesting C extension proposal (https://hal.inria.fr/hal-03059076/document). It uses the name "guard" for the conceptual equivalent of the Cleaner class, but that didn't convince me. I'm not sure to really like the "Cleaner" name though, alternative naming proposals are welcome. One point I'm not happy with is the need to call Cleaner::clear() in the success path. This is error-prone as it could be forgotten. We could switch to the opposite, with cleanups disabled by default and an explicit call in the error paths, but that could also be forgotten, resulting in missing cleanups in some error paths. The result would possibly be worse, as error paths are less tested. Another option would be to require explicit calls in both the success and error paths, and complain loudly in the destructor if none of those functions have been called. I'm not sure to be convinced it would be the best idea, although finding good names for the success/error functions may help. Alternative ideas are welcome too, as well as opinions regarding whether this is worth it or not. Laurent Pinchart (2): libcamera: utils: Add Cleaner class pipeline: rkisp1: Use utils::Cleaner to simplify error handling in start include/libcamera/base/utils.h | 13 +++ src/libcamera/base/utils.cpp | 130 +++++++++++++++++++++++ src/libcamera/pipeline/rkisp1/rkisp1.cpp | 31 ++---- 3 files changed, 154 insertions(+), 20 deletions(-)