[{"id":12533,"web_url":"https://patchwork.libcamera.org/comment/12533/","msgid":"<20200916000102.GE14954@pendragon.ideasonboard.com>","date":"2020-09-16T00:01:02","subject":"Re: [libcamera-devel] [PATCH 01/23] libcamera: ProcessManager: make\n\tProcessManager lifetime explicitly managed","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Tue, Sep 15, 2020 at 11:20:16PM +0900, Paul Elder wrote:\n> If any Process instances are destroyed after the ProcessManager is\n> destroyed, then a segfault will occur.\n> \n> Fix this by making the lifetime of the ProcessManager explicit, and make\n> the CameraManager construct and deconstruct (automatically, via a unique\n> pointer) the ProcessManager.\n\nI can't see a unique pointer. The code looks fine, only the commit\nmessage seems to need an update.\n\nReviewed-by: Laurent Pinchart \n\n> Signed-off-by: Paul Elder \n> ---\n> include/libcamera/internal/process.h | 27 ++++++++++++++++\n> src/libcamera/camera_manager.cpp | 2 ++\n> src/libcamera/process.cpp | 46 ++++++++++++----------------\n> 3 files changed, 48 insertions(+), 27 deletions(-)\n> \n> diff --git a/include/libcamera/internal/process.h b/include/libcamera/internal/process.h\n> index 36595106..df697c7f 100644\n> --- a/include/libcamera/internal/process.h\n> +++ b/include/libcamera/internal/process.h\n> @@ -7,6 +7,7 @@\n> #ifndef __LIBCAMERA_INTERNAL_PROCESS_H__\n> #define __LIBCAMERA_INTERNAL_PROCESS_H__\n> \n> +#include \n> #include \n> #include \n> \n> @@ -50,6 +51,32 @@ private:\n> \tfriend class ProcessManager;\n> };\n> \n> +class ProcessManager\n> +{\n> +public:\n> +\tProcessManager();\n> +\t~ProcessManager();\n> +\n> +\tvoid registerProcess(Process *proc);\n> +\n> +\tstatic ProcessManager *instance();\n> +\n> +\tint writePipe() const;\n> +\n> +\tconst struct sigaction &oldsa() const;\n> +\n> +private:\n> +\tstatic ProcessManager *self_;\n> +\n> +\tvoid sighandler(EventNotifier *notifier);\n> +\n> +\tstd::list processes_;\n> +\n> +\tstruct sigaction oldsa_;\n> +\tEventNotifier *sigEvent_;\n> +\tint pipe_[2];\n> +};\n> +\n> } /* namespace libcamera */\n> \n> #endif /* __LIBCAMERA_INTERNAL_PROCESS_H__ */\n> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp\n> index 47d56256..b8d3ccc8 100644\n> --- a/src/libcamera/camera_manager.cpp\n> +++ b/src/libcamera/camera_manager.cpp\n> @@ -18,6 +18,7 @@\n> #include \"libcamera/internal/ipa_manager.h\"\n> #include \"libcamera/internal/log.h\"\n> #include \"libcamera/internal/pipeline_handler.h\"\n> +#include \"libcamera/internal/process.h\"\n> #include \"libcamera/internal/thread.h\"\n> #include \"libcamera/internal/utils.h\"\n> \n> @@ -67,6 +68,7 @@ private:\n> \tstd::unique_ptr enumerator_;\n> \n> \tIPAManager ipaManager_;\n> +\tProcessManager processManager_;\n> };\n> \n> CameraManager::Private::Private(CameraManager *cm)\n> diff --git a/src/libcamera/process.cpp b/src/libcamera/process.cpp\n> index 994190dc..72b5afe2 100644\n> --- a/src/libcamera/process.cpp\n> +++ b/src/libcamera/process.cpp\n> @@ -41,28 +41,6 @@ LOG_DEFINE_CATEGORY(Process)\n> * The ProcessManager singleton keeps track of all created Process instances,\n> * and manages the signal handling involved in terminating processes.\n> */\n> -class ProcessManager\n> -{\n> -public:\n> -\tvoid registerProcess(Process *proc);\n> -\n> -\tstatic ProcessManager *instance();\n> -\n> -\tint writePipe() const;\n> -\n> -\tconst struct sigaction &oldsa() const;\n> -\n> -private:\n> -\tvoid sighandler(EventNotifier *notifier);\n> -\tProcessManager();\n> -\t~ProcessManager();\n> -\n> -\tstd::list processes_;\n> -\n> -\tstruct sigaction oldsa_;\n> -\tEventNotifier *sigEvent_;\n> -\tint pipe_[2];\n> -};\n> \n> namespace {\n> \n> @@ -127,8 +105,20 @@ void ProcessManager::registerProcess(Process *proc)\n> \tprocesses_.push_back(proc);\n> }\n> \n> +ProcessManager *ProcessManager::self_ = nullptr;\n> +\n> +/**\n> + * \\brief Construct a ProcessManager instance\n> + *\n> + * The ProcessManager class is meant to only be instantiated once, by the\n> + * CameraManager.\n> + */\n> ProcessManager::ProcessManager()\n> {\n> +\tif (self_)\n> +\t\tLOG(Process, Fatal)\n> +\t\t\t<< \"Multiple ProcessManager objects are not allowed\";\n> +\n> \tsigaction(SIGCHLD, NULL, &oldsa_);\n> \n> \tstruct sigaction sa;\n> @@ -145,6 +135,8 @@ ProcessManager::ProcessManager()\n> \t\t\t<< \"Failed to initialize pipe for signal handling\";\n> \tsigEvent_ = new EventNotifier(pipe_[0], EventNotifier::Read);\n> \tsigEvent_->activated.connect(this, &ProcessManager::sighandler);\n> +\n> +\tself_ = this;\n> }\n> \n> ProcessManager::~ProcessManager()\n> @@ -153,21 +145,21 @@ ProcessManager::~ProcessManager()\n> \tdelete sigEvent_;\n> \tclose(pipe_[0]);\n> \tclose(pipe_[1]);\n> +\n> +\tself_ = nullptr;\n> }\n> \n> /**\n> * \\brief Retrieve the Process manager instance\n> *\n> - * The ProcessManager is a singleton and can't be constructed manually. This\n> - * method shall instead be used to retrieve the single global instance of the\n> - * manager.\n> + * The ProcessManager is constructed by the CameraManager. This function shall\n> + * be used to retrieve the single instance of the manager.\n> *\n> * \\return The Process manager instance\n> */\n> ProcessManager *ProcessManager::instance()\n> {\n> -\tstatic ProcessManager processManager;\n> -\treturn &processManager;\n> +\treturn self_;\n> }\n> \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 CE1A3C3B5B\n\tfor ;\n\tWed, 16 Sep 2020 00:01:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2E25D62E13;\n\tWed, 16 Sep 2020 02:01:34 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DF99562C8C\n\tfor ;\n\tWed, 16 Sep 2020 02:01:32 +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 67D16FD8;\n\tWed, 16 Sep 2020 02:01:32 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"KYuTVznH\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1600214492;\n\tbh=vN/V9X/fk4eJ5PbRSOYARO5kzrD6eoepNZf6jgSRZqI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=KYuTVznHijiXx4yZi+q9ooLQX2JR6qjX35BUZ9J6uNA3fWn5lBQLJB4K/QPWv5Me1\n\tClts2LeGk3YyC9mtuoGKAu5uFCowH0RzO6RKfeGbWam6UnYVoLNs/fiu6j3sntAy/5\n\tHxAz7W+QN7cfWUsGuZli1iJ98IeXjrBjMd5dsI3g=","Date":"Wed, 16 Sep 2020 03:01:02 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Message-ID":"<20200916000102.GE14954@pendragon.ideasonboard.com>","References":"<20200915142038.28757-1-paul.elder@ideasonboard.com>\n\t<20200915142038.28757-2-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200915142038.28757-2-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 01/23] libcamera: ProcessManager: make\n\tProcessManager lifetime explicitly managed","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12556,"web_url":"https://patchwork.libcamera.org/comment/12556/","msgid":"<20200916161039.GL1850958@oden.dyn.berto.se>","date":"2020-09-16T16:10:39","subject":"Re: [libcamera-devel] [PATCH 01/23] libcamera: ProcessManager: make\n\tProcessManager lifetime explicitly managed","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Paul,\n\nThanks for your work.\n\nOn 2020-09-15 23:20:16 +0900, Paul Elder wrote:\n> If any Process instances are destroyed after the ProcessManager is\n> destroyed, then a segfault will occur.\n> \n> Fix this by making the lifetime of the ProcessManager explicit, and make\n> the CameraManager construct and deconstruct (automatically, via a unique\n> pointer) the ProcessManager.\n> \n> Signed-off-by: Paul Elder \n\nWith the comment about commit message pointed out by Laurent fixed,\n\nReviewed-by: Niklas Söderlund \n\n> ---\n> include/libcamera/internal/process.h | 27 ++++++++++++++++\n> src/libcamera/camera_manager.cpp | 2 ++\n> src/libcamera/process.cpp | 46 ++++++++++++----------------\n> 3 files changed, 48 insertions(+), 27 deletions(-)\n> \n> diff --git a/include/libcamera/internal/process.h b/include/libcamera/internal/process.h\n> index 36595106..df697c7f 100644\n> --- a/include/libcamera/internal/process.h\n> +++ b/include/libcamera/internal/process.h\n> @@ -7,6 +7,7 @@\n> #ifndef __LIBCAMERA_INTERNAL_PROCESS_H__\n> #define __LIBCAMERA_INTERNAL_PROCESS_H__\n> \n> +#include \n> #include \n> #include \n> \n> @@ -50,6 +51,32 @@ private:\n> \tfriend class ProcessManager;\n> };\n> \n> +class ProcessManager\n> +{\n> +public:\n> +\tProcessManager();\n> +\t~ProcessManager();\n> +\n> +\tvoid registerProcess(Process *proc);\n> +\n> +\tstatic ProcessManager *instance();\n> +\n> +\tint writePipe() const;\n> +\n> +\tconst struct sigaction &oldsa() const;\n> +\n> +private:\n> +\tstatic ProcessManager *self_;\n> +\n> +\tvoid sighandler(EventNotifier *notifier);\n> +\n> +\tstd::list processes_;\n> +\n> +\tstruct sigaction oldsa_;\n> +\tEventNotifier *sigEvent_;\n> +\tint pipe_[2];\n> +};\n> +\n> } /* namespace libcamera */\n> \n> #endif /* __LIBCAMERA_INTERNAL_PROCESS_H__ */\n> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp\n> index 47d56256..b8d3ccc8 100644\n> --- a/src/libcamera/camera_manager.cpp\n> +++ b/src/libcamera/camera_manager.cpp\n> @@ -18,6 +18,7 @@\n> #include \"libcamera/internal/ipa_manager.h\"\n> #include \"libcamera/internal/log.h\"\n> #include \"libcamera/internal/pipeline_handler.h\"\n> +#include \"libcamera/internal/process.h\"\n> #include \"libcamera/internal/thread.h\"\n> #include \"libcamera/internal/utils.h\"\n> \n> @@ -67,6 +68,7 @@ private:\n> \tstd::unique_ptr enumerator_;\n> \n> \tIPAManager ipaManager_;\n> +\tProcessManager processManager_;\n> };\n> \n> CameraManager::Private::Private(CameraManager *cm)\n> diff --git a/src/libcamera/process.cpp b/src/libcamera/process.cpp\n> index 994190dc..72b5afe2 100644\n> --- a/src/libcamera/process.cpp\n> +++ b/src/libcamera/process.cpp\n> @@ -41,28 +41,6 @@ LOG_DEFINE_CATEGORY(Process)\n> * The ProcessManager singleton keeps track of all created Process instances,\n> * and manages the signal handling involved in terminating processes.\n> */\n> -class ProcessManager\n> -{\n> -public:\n> -\tvoid registerProcess(Process *proc);\n> -\n> -\tstatic ProcessManager *instance();\n> -\n> -\tint writePipe() const;\n> -\n> -\tconst struct sigaction &oldsa() const;\n> -\n> -private:\n> -\tvoid sighandler(EventNotifier *notifier);\n> -\tProcessManager();\n> -\t~ProcessManager();\n> -\n> -\tstd::list processes_;\n> -\n> -\tstruct sigaction oldsa_;\n> -\tEventNotifier *sigEvent_;\n> -\tint pipe_[2];\n> -};\n> \n> namespace {\n> \n> @@ -127,8 +105,20 @@ void ProcessManager::registerProcess(Process *proc)\n> \tprocesses_.push_back(proc);\n> }\n> \n> +ProcessManager *ProcessManager::self_ = nullptr;\n> +\n> +/**\n> + * \\brief Construct a ProcessManager instance\n> + *\n> + * The ProcessManager class is meant to only be instantiated once, by the\n> + * CameraManager.\n> + */\n> ProcessManager::ProcessManager()\n> {\n> +\tif (self_)\n> +\t\tLOG(Process, Fatal)\n> +\t\t\t<< \"Multiple ProcessManager objects are not allowed\";\n> +\n> \tsigaction(SIGCHLD, NULL, &oldsa_);\n> \n> \tstruct sigaction sa;\n> @@ -145,6 +135,8 @@ ProcessManager::ProcessManager()\n> \t\t\t<< \"Failed to initialize pipe for signal handling\";\n> \tsigEvent_ = new EventNotifier(pipe_[0], EventNotifier::Read);\n> \tsigEvent_->activated.connect(this, &ProcessManager::sighandler);\n> +\n> +\tself_ = this;\n> }\n> \n> ProcessManager::~ProcessManager()\n> @@ -153,21 +145,21 @@ ProcessManager::~ProcessManager()\n> \tdelete sigEvent_;\n> \tclose(pipe_[0]);\n> \tclose(pipe_[1]);\n> +\n> +\tself_ = nullptr;\n> }\n> \n> /**\n> * \\brief Retrieve the Process manager instance\n> *\n> - * The ProcessManager is a singleton and can't be constructed manually. This\n> - * method shall instead be used to retrieve the single global instance of the\n> - * manager.\n> + * The ProcessManager is constructed by the CameraManager. This function shall\n> + * be used to retrieve the single instance of the manager.\n> *\n> * \\return The Process manager instance\n> */\n> ProcessManager *ProcessManager::instance()\n> {\n> -\tstatic ProcessManager processManager;\n> -\treturn &processManager;\n> +\treturn self_;\n> }\n> \n> /**\n> -- \n> 2.27.0\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 A36DEBF01C\n\tfor ;\n\tWed, 16 Sep 2020 16:10:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2FCCD62E75;\n\tWed, 16 Sep 2020 18:10:43 +0200 (CEST)","from mail-lf1-x141.google.com (mail-lf1-x141.google.com\n\t[IPv6:2a00:1450:4864:20::141])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1A10961E2B\n\tfor ;\n\tWed, 16 Sep 2020 18:10:41 +0200 (CEST)","by mail-lf1-x141.google.com with SMTP id y2so7548327lfy.10\n\tfor ;\n\tWed, 16 Sep 2020 09:10:41 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\ts20sm4673223lfs.135.2020.09.16.09.10.39\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 16 Sep 2020 09:10:39 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"nAiZ7R1U\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=/qsfcJeY3wQxDZWjPld4KY9VwS1JiPmzdGz3y6K2syE=;\n\tb=nAiZ7R1Uclh+BA7dyPyD2H3xqp5zCfqJYL3UCN+HeDSGvhVTkitm4S47Yhem6Qsrba\n\t0LIXICdNwaOSCtiyFbCVQjTsbXb/duWM5lhyLVx/xN+amtR0myW8m7rAEGTMmm9fEIPB\n\tUrWkm0v+rpBYJmhPbbaawfiWRZOtXr7uNA+xKE5G4EuB2++mCWszQec1lKyWvPZI3oP0\n\tWPQ2MkeXgh2WDzuXlPasEvKTIUsXE7gvxjy/QeGShc4/qVoTbX/8WG5fu7uiAgrga9a0\n\tOS6IDfrDO6Q9sMUrP8L0oPIn1rvz6fMKlfiiAaTMn2X6Fju2RITJFrnAmRDaaCvmKUXb\n\thUDQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=/qsfcJeY3wQxDZWjPld4KY9VwS1JiPmzdGz3y6K2syE=;\n\tb=TKJLmZo/zLxn6qqL6qxw+IBfSTwgd3Ne8kEdH/dNkuDNX65PNLKHmvm3HVYKy4efWY\n\tBWafhSDrOT7hEYnG+SU3wtSNcKwq4BFzNTrlScxYTVPD+CV8rP9gUI+5iR4FI6bzNJoE\n\tqL2etfw/2HWM2z1YlX91HBk2yBB3BDSPPbTFhPHUzwBRbjlTFSe514ro5ca0sS8iR/da\n\tPVAM7q7T6pd8nPpomiRtEbc9qqtcunLX/nMyufITPZySpxJtw889OxVWVo1nbRhdHRv9\n\tBAlDKLVLD+Wh0AX3Bun4864iPnU6epR5UyVp+JfvDMBH01qcfIyTdStVJuSgSESEV3Sk\n\txdvQ==","X-Gm-Message-State":"AOAM5328vWOA+QpoGFIm9DPJ5bcyNcALbpXgf+cuMu2hj59tSbrOPBJP\n\tbJQEzaiW7wWqkkz+d8CcvyI9hQcAIGJgWw==","X-Google-Smtp-Source":"ABdhPJySlJevpDf+A99zbLq3y5ZzV3I+348gA4VGqgdxRvbEXbTe6af/xlM1adhs7SotSVgJ8+fQ1A==","X-Received":"by 2002:a19:48d2:: with SMTP id\n\tv201mr7548162lfa.96.1600272640328; \n\tWed, 16 Sep 2020 09:10:40 -0700 (PDT)","Date":"Wed, 16 Sep 2020 18:10:39 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Paul Elder ","Message-ID":"<20200916161039.GL1850958@oden.dyn.berto.se>","References":"<20200915142038.28757-1-paul.elder@ideasonboard.com>\n\t<20200915142038.28757-2-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200915142038.28757-2-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 01/23] libcamera: ProcessManager: make\n\tProcessManager lifetime explicitly managed","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]