[{"id":33700,"web_url":"https://patchwork.libcamera.org/comment/33700/","msgid":"<174292977704.2136573.9696533061048901069@ping.linuxembedded.co.uk>","date":"2025-03-25T19:09:37","subject":"Re: [RFC PATCH v3 7/9] libcamera: process: Move\n\t`closeAllFdsExcept()`","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Barnabás Pőcze (2025-03-25 18:08:19)\n> Remove `closeAllFdsExcept()` from the `Process` class and have\n> it as a local function since it is no needed \"outside\" and does\n> not depend on any part of the `Process` class.\n\nAny impact on https://patchwork.libcamera.org/patch/22403/ ? Could you\nreview that please?\n\n> \n> Signed-off-by: Barnabás Pőcze \n> ---\n> include/libcamera/internal/process.h | 1 -\n> src/libcamera/process.cpp | 56 ++++++++++++++--------------\n> 2 files changed, 28 insertions(+), 29 deletions(-)\n> \n> diff --git a/include/libcamera/internal/process.h b/include/libcamera/internal/process.h\n> index e4f5bb979..14391d60a 100644\n> --- a/include/libcamera/internal/process.h\n> +++ b/include/libcamera/internal/process.h\n> @@ -44,7 +44,6 @@ public:\n> private:\n> LIBCAMERA_DISABLE_COPY_AND_MOVE(Process)\n> \n> - void closeAllFdsExcept(Span fds);\n\nIt's already private though isn't it ? \n\nIs it just better to keep functions isolated if they aren't directly\nusing the class ?\n\n> int isolate();\n> void died(int wstatus);\n> \n> diff --git a/src/libcamera/process.cpp b/src/libcamera/process.cpp\n> index 5603dc903..01503e485 100644\n> --- a/src/libcamera/process.cpp\n> +++ b/src/libcamera/process.cpp\n> @@ -63,6 +63,34 @@ void sigact(int signal, siginfo_t *info, void *ucontext)\n> }\n> }\n> \n> +void closeAllFdsExcept(Span fds)\n\nstatic? or is this already in an annoymous namespace or such ?\n\n> +{\n> + std::vector v(fds.begin(), fds.end());\n> + std::sort(v.begin(), v.end());\n> +\n> + ASSERT(v.empty() || v.front() >= 0);\n> +\n> + DIR *dir = opendir(\"/proc/self/fd\");\n> + if (!dir)\n> + return;\n> +\n> + int dfd = dirfd(dir);\n> +\n> + struct dirent *ent;\n> + while ((ent = readdir(dir)) != nullptr) {\n> + char *endp;\n> + int fd = strtoul(ent->d_name, &endp, 10);\n> + if (*endp)\n> + continue;\n> +\n> + if (fd >= 0 && fd != dfd &&\n> + !std::binary_search(v.begin(), v.end(), fd))\n> + close(fd);\n> + }\n> +\n> + closedir(dir);\n> +}\n> +\n> } /* namespace */\n> \n> void ProcessManager::sighandler()\n> @@ -282,34 +310,6 @@ int Process::start(const std::string &path,\n> }\n> }\n> \n> -void Process::closeAllFdsExcept(Span fds)\n> -{\n> - std::vector v(fds.begin(), fds.end());\n> - sort(v.begin(), v.end());\n> -\n> - ASSERT(v.empty() || v.front() >= 0);\n> -\n> - DIR *dir = opendir(\"/proc/self/fd\");\n> - if (!dir)\n> - return;\n> -\n> - int dfd = dirfd(dir);\n> -\n> - struct dirent *ent;\n> - while ((ent = readdir(dir)) != nullptr) {\n> - char *endp;\n> - int fd = strtoul(ent->d_name, &endp, 10);\n> - if (*endp)\n> - continue;\n> -\n> - if (fd >= 0 && fd != dfd &&\n> - !std::binary_search(v.begin(), v.end(), fd))\n> - close(fd);\n> - }\n> -\n> - closedir(dir);\n> -}\n> -\n> int Process::isolate()\n> {\n> int ret = unshare(CLONE_NEWUSER | CLONE_NEWNET);\n> -- \n> 2.49.0\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 86CCDC323E\n\tfor ;\n\tTue, 25 Mar 2025 19:09:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BBC2A68952;\n\tTue, 25 Mar 2025 20:09:41 +0100 (CET)","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 3D27D600EB\n\tfor ;\n\tTue, 25 Mar 2025 20:09:40 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 95090353;\n\tTue, 25 Mar 2025 20:07:52 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"PguIUgo9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1742929672;\n\tbh=jqWtUoALlJhI++w3VCzxbdmCJ97VWNtg0pak1uC7KNw=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=PguIUgo9Xxkzk6qLVVXmtZYm4nZCpEMS+D0aqirMs0aOv4b9h1PnoT39va9mlOlqL\n\tjXJ53nLrJPR1Zv0X/jVWiIRrWtbpEjDxhtM+sCwuZhUFMW+6/k9q6OAnJNprrTzGQM\n\t8Ak/x9e0lvd/jGPd0dkBmKKuFEpSqZ0h4CzcXN9o=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250325180821.1456154-8-barnabas.pocze@ideasonboard.com>","References":"<20250325180821.1456154-1-barnabas.pocze@ideasonboard.com>\n\t<20250325180821.1456154-8-barnabas.pocze@ideasonboard.com>","Subject":"Re: [RFC PATCH v3 7/9] libcamera: process: Move\n\t`closeAllFdsExcept()`","From":"Kieran Bingham ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 25 Mar 2025 19:09:37 +0000","Message-ID":"<174292977704.2136573.9696533061048901069@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":33702,"web_url":"https://patchwork.libcamera.org/comment/33702/","msgid":"<174292993145.2136573.11115614519892446709@ping.linuxembedded.co.uk>","date":"2025-03-25T19:12:11","subject":"Re: [RFC PATCH v3 2/9] libcamera: process: Misc. cleanup around\n\t`execv()`","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Barnabás Pőcze (2025-03-25 18:08:14)\n> Firstly, get the number of arguments first, and use that to determine the\n> size of the allocation instead of retrieving it twice.\n> \n> Secondly, use `const_cast` instead of a C-style cast when calling `execv()`.\n> \n> Third, use `size_t` to match the type of `args.size()`.\n> \n> Signed-off-by: Barnabás Pőcze \n> ---\n> src/libcamera/process.cpp | 9 +++++----\n> 1 file changed, 5 insertions(+), 4 deletions(-)\n> \n> diff --git a/src/libcamera/process.cpp b/src/libcamera/process.cpp\n> index 68fad3270..567b878d4 100644\n> --- a/src/libcamera/process.cpp\n> +++ b/src/libcamera/process.cpp\n> @@ -265,14 +265,15 @@ int Process::start(const std::string &path,\n> if (file && strcmp(file, \"syslog\"))\n> unsetenv(\"LIBCAMERA_LOG_FILE\");\n> \n> - const char **argv = new const char *[args.size() + 2];\n> - unsigned int len = args.size();\n> + size_t len = args.size();\n> + auto argv = std::make_unique(len + 2);\n\nThere's also a unique_ptr instaed of a 'new'. Will that have any side\neffect at the execv ? I assume not ...\n\n\nReviewed-by: Kieran Bingham \n\n> +\n> argv[0] = path.c_str();\n> - for (unsigned int i = 0; i < len; i++)\n> + for (size_t i = 0; i < len; i++)\n> argv[i + 1] = args[i].c_str();\n> argv[len + 1] = nullptr;\n> \n> - execv(path.c_str(), (char **)argv);\n> + execv(path.c_str(), const_cast(argv.get()));\n> \n> exit(EXIT_FAILURE);\n> }\n> -- \n> 2.49.0\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 D0A20C3213\n\tfor ;\n\tTue, 25 Mar 2025 19:12:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2492268956;\n\tTue, 25 Mar 2025 20:12:16 +0100 (CET)","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 C8224600EB\n\tfor ;\n\tTue, 25 Mar 2025 20:12:14 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 657B5353;\n\tTue, 25 Mar 2025 20:10:27 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"pv4SO8Q+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1742929827;\n\tbh=b4r2nOrwoYhI7Yhc5iuhKMYmRdHhDqRAPYTvkgD8aPQ=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=pv4SO8Q+uuhgFgWjrsbmHTmgBlta9Fb6Sy5kW3zsEjyQvaMbGDf5e4XxoVcBegvRE\n\tansiOVVB5wolCW0lJmnF+dqJsbrZoiRWlb7XNEDcOYu+NUlXvh6gPvol7bTZRtjHB4\n\tDSSb5d7Exi43ST3WOD5q1T2VVOdowdk9KFO8dsy8=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250325180821.1456154-3-barnabas.pocze@ideasonboard.com>","References":"<20250325180821.1456154-1-barnabas.pocze@ideasonboard.com>\n\t<20250325180821.1456154-3-barnabas.pocze@ideasonboard.com>","Subject":"Re: [RFC PATCH v3 2/9] libcamera: process: Misc. cleanup around\n\t`execv()`","From":"Kieran Bingham ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 25 Mar 2025 19:12:11 +0000","Message-ID":"<174292993145.2136573.11115614519892446709@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":33703,"web_url":"https://patchwork.libcamera.org/comment/33703/","msgid":"<174292997419.2136573.3597696961248152709@ping.linuxembedded.co.uk>","date":"2025-03-25T19:12:54","subject":"Re: [RFC PATCH v3 3/9] libcamera: process: Use `pid_` member to\n\tdecide if running","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Barnabás Pőcze (2025-03-25 18:08:15)\n> Instead of using a separate member variable, use `pid_ > 0` to determine\n> if the process is still running. Previously the value of `pid_` was not\n> reset to -1 when the process terminated, but since it is only meaningful\n> while the process is running, reset it to -1 in `Process::died()`.\n> \n> Neither `pid_` nor `running_` are exposed, so this change has no effect\n> on the public interface or observable behaviour.\n> \n> Signed-off-by: Barnabás Pőcze \n\nReviewed-by: Kieran Bingham \n\n> ---\n> include/libcamera/internal/process.h | 1 -\n> src/libcamera/process.cpp | 8 +++-----\n> 2 files changed, 3 insertions(+), 6 deletions(-)\n> \n> diff --git a/include/libcamera/internal/process.h b/include/libcamera/internal/process.h\n> index 58e3e41ce..030a1e66e 100644\n> --- a/include/libcamera/internal/process.h\n> +++ b/include/libcamera/internal/process.h\n> @@ -49,7 +49,6 @@ private:\n> void died(int wstatus);\n> \n> pid_t pid_;\n> - bool running_;\n> enum ExitStatus exitStatus_;\n> int exitCode_;\n> \n> diff --git a/src/libcamera/process.cpp b/src/libcamera/process.cpp\n> index 567b878d4..aa9e8f519 100644\n> --- a/src/libcamera/process.cpp\n> +++ b/src/libcamera/process.cpp\n> @@ -208,7 +208,7 @@ const struct sigaction &ProcessManager::oldsa() const\n> */\n> \n> Process::Process()\n> - : pid_(-1), running_(false), exitStatus_(NotExited), exitCode_(0)\n> + : pid_(-1), exitStatus_(NotExited), exitCode_(0)\n> {\n> }\n> \n> @@ -240,7 +240,7 @@ int Process::start(const std::string &path,\n> {\n> int ret;\n> \n> - if (running_)\n> + if (pid_ > 0)\n> return 0;\n> \n> int childPid = fork();\n> @@ -252,8 +252,6 @@ int Process::start(const std::string &path,\n> pid_ = childPid;\n> ProcessManager::instance()->registerProcess(this);\n> \n> - running_ = true;\n> -\n> return 0;\n> } else {\n> if (isolate())\n> @@ -327,7 +325,7 @@ int Process::isolate()\n> */\n> void Process::died(int wstatus)\n> {\n> - running_ = false;\n> + pid_ = -1;\n> exitStatus_ = WIFEXITED(wstatus) ? NormalExit : SignalExit;\n> exitCode_ = exitStatus_ == NormalExit ? WEXITSTATUS(wstatus) : -1;\n> \n> -- \n> 2.49.0\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 3F106C3213\n\tfor ;\n\tTue, 25 Mar 2025 19:12:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DD37C68956;\n\tTue, 25 Mar 2025 20:12:57 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 029AC600EB\n\tfor ;\n\tTue, 25 Mar 2025 20:12:57 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B0BAD353;\n\tTue, 25 Mar 2025 20:11:09 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"XLjN9uVZ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1742929869;\n\tbh=gtiaBdG9xKbKc+ESu4ThdYxYj8+H0dhTEBnOuuokYT4=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=XLjN9uVZ+oU+dAsDi4jZoMLE503wQVAa6MV4r7NTGjuJ56le4met5Zwe9z5xVdccK\n\tz6mENHeum4Igtd3b9IsDr2IE8eBNEmwF1nHq+5EDuNIIGz6/iNawLnIbg3R8hICm+z\n\tcaxOYeSHC9IBdbQw4U1CKkjqPo0+XqN6XwI/AOLs=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250325180821.1456154-4-barnabas.pocze@ideasonboard.com>","References":"<20250325180821.1456154-1-barnabas.pocze@ideasonboard.com>\n\t<20250325180821.1456154-4-barnabas.pocze@ideasonboard.com>","Subject":"Re: [RFC PATCH v3 3/9] libcamera: process: Use `pid_` member to\n\tdecide if running","From":"Kieran Bingham ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 25 Mar 2025 19:12:54 +0000","Message-ID":"<174292997419.2136573.3597696961248152709@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":33726,"web_url":"https://patchwork.libcamera.org/comment/33726/","msgid":"<20250326142751.GG8303@pendragon.ideasonboard.com>","date":"2025-03-26T14:27:51","subject":"Re: [RFC PATCH v3 7/9] libcamera: process: Move\n\t`closeAllFdsExcept()`","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Tue, Mar 25, 2025 at 07:09:37PM +0000, Kieran Bingham wrote:\n> Quoting Barnabás Pőcze (2025-03-25 18:08:19)\n> > Remove `closeAllFdsExcept()` from the `Process` class and have\n> > it as a local function since it is no needed \"outside\" and does\n> > not depend on any part of the `Process` class.\n> \n> Any impact on https://patchwork.libcamera.org/patch/22403/ ? Could you\n> review that please?\n> \n> > \n> > Signed-off-by: Barnabás Pőcze \n> > ---\n> > include/libcamera/internal/process.h | 1 -\n> > src/libcamera/process.cpp | 56 ++++++++++++++--------------\n> > 2 files changed, 28 insertions(+), 29 deletions(-)\n> > \n> > diff --git a/include/libcamera/internal/process.h b/include/libcamera/internal/process.h\n> > index e4f5bb979..14391d60a 100644\n> > --- a/include/libcamera/internal/process.h\n> > +++ b/include/libcamera/internal/process.h\n> > @@ -44,7 +44,6 @@ public:\n> > private:\n> > LIBCAMERA_DISABLE_COPY_AND_MOVE(Process)\n> > \n> > - void closeAllFdsExcept(Span fds);\n> \n> It's already private though isn't it ? \n> \n> Is it just better to keep functions isolated if they aren't directly\n> using the class ?\n\nThe rationale for not including members in the class definition, even as\nprivate members, is two-fold:\n\n- It avoids ABI breakages\n\n- It avoids recompilation of users of the class\n\nThe former is only applicable to member data, and member virtual\nfunctions. Adding, removing, or changing the signature of a non-virtual\nmember function doesn't affect the ABI.\n\nThe latter is true in all cases, but the practical impact is limited,\nunless you routinely make modifications to a class whose header file is\nwidely included.\n\nThe downsides of avoiding private member functions is a decrease (in my\nopinion) of readability. Making them non-member static functions\npossibly requires passing member data to the function. It also decouples\nthe function from the class, which can make the code harder to follow.\nIn this specific case the function didn't use any member data, and the\nreadability isn't very negatively impacted. I am therefore not opposed\nto this patch, even if I probably wouldn't have authored it. I think\nit's partly a matter of personal preference.\n\n> > int isolate();\n> > void died(int wstatus);\n> > \n> > diff --git a/src/libcamera/process.cpp b/src/libcamera/process.cpp\n> > index 5603dc903..01503e485 100644\n> > --- a/src/libcamera/process.cpp\n> > +++ b/src/libcamera/process.cpp\n> > @@ -63,6 +63,34 @@ void sigact(int signal, siginfo_t *info, void *ucontext)\n> > }\n> > }\n> > \n> > +void closeAllFdsExcept(Span fds)\n> \n> static? or is this already in an annoymous namespace or such ?\n\nIt's already in an anonymous namespace.\n\nReviewed-by: Laurent Pinchart \n\n> > +{\n> > + std::vector v(fds.begin(), fds.end());\n> > + std::sort(v.begin(), v.end());\n> > +\n> > + ASSERT(v.empty() || v.front() >= 0);\n> > +\n> > + DIR *dir = opendir(\"/proc/self/fd\");\n> > + if (!dir)\n> > + return;\n> > +\n> > + int dfd = dirfd(dir);\n> > +\n> > + struct dirent *ent;\n> > + while ((ent = readdir(dir)) != nullptr) {\n> > + char *endp;\n> > + int fd = strtoul(ent->d_name, &endp, 10);\n> > + if (*endp)\n> > + continue;\n> > +\n> > + if (fd >= 0 && fd != dfd &&\n> > + !std::binary_search(v.begin(), v.end(), fd))\n> > + close(fd);\n> > + }\n> > +\n> > + closedir(dir);\n> > +}\n> > +\n> > } /* namespace */\n> > \n> > void ProcessManager::sighandler()\n> > @@ -282,34 +310,6 @@ int Process::start(const std::string &path,\n> > }\n> > }\n> > \n> > -void Process::closeAllFdsExcept(Span fds)\n> > -{\n> > - std::vector v(fds.begin(), fds.end());\n> > - sort(v.begin(), v.end());\n> > -\n> > - ASSERT(v.empty() || v.front() >= 0);\n> > -\n> > - DIR *dir = opendir(\"/proc/self/fd\");\n> > - if (!dir)\n> > - return;\n> > -\n> > - int dfd = dirfd(dir);\n> > -\n> > - struct dirent *ent;\n> > - while ((ent = readdir(dir)) != nullptr) {\n> > - char *endp;\n> > - int fd = strtoul(ent->d_name, &endp, 10);\n> > - if (*endp)\n> > - continue;\n> > -\n> > - if (fd >= 0 && fd != dfd &&\n> > - !std::binary_search(v.begin(), v.end(), fd))\n> > - close(fd);\n> > - }\n> > -\n> > - closedir(dir);\n> > -}\n> > -\n> > int Process::isolate()\n> > {\n> > int ret = unshare(CLONE_NEWUSER | CLONE_NEWNET);","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 2D35EC323E\n\tfor ;\n\tWed, 26 Mar 2025 14:28:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 090BB68969;\n\tWed, 26 Mar 2025 15:28:16 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0158868950\n\tfor ;\n\tWed, 26 Mar 2025 15:28:13 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B5F66475;\n\tWed, 26 Mar 2025 15:26:25 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"UQYlnPLb\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1742999185;\n\tbh=IEPXTrh8fvIfjVPXIpnenovz74XKx27dz5QHxTZZVLM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=UQYlnPLbV8cjoAl7FpSLpp9y7x19Qu4At/9dqJ0926rlG5xFZDBPfo6Ai/dCo1+4h\n\tDl/4NhPyXkFeX+Ns0/ZcJzNOWxYATyN3TKeHnfV4+wNKd9tRUSPqIjxzL4tqwYti7S\n\t9jnozWEj9d10B34PJyFHyldBEXNsVeLv7Doi8dSM=","Date":"Wed, 26 Mar 2025 16:27:51 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v3 7/9] libcamera: process: Move\n\t`closeAllFdsExcept()`","Message-ID":"<20250326142751.GG8303@pendragon.ideasonboard.com>","References":"<20250325180821.1456154-1-barnabas.pocze@ideasonboard.com>\n\t<20250325180821.1456154-8-barnabas.pocze@ideasonboard.com>\n\t<174292977704.2136573.9696533061048901069@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<174292977704.2136573.9696533061048901069@ping.linuxembedded.co.uk>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":33728,"web_url":"https://patchwork.libcamera.org/comment/33728/","msgid":"<20250326150325.GI8303@pendragon.ideasonboard.com>","date":"2025-03-26T15:03:25","subject":"Re: [RFC PATCH v3 9/9] libcamera: process: Remove `ProcessManager`\n\tsingleton","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Barnabás,\n\nThank you for the patch.\n\nOn Tue, Mar 25, 2025 at 07:08:21PM +0100, Barnabás Pőcze wrote:\n> The `ProcessManager` is a singleton class to handle `SIGCHLD` signals\n> and report the exit status to the particular `Process` instances.\n> \n> However, having a singleton in a library is not favourable and it is\n> even less favourable if it installs a signal handler.\n> \n> Using pidfd it is possible to avoid the need for the signal handler;\n> and the `Process` objects can watch their pidfd themselves, eliminating\n> the need for the `ProcessManager` class altogether.\n> \n> `P_PIDFD` for `waitid()` was introduced in Linux 5.4, so this change\n> raises the minimum supported kernel version. `clone3()`, `CLONE_PIDFD`,\n> `pidfd_send_signal()` were all introduced earlier.\n> \n> Furthermore, the call to the `unshare()` system call can be removed\n> as those options can be passed to `clone3()` directly.\n> \n> Signed-off-by: Barnabás Pőcze \n> ---\n> include/libcamera/internal/camera_manager.h | 1 -\n> include/libcamera/internal/process.h | 34 +--\n> meson.build | 2 +-\n> src/libcamera/process.cpp | 241 +++++---------------\n> test/ipc/unixsocket_ipc.cpp | 2 -\n> test/log/log_process.cpp | 2 -\n> test/process/process_test.cpp | 2 -\n> 7 files changed, 55 insertions(+), 229 deletions(-)\n> \n> diff --git a/include/libcamera/internal/camera_manager.h b/include/libcamera/internal/camera_manager.h\n> index 0150ca61f..5dfbe1f65 100644\n> --- a/include/libcamera/internal/camera_manager.h\n> +++ b/include/libcamera/internal/camera_manager.h\n> @@ -65,7 +65,6 @@ private:\n> \tstd::unique_ptr enumerator_;\n> \n> \tstd::unique_ptr ipaManager_;\n> -\tProcessManager processManager_;\n> };\n> \n> } /* namespace libcamera */\n> diff --git a/include/libcamera/internal/process.h b/include/libcamera/internal/process.h\n> index 14391d60a..e600a49f8 100644\n> --- a/include/libcamera/internal/process.h\n> +++ b/include/libcamera/internal/process.h\n> @@ -7,7 +7,6 @@\n> \n> #pragma once\n> \n> -#include \n> #include \n> \n> #include \n> @@ -44,41 +43,14 @@ public:\n> private:\n> \tLIBCAMERA_DISABLE_COPY_AND_MOVE(Process)\n> \n> -\tint isolate();\n> -\tvoid died(int wstatus);\n> -\n> \tpid_t pid_;\n> \tenum ExitStatus exitStatus_;\n> \tint exitCode_;\n> \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();\n> -\n> -\tstd::list processes_;\n> -\n> -\tstruct sigaction oldsa_;\n> +\tUniqueFD pidfd_;\n> +\tstd::unique_ptr pidfdNotify_;\n> \n> -\tEventNotifier *sigEvent_;\n> -\tUniqueFD pipe_[2];\n> +\tvoid onPidfdNotify();\n\nWe declare member functions before member variables.\n\n> };\n> \n> } /* namespace libcamera */\n> diff --git a/meson.build b/meson.build\n> index 00291d628..bd2c88dfa 100644\n> --- a/meson.build\n> +++ b/meson.build\n> @@ -265,7 +265,7 @@ subdir('Documentation')\n> subdir('test')\n> \n> if not meson.is_cross_build()\n> - kernel_version_req = '>= 5.0.0'\n> + kernel_version_req = '>= 5.4.0'\n> kernel_version = run_command('uname', '-r', check : true).stdout().strip()\n> if not kernel_version.version_compare(kernel_version_req)\n> warning('The current running kernel version @0@ is too old to run libcamera.'\n> diff --git a/src/libcamera/process.cpp b/src/libcamera/process.cpp\n> index 5fa813300..cadf5c43e 100644\n> --- a/src/libcamera/process.cpp\n> +++ b/src/libcamera/process.cpp\n> @@ -10,15 +10,19 @@\n> #include \n> #include \n> #include \n> -#include \n> +#include \n> #include \n> #include \n> #include \n> +#include \n> #include \n> #include \n> #include \n> #include \n> \n> +#include \n> +#include /* glibc only provides `P_PIDFD` in `sys/wait.h` since 2.36 */\n> +\n> #include \n> #include \n> #include \n> @@ -32,37 +36,8 @@ namespace libcamera {\n> \n> LOG_DEFINE_CATEGORY(Process)\n> \n> -/**\n> - * \\class ProcessManager\n> - * \\brief Manager of processes\n> - *\n> - * The ProcessManager singleton keeps track of all created Process instances,\n> - * and manages the signal handling involved in terminating processes.\n> - */\n> -\n> namespace {\n> \n> -void sigact(int signal, siginfo_t *info, void *ucontext)\n> -{\n> -#pragma GCC diagnostic push\n> -#pragma GCC diagnostic ignored \"-Wunused-result\"\n> -\t/*\n> -\t * We're in a signal handler so we can't log any message, and we need\n> -\t * to continue anyway.\n> -\t */\n> -\tchar data = 0;\n> -\twrite(ProcessManager::instance()->writePipe(), &data, sizeof(data));\n> -#pragma GCC diagnostic pop\n> -\n> -\tconst struct sigaction &oldsa = ProcessManager::instance()->oldsa();\n> -\tif (oldsa.sa_flags & SA_SIGINFO) {\n> -\t\toldsa.sa_sigaction(signal, info, ucontext);\n> -\t} else {\n> -\t\tif (oldsa.sa_handler != SIG_IGN && oldsa.sa_handler != SIG_DFL)\n> -\t\t\toldsa.sa_handler(signal);\n> -\t}\n> -}\n> -\n> void closeAllFdsExcept(Span fds)\n> {\n> \tstd::vector v(fds.begin(), fds.end());\n> @@ -113,129 +88,6 @@ void closeAllFdsExcept(Span fds)\n> \n> } /* namespace */\n> \n> -void ProcessManager::sighandler()\n> -{\n> -\tchar data;\n> -\tssize_t ret = read(pipe_[0].get(), &data, sizeof(data));\n> -\tif (ret < 0) {\n> -\t\tLOG(Process, Error)\n> -\t\t\t<< \"Failed to read byte from signal handler pipe\";\n> -\t\treturn;\n> -\t}\n> -\n> -\tfor (auto it = processes_.begin(); it != processes_.end();) {\n> -\t\tProcess *process = *it;\n> -\n> -\t\tint wstatus;\n> -\t\tpid_t pid = waitpid(process->pid_, &wstatus, WNOHANG);\n> -\t\tif (process->pid_ != pid) {\n> -\t\t\t++it;\n> -\t\t\tcontinue;\n> -\t\t}\n> -\n> -\t\tit = processes_.erase(it);\n> -\t\tprocess->died(wstatus);\n> -\t}\n> -}\n> -\n> -/**\n> - * \\brief Register process with process manager\n> - * \\param[in] proc Process to register\n> - *\n> - * This function registers the \\a proc with the process manager. It\n> - * shall be called by the parent process after successfully forking, in\n> - * order to let the parent signal process termination.\n> - */\n> -void ProcessManager::registerProcess(Process *proc)\n> -{\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> -\tmemset(&sa, 0, sizeof(sa));\n> -\tsa.sa_sigaction = &sigact;\n> -\tmemcpy(&sa.sa_mask, &oldsa_.sa_mask, sizeof(sa.sa_mask));\n> -\tsigaddset(&sa.sa_mask, SIGCHLD);\n> -\tsa.sa_flags = oldsa_.sa_flags | SA_SIGINFO;\n> -\n> -\tsigaction(SIGCHLD, &sa, NULL);\n> -\n> -\tint pipe[2];\n> -\tif (pipe2(pipe, O_CLOEXEC | O_DIRECT | O_NONBLOCK))\n> -\t\tLOG(Process, Fatal)\n> -\t\t\t<< \"Failed to initialize pipe for signal handling\";\n> -\n> -\tpipe_[0] = UniqueFD(pipe[0]);\n> -\tpipe_[1] = UniqueFD(pipe[1]);\n> -\n> -\tsigEvent_ = new EventNotifier(pipe_[0].get(), EventNotifier::Read);\n> -\tsigEvent_->activated.connect(this, &ProcessManager::sighandler);\n> -\n> -\tself_ = this;\n> -}\n> -\n> -ProcessManager::~ProcessManager()\n> -{\n> -\tsigaction(SIGCHLD, &oldsa_, NULL);\n> -\n> -\tdelete sigEvent_;\n> -\n> -\tself_ = nullptr;\n> -}\n> -\n> -/**\n> - * \\brief Retrieve the Process manager instance\n> - *\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> -\treturn self_;\n> -}\n> -\n> -/**\n> - * \\brief Retrieve the Process manager's write pipe\n> - *\n> - * This function is meant only to be used by the static signal handler.\n> - *\n> - * \\return Pipe for writing\n> - */\n> -int ProcessManager::writePipe() const\n> -{\n> -\treturn pipe_[1].get();\n> -}\n> -\n> -/**\n> - * \\brief Retrive the old signal action data\n> - *\n> - * This function is meant only to be used by the static signal handler.\n> - *\n> - * \\return The old signal action data\n> - */\n> -const struct sigaction &ProcessManager::oldsa() const\n> -{\n> -\treturn oldsa_;\n> -}\n> -\n> /**\n> * \\class Process\n> * \\brief Process object\n> @@ -286,8 +138,6 @@ int Process::start(const std::string &path,\n> \t\t Span args,\n> \t\t Span fds)\n> {\n> -\tint ret;\n> -\n> \tif (pid_ > 0)\n> \t\treturn -EBUSY;\n> \n> @@ -296,20 +146,26 @@ int Process::start(const std::string &path,\n> \t\t\treturn -EINVAL;\n> \t}\n> \n> -\tint childPid = fork();\n> -\tif (childPid == -1) {\n> -\t\tret = -errno;\n> +\tint pidfd;\n> +\tclone_args cargs = {};\n> +\n> +\tcargs.flags = CLONE_CLEAR_SIGHAND | CLONE_PIDFD | CLONE_NEWUSER | CLONE_NEWNET;\n> +\tcargs.pidfd = reinterpret_cast(&pidfd);\n> +\tcargs.exit_signal = SIGCHLD;\n> +\n> +\tlong childPid = syscall(SYS_clone3, &cargs, sizeof(cargs));\n> +\tif (childPid < 0) {\n> +\t\tint ret = -errno;\n> \t\tLOG(Process, Error) << \"Failed to fork: \" << strerror(-ret);\n> \t\treturn ret;\n> -\t} else if (childPid) {\n> -\t\tpid_ = childPid;\n> -\t\tProcessManager::instance()->registerProcess(this);\n> +\t}\n> \n> -\t\treturn 0;\n> +\tif (childPid) {\n> +\t\tpid_ = childPid;\n> +\t\tpidfd_ = UniqueFD(pidfd);\n> +\t\tpidfdNotify_ = std::make_unique(pidfd_.get(), EventNotifier::Type::Read);\n> +\t\tpidfdNotify_->activated.connect(this, &Process::onPidfdNotify);\n> \t} else {\n> -\t\tif (isolate())\n> -\t\t\t_exit(EXIT_FAILURE);\n> -\n> \t\tcloseAllFdsExcept(fds);\n> \n> \t\tconst char *file = utils::secure_getenv(\"LIBCAMERA_LOG_FILE\");\n> @@ -328,35 +184,40 @@ int Process::start(const std::string &path,\n> \n> \t\texit(EXIT_FAILURE);\n> \t}\n> -}\n> -\n> -int Process::isolate()\n> -{\n> -\tint ret = unshare(CLONE_NEWUSER | CLONE_NEWNET);\n> -\tif (ret) {\n> -\t\tret = -errno;\n> -\t\tLOG(Process, Error) << \"Failed to unshare execution context: \"\n> -\t\t\t\t << strerror(-ret);\n> -\t\treturn ret;\n> -\t}\n> \n> \treturn 0;\n> }\n> \n> -/**\n> - * \\brief SIGCHLD handler\n> - * \\param[in] wstatus The status as output by waitpid()\n> - *\n> - * This function is called when the process associated with Process terminates.\n> - * It emits the Process::finished signal.\n> - */\n> -void Process::died(int wstatus)\n> +void Process::onPidfdNotify()\n> {\n> -\tpid_ = -1;\n> -\texitStatus_ = WIFEXITED(wstatus) ? NormalExit : SignalExit;\n> -\texitCode_ = exitStatus_ == NormalExit ? WEXITSTATUS(wstatus) : -1;\n> +\tauto pidfdNotify = std::exchange(pidfdNotify_, {});\n> +\tauto pidfd = std::exchange(pidfd_, {});\n> +\tauto pid = std::exchange(pid_, -1);\n> +\n> +\tASSERT(pidfdNotify);\n\nDo we have to guard against this, or could we just write\n\n\tpidfdNotify_.reset();\n\n?\n\n> +\tASSERT(pidfd.isValid());\n> +\tASSERT(pid > 0);\n> +\n> +\tsiginfo_t info;\n> +\n> +\tif (waitid(static_cast(P_PIDFD), pidfd.get(), &info, WNOHANG | WEXITED) >= 0) {\n> +\t\tASSERT(info.si_pid == pid);\n> \n> -\tfinished.emit(exitStatus_, exitCode_);\n> +\t\tLOG(Process, Debug)\n> +\t\t\t<< this << \"[\" << pid << \":\" << pidfd.get() << \"]\"\n\nSounds like a candidate to inherit from Loggable and avoid duplication\nof this in log message. You would need to reset pid_ to -1 at the end of\nthe function though. Except it should be done before emitting the\nfinished signal, as there's no running_ variable anymore. Annoying... I\nwonder if we should keep running_, or if there's a cleaner way.\n\nReviewed-by: Laurent Pinchart \n\n> +\t\t\t<< \" code: \" << info.si_code\n> +\t\t\t<< \" status: \" << info.si_status;\n> +\n> +\t\texitStatus_ = info.si_code == CLD_EXITED ? Process::NormalExit : Process::SignalExit;\n> +\t\texitCode_ = info.si_code == CLD_EXITED ? info.si_status : -1;\n> +\n> +\t\tfinished.emit(exitStatus_, exitCode_);\n> +\t} else {\n> +\t\tint err = errno;\n> +\t\tLOG(Process, Warning)\n> +\t\t\t<< this << \"[\" << pid << \":\" << pidfd.get() << \"]\"\n> +\t\t\t<< \" waitid() failed: \" << strerror(err);\n> +\t}\n> }\n> \n> /**\n> @@ -394,8 +255,8 @@ void Process::died(int wstatus)\n> */\n> void Process::kill()\n> {\n> -\tif (pid_ > 0)\n> -\t\t::kill(pid_, SIGKILL);\n> +\tif (pidfd_.isValid())\n> +\t\tsyscall(SYS_pidfd_send_signal, pidfd_.get(), SIGKILL, nullptr, 0);\n> }\n> \n> } /* namespace libcamera */\n> diff --git a/test/ipc/unixsocket_ipc.cpp b/test/ipc/unixsocket_ipc.cpp\n> index df7d9c2b4..f3e3c09ef 100644\n> --- a/test/ipc/unixsocket_ipc.cpp\n> +++ b/test/ipc/unixsocket_ipc.cpp\n> @@ -209,8 +209,6 @@ protected:\n> \t}\n> \n> private:\n> -\tProcessManager processManager_;\n> -\n> \tunique_ptr ipc_;\n> };\n> \n> diff --git a/test/log/log_process.cpp b/test/log/log_process.cpp\n> index 9609e23d5..367b78803 100644\n> --- a/test/log/log_process.cpp\n> +++ b/test/log/log_process.cpp\n> @@ -137,8 +137,6 @@ private:\n> \t\texitCode_ = exitCode;\n> \t}\n> \n> -\tProcessManager processManager_;\n> -\n> \tProcess proc_;\n> \tProcess::ExitStatus exitStatus_ = Process::NotExited;\n> \tstring logPath_;\n> diff --git a/test/process/process_test.cpp b/test/process/process_test.cpp\n> index a88d8fef1..fe76666e6 100644\n> --- a/test/process/process_test.cpp\n> +++ b/test/process/process_test.cpp\n> @@ -86,8 +86,6 @@ private:\n> \t\texitCode_ = exitCode;\n> \t}\n> \n> -\tProcessManager processManager_;\n> -\n> \tProcess proc_;\n> \tenum Process::ExitStatus exitStatus_;\n> \tint exitCode_;","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 81B34C323E\n\tfor ;\n\tWed, 26 Mar 2025 15:03:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7A5716896A;\n\tWed, 26 Mar 2025 16:03:50 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 968C068950\n\tfor ;\n\tWed, 26 Mar 2025 16:03:48 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7AAAE3A4;\n\tWed, 26 Mar 2025 16:02:00 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"mbzWiJHj\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1743001320;\n\tbh=1CcX7JQgb0Fsv+LbcmM01/G2ZzAIA9uh4i3A56h6f44=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=mbzWiJHjeU+hQXiqlOAq+xC1hf+PkH0FloJekdBnAVhVNebo8IRPi8w/cBjkxMGaa\n\tKsEyRLGJ2cagJQZDlEntm3RGOxa8ab1p5CmfzjCmiBlSrCNpb0vOwM29qYT+BgK1rd\n\tPI/ZbaqIKT4aKuroJri7rIE3hRHNCxaPfyy3vIZE=","Date":"Wed, 26 Mar 2025 17:03:25 +0200","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v3 9/9] libcamera: process: Remove `ProcessManager`\n\tsingleton","Message-ID":"<20250326150325.GI8303@pendragon.ideasonboard.com>","References":"<20250325180821.1456154-1-barnabas.pocze@ideasonboard.com>\n\t<20250325180821.1456154-10-barnabas.pocze@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20250325180821.1456154-10-barnabas.pocze@ideasonboard.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":33748,"web_url":"https://patchwork.libcamera.org/comment/33748/","msgid":"<6260c1de-296f-47e0-a608-787763355508@ideasonboard.com>","date":"2025-03-27T16:13:05","subject":"Re: [RFC PATCH v3 9/9] libcamera: process: Remove `ProcessManager`\n\tsingleton","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n\n2025. 03. 26. 16:03 keltezéssel, Laurent Pinchart írta:\n> Hi Barnabás,\n> \n> Thank you for the patch.\n> \n> On Tue, Mar 25, 2025 at 07:08:21PM +0100, Barnabás Pőcze wrote:\n>> The `ProcessManager` is a singleton class to handle `SIGCHLD` signals\n>> and report the exit status to the particular `Process` instances.\n>>\n>> However, having a singleton in a library is not favourable and it is\n>> even less favourable if it installs a signal handler.\n>>\n>> Using pidfd it is possible to avoid the need for the signal handler;\n>> and the `Process` objects can watch their pidfd themselves, eliminating\n>> the need for the `ProcessManager` class altogether.\n>>\n>> `P_PIDFD` for `waitid()` was introduced in Linux 5.4, so this change\n>> raises the minimum supported kernel version. `clone3()`, `CLONE_PIDFD`,\n>> `pidfd_send_signal()` were all introduced earlier.\n>>\n>> Furthermore, the call to the `unshare()` system call can be removed\n>> as those options can be passed to `clone3()` directly.\n>>\n>> Signed-off-by: Barnabás Pőcze \n>> ---\n>> include/libcamera/internal/camera_manager.h | 1 -\n>> include/libcamera/internal/process.h | 34 +--\n>> meson.build | 2 +-\n>> src/libcamera/process.cpp | 241 +++++---------------\n>> test/ipc/unixsocket_ipc.cpp | 2 -\n>> test/log/log_process.cpp | 2 -\n>> test/process/process_test.cpp | 2 -\n>> 7 files changed, 55 insertions(+), 229 deletions(-)\n>>\n>> diff --git a/include/libcamera/internal/camera_manager.h b/include/libcamera/internal/camera_manager.h\n>> index 0150ca61f..5dfbe1f65 100644\n>> --- a/include/libcamera/internal/camera_manager.h\n>> +++ b/include/libcamera/internal/camera_manager.h\n>> @@ -65,7 +65,6 @@ private:\n>> \tstd::unique_ptr enumerator_;\n>> \n>> \tstd::unique_ptr ipaManager_;\n>> -\tProcessManager processManager_;\n>> };\n>> \n>> } /* namespace libcamera */\n>> diff --git a/include/libcamera/internal/process.h b/include/libcamera/internal/process.h\n>> index 14391d60a..e600a49f8 100644\n>> --- a/include/libcamera/internal/process.h\n>> +++ b/include/libcamera/internal/process.h\n>> @@ -7,7 +7,6 @@\n>> \n>> #pragma once\n>> \n>> -#include \n>> #include \n>> \n>> #include \n>> @@ -44,41 +43,14 @@ public:\n>> private:\n>> \tLIBCAMERA_DISABLE_COPY_AND_MOVE(Process)\n>> \n>> -\tint isolate();\n>> -\tvoid died(int wstatus);\n>> -\n>> \tpid_t pid_;\n>> \tenum ExitStatus exitStatus_;\n>> \tint exitCode_;\n>> \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();\n>> -\n>> -\tstd::list processes_;\n>> -\n>> -\tstruct sigaction oldsa_;\n>> +\tUniqueFD pidfd_;\n>> +\tstd::unique_ptr pidfdNotify_;\n>> \n>> -\tEventNotifier *sigEvent_;\n>> -\tUniqueFD pipe_[2];\n>> +\tvoid onPidfdNotify();\n> \n> We declare member functions before member variables.\n> \n>> };\n>> \n>> } /* namespace libcamera */\n>> diff --git a/meson.build b/meson.build\n>> index 00291d628..bd2c88dfa 100644\n>> --- a/meson.build\n>> +++ b/meson.build\n>> @@ -265,7 +265,7 @@ subdir('Documentation')\n>> subdir('test')\n>> \n>> if not meson.is_cross_build()\n>> - kernel_version_req = '>= 5.0.0'\n>> + kernel_version_req = '>= 5.4.0'\n>> kernel_version = run_command('uname', '-r', check : true).stdout().strip()\n>> if not kernel_version.version_compare(kernel_version_req)\n>> warning('The current running kernel version @0@ is too old to run libcamera.'\n>> diff --git a/src/libcamera/process.cpp b/src/libcamera/process.cpp\n>> index 5fa813300..cadf5c43e 100644\n>> --- a/src/libcamera/process.cpp\n>> +++ b/src/libcamera/process.cpp\n>> @@ -10,15 +10,19 @@\n>> #include \n>> #include \n>> #include \n>> -#include \n>> +#include \n>> #include \n>> #include \n>> #include \n>> +#include \n>> #include \n>> #include \n>> #include \n>> #include \n>> \n>> +#include \n>> +#include /* glibc only provides `P_PIDFD` in `sys/wait.h` since 2.36 */\n>> +\n>> #include \n>> #include \n>> #include \n>> @@ -32,37 +36,8 @@ namespace libcamera {\n>> \n>> LOG_DEFINE_CATEGORY(Process)\n>> \n>> -/**\n>> - * \\class ProcessManager\n>> - * \\brief Manager of processes\n>> - *\n>> - * The ProcessManager singleton keeps track of all created Process instances,\n>> - * and manages the signal handling involved in terminating processes.\n>> - */\n>> -\n>> namespace {\n>> \n>> -void sigact(int signal, siginfo_t *info, void *ucontext)\n>> -{\n>> -#pragma GCC diagnostic push\n>> -#pragma GCC diagnostic ignored \"-Wunused-result\"\n>> -\t/*\n>> -\t * We're in a signal handler so we can't log any message, and we need\n>> -\t * to continue anyway.\n>> -\t */\n>> -\tchar data = 0;\n>> -\twrite(ProcessManager::instance()->writePipe(), &data, sizeof(data));\n>> -#pragma GCC diagnostic pop\n>> -\n>> -\tconst struct sigaction &oldsa = ProcessManager::instance()->oldsa();\n>> -\tif (oldsa.sa_flags & SA_SIGINFO) {\n>> -\t\toldsa.sa_sigaction(signal, info, ucontext);\n>> -\t} else {\n>> -\t\tif (oldsa.sa_handler != SIG_IGN && oldsa.sa_handler != SIG_DFL)\n>> -\t\t\toldsa.sa_handler(signal);\n>> -\t}\n>> -}\n>> -\n>> void closeAllFdsExcept(Span fds)\n>> {\n>> \tstd::vector v(fds.begin(), fds.end());\n>> @@ -113,129 +88,6 @@ void closeAllFdsExcept(Span fds)\n>> \n>> } /* namespace */\n>> \n>> -void ProcessManager::sighandler()\n>> -{\n>> -\tchar data;\n>> -\tssize_t ret = read(pipe_[0].get(), &data, sizeof(data));\n>> -\tif (ret < 0) {\n>> -\t\tLOG(Process, Error)\n>> -\t\t\t<< \"Failed to read byte from signal handler pipe\";\n>> -\t\treturn;\n>> -\t}\n>> -\n>> -\tfor (auto it = processes_.begin(); it != processes_.end();) {\n>> -\t\tProcess *process = *it;\n>> -\n>> -\t\tint wstatus;\n>> -\t\tpid_t pid = waitpid(process->pid_, &wstatus, WNOHANG);\n>> -\t\tif (process->pid_ != pid) {\n>> -\t\t\t++it;\n>> -\t\t\tcontinue;\n>> -\t\t}\n>> -\n>> -\t\tit = processes_.erase(it);\n>> -\t\tprocess->died(wstatus);\n>> -\t}\n>> -}\n>> -\n>> -/**\n>> - * \\brief Register process with process manager\n>> - * \\param[in] proc Process to register\n>> - *\n>> - * This function registers the \\a proc with the process manager. It\n>> - * shall be called by the parent process after successfully forking, in\n>> - * order to let the parent signal process termination.\n>> - */\n>> -void ProcessManager::registerProcess(Process *proc)\n>> -{\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>> -\tmemset(&sa, 0, sizeof(sa));\n>> -\tsa.sa_sigaction = &sigact;\n>> -\tmemcpy(&sa.sa_mask, &oldsa_.sa_mask, sizeof(sa.sa_mask));\n>> -\tsigaddset(&sa.sa_mask, SIGCHLD);\n>> -\tsa.sa_flags = oldsa_.sa_flags | SA_SIGINFO;\n>> -\n>> -\tsigaction(SIGCHLD, &sa, NULL);\n>> -\n>> -\tint pipe[2];\n>> -\tif (pipe2(pipe, O_CLOEXEC | O_DIRECT | O_NONBLOCK))\n>> -\t\tLOG(Process, Fatal)\n>> -\t\t\t<< \"Failed to initialize pipe for signal handling\";\n>> -\n>> -\tpipe_[0] = UniqueFD(pipe[0]);\n>> -\tpipe_[1] = UniqueFD(pipe[1]);\n>> -\n>> -\tsigEvent_ = new EventNotifier(pipe_[0].get(), EventNotifier::Read);\n>> -\tsigEvent_->activated.connect(this, &ProcessManager::sighandler);\n>> -\n>> -\tself_ = this;\n>> -}\n>> -\n>> -ProcessManager::~ProcessManager()\n>> -{\n>> -\tsigaction(SIGCHLD, &oldsa_, NULL);\n>> -\n>> -\tdelete sigEvent_;\n>> -\n>> -\tself_ = nullptr;\n>> -}\n>> -\n>> -/**\n>> - * \\brief Retrieve the Process manager instance\n>> - *\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>> -\treturn self_;\n>> -}\n>> -\n>> -/**\n>> - * \\brief Retrieve the Process manager's write pipe\n>> - *\n>> - * This function is meant only to be used by the static signal handler.\n>> - *\n>> - * \\return Pipe for writing\n>> - */\n>> -int ProcessManager::writePipe() const\n>> -{\n>> -\treturn pipe_[1].get();\n>> -}\n>> -\n>> -/**\n>> - * \\brief Retrive the old signal action data\n>> - *\n>> - * This function is meant only to be used by the static signal handler.\n>> - *\n>> - * \\return The old signal action data\n>> - */\n>> -const struct sigaction &ProcessManager::oldsa() const\n>> -{\n>> -\treturn oldsa_;\n>> -}\n>> -\n>> /**\n>> * \\class Process\n>> * \\brief Process object\n>> @@ -286,8 +138,6 @@ int Process::start(const std::string &path,\n>> \t\t Span args,\n>> \t\t Span fds)\n>> {\n>> -\tint ret;\n>> -\n>> \tif (pid_ > 0)\n>> \t\treturn -EBUSY;\n>> \n>> @@ -296,20 +146,26 @@ int Process::start(const std::string &path,\n>> \t\t\treturn -EINVAL;\n>> \t}\n>> \n>> -\tint childPid = fork();\n>> -\tif (childPid == -1) {\n>> -\t\tret = -errno;\n>> +\tint pidfd;\n>> +\tclone_args cargs = {};\n>> +\n>> +\tcargs.flags = CLONE_CLEAR_SIGHAND | CLONE_PIDFD | CLONE_NEWUSER | CLONE_NEWNET;\n>> +\tcargs.pidfd = reinterpret_cast(&pidfd);\n>> +\tcargs.exit_signal = SIGCHLD;\n>> +\n>> +\tlong childPid = syscall(SYS_clone3, &cargs, sizeof(cargs));\n>> +\tif (childPid < 0) {\n>> +\t\tint ret = -errno;\n>> \t\tLOG(Process, Error) << \"Failed to fork: \" << strerror(-ret);\n>> \t\treturn ret;\n>> -\t} else if (childPid) {\n>> -\t\tpid_ = childPid;\n>> -\t\tProcessManager::instance()->registerProcess(this);\n>> +\t}\n>> \n>> -\t\treturn 0;\n>> +\tif (childPid) {\n>> +\t\tpid_ = childPid;\n>> +\t\tpidfd_ = UniqueFD(pidfd);\n>> +\t\tpidfdNotify_ = std::make_unique(pidfd_.get(), EventNotifier::Type::Read);\n>> +\t\tpidfdNotify_->activated.connect(this, &Process::onPidfdNotify);\n>> \t} else {\n>> -\t\tif (isolate())\n>> -\t\t\t_exit(EXIT_FAILURE);\n>> -\n>> \t\tcloseAllFdsExcept(fds);\n>> \n>> \t\tconst char *file = utils::secure_getenv(\"LIBCAMERA_LOG_FILE\");\n>> @@ -328,35 +184,40 @@ int Process::start(const std::string &path,\n>> \n>> \t\texit(EXIT_FAILURE);\n>> \t}\n>> -}\n>> -\n>> -int Process::isolate()\n>> -{\n>> -\tint ret = unshare(CLONE_NEWUSER | CLONE_NEWNET);\n>> -\tif (ret) {\n>> -\t\tret = -errno;\n>> -\t\tLOG(Process, Error) << \"Failed to unshare execution context: \"\n>> -\t\t\t\t << strerror(-ret);\n>> -\t\treturn ret;\n>> -\t}\n>> \n>> \treturn 0;\n>> }\n>> \n>> -/**\n>> - * \\brief SIGCHLD handler\n>> - * \\param[in] wstatus The status as output by waitpid()\n>> - *\n>> - * This function is called when the process associated with Process terminates.\n>> - * It emits the Process::finished signal.\n>> - */\n>> -void Process::died(int wstatus)\n>> +void Process::onPidfdNotify()\n>> {\n>> -\tpid_ = -1;\n>> -\texitStatus_ = WIFEXITED(wstatus) ? NormalExit : SignalExit;\n>> -\texitCode_ = exitStatus_ == NormalExit ? WEXITSTATUS(wstatus) : -1;\n>> +\tauto pidfdNotify = std::exchange(pidfdNotify_, {});\n>> +\tauto pidfd = std::exchange(pidfd_, {});\n>> +\tauto pid = std::exchange(pid_, -1);\n>> +\n>> +\tASSERT(pidfdNotify);\n> \n> Do we have to guard against this, or could we just write\n> \n> \tpidfdNotify_.reset();\n> \n> ?\n> \n>> +\tASSERT(pidfd.isValid());\n>> +\tASSERT(pid > 0);\n>> +\n>> +\tsiginfo_t info;\n>> +\n>> +\tif (waitid(static_cast(P_PIDFD), pidfd.get(), &info, WNOHANG | WEXITED) >= 0) {\n>> +\t\tASSERT(info.si_pid == pid);\n>> \n>> -\tfinished.emit(exitStatus_, exitCode_);\n>> +\t\tLOG(Process, Debug)\n>> +\t\t\t<< this << \"[\" << pid << \":\" << pidfd.get() << \"]\"\n> \n> Sounds like a candidate to inherit from Loggable and avoid duplication\n> of this in log message. You would need to reset pid_ to -1 at the end of\n> the function though. Except it should be done before emitting the\n> finished signal, as there's no running_ variable anymore. Annoying... I\n> wonder if we should keep running_, or if there's a cleaner way.\n\nI'll give it some thought because this indeed does not look ideal.\n\nAlso, regrettably it seems chromeos does not have new enough linux headers.\nI'm wondering if it would be possible to update the CI image to something newer?\nI have tested release-R135-16209.B locally, and that appears to have the\nnecessary definitions.\n\n\nRegards,\nBarnabás Pőcze\n\n> \n> Reviewed-by: Laurent Pinchart \n> \n>> +\t\t\t<< \" code: \" << info.si_code\n>> +\t\t\t<< \" status: \" << info.si_status;\n>> +\n>> +\t\texitStatus_ = info.si_code == CLD_EXITED ? Process::NormalExit : Process::SignalExit;\n>> +\t\texitCode_ = info.si_code == CLD_EXITED ? info.si_status : -1;\n>> +\n>> +\t\tfinished.emit(exitStatus_, exitCode_);\n>> +\t} else {\n>> +\t\tint err = errno;\n>> +\t\tLOG(Process, Warning)\n>> +\t\t\t<< this << \"[\" << pid << \":\" << pidfd.get() << \"]\"\n>> +\t\t\t<< \" waitid() failed: \" << strerror(err);\n>> +\t}\n>> }\n>> \n>> /**\n>> @@ -394,8 +255,8 @@ void Process::died(int wstatus)\n>> */\n>> void Process::kill()\n>> {\n>> -\tif (pid_ > 0)\n>> -\t\t::kill(pid_, SIGKILL);\n>> +\tif (pidfd_.isValid())\n>> +\t\tsyscall(SYS_pidfd_send_signal, pidfd_.get(), SIGKILL, nullptr, 0);\n>> }\n>> \n>> } /* namespace libcamera */\n>> diff --git a/test/ipc/unixsocket_ipc.cpp b/test/ipc/unixsocket_ipc.cpp\n>> index df7d9c2b4..f3e3c09ef 100644\n>> --- a/test/ipc/unixsocket_ipc.cpp\n>> +++ b/test/ipc/unixsocket_ipc.cpp\n>> @@ -209,8 +209,6 @@ protected:\n>> \t}\n>> \n>> private:\n>> -\tProcessManager processManager_;\n>> -\n>> \tunique_ptr ipc_;\n>> };\n>> \n>> diff --git a/test/log/log_process.cpp b/test/log/log_process.cpp\n>> index 9609e23d5..367b78803 100644\n>> --- a/test/log/log_process.cpp\n>> +++ b/test/log/log_process.cpp\n>> @@ -137,8 +137,6 @@ private:\n>> \t\texitCode_ = exitCode;\n>> \t}\n>> \n>> -\tProcessManager processManager_;\n>> -\n>> \tProcess proc_;\n>> \tProcess::ExitStatus exitStatus_ = Process::NotExited;\n>> \tstring logPath_;\n>> diff --git a/test/process/process_test.cpp b/test/process/process_test.cpp\n>> index a88d8fef1..fe76666e6 100644\n>> --- a/test/process/process_test.cpp\n>> +++ b/test/process/process_test.cpp\n>> @@ -86,8 +86,6 @@ private:\n>> \t\texitCode_ = exitCode;\n>> \t}\n>> \n>> -\tProcessManager processManager_;\n>> -\n>> \tProcess proc_;\n>> \tenum Process::ExitStatus exitStatus_;\n>> \tint exitCode_;\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 10D24C323E\n\tfor ;\n\tThu, 27 Mar 2025 16:13:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0DF0C68971;\n\tThu, 27 Mar 2025 17:13:14 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 78AC168947\n\tfor ;\n\tThu, 27 Mar 2025 17:13:11 +0100 (CET)","from [192.168.33.10] (185.221.143.221.nat.pool.zt.hu\n\t[185.221.143.221])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B88B2446;\n\tThu, 27 Mar 2025 17:11:20 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ZEJWvBth\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1743091882;\n\tbh=CqeseHHmcs6sMO8HW7C+tCvSB18G/N9WWVOYeWVqBGg=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=ZEJWvBthtXfmIThcacNH4WILBLvEPqSACW6NKI87RuWauuYcLE7rix9CeaKwlCYia\n\tD29cGJVveYbsxruVD4OX4imxCyLWOObNusDgtcNPTsYCIBs3xNS8Pb3PnOJwJVXTLQ\n\tCxkng0/WssukajyaXCFY5l6N+L7LcsMOwjJaqRzE=","Message-ID":"<6260c1de-296f-47e0-a608-787763355508@ideasonboard.com>","Date":"Thu, 27 Mar 2025 17:13:05 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [RFC PATCH v3 9/9] libcamera: process: Remove `ProcessManager`\n\tsingleton","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20250325180821.1456154-1-barnabas.pocze@ideasonboard.com>\n\t<20250325180821.1456154-10-barnabas.pocze@ideasonboard.com>\n\t<20250326150325.GI8303@pendragon.ideasonboard.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<20250326150325.GI8303@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":33751,"web_url":"https://patchwork.libcamera.org/comment/33751/","msgid":"<20250327234531.GN4861@pendragon.ideasonboard.com>","date":"2025-03-27T23:45:31","subject":"Re: [RFC PATCH v3 9/9] libcamera: process: Remove `ProcessManager`\n\tsingleton","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Thu, Mar 27, 2025 at 05:13:05PM +0100, Barnabás Pőcze wrote:\n> 2025. 03. 26. 16:03 keltezéssel, Laurent Pinchart írta:\n> > On Tue, Mar 25, 2025 at 07:08:21PM +0100, Barnabás Pőcze wrote:\n> >> The `ProcessManager` is a singleton class to handle `SIGCHLD` signals\n> >> and report the exit status to the particular `Process` instances.\n> >>\n> >> However, having a singleton in a library is not favourable and it is\n> >> even less favourable if it installs a signal handler.\n> >>\n> >> Using pidfd it is possible to avoid the need for the signal handler;\n> >> and the `Process` objects can watch their pidfd themselves, eliminating\n> >> the need for the `ProcessManager` class altogether.\n> >>\n> >> `P_PIDFD` for `waitid()` was introduced in Linux 5.4, so this change\n> >> raises the minimum supported kernel version. `clone3()`, `CLONE_PIDFD`,\n> >> `pidfd_send_signal()` were all introduced earlier.\n> >>\n> >> Furthermore, the call to the `unshare()` system call can be removed\n> >> as those options can be passed to `clone3()` directly.\n> >>\n> >> Signed-off-by: Barnabás Pőcze \n> >> ---\n> >> include/libcamera/internal/camera_manager.h | 1 -\n> >> include/libcamera/internal/process.h | 34 +--\n> >> meson.build | 2 +-\n> >> src/libcamera/process.cpp | 241 +++++---------------\n> >> test/ipc/unixsocket_ipc.cpp | 2 -\n> >> test/log/log_process.cpp | 2 -\n> >> test/process/process_test.cpp | 2 -\n> >> 7 files changed, 55 insertions(+), 229 deletions(-)\n> >>\n> >> diff --git a/include/libcamera/internal/camera_manager.h b/include/libcamera/internal/camera_manager.h\n> >> index 0150ca61f..5dfbe1f65 100644\n> >> --- a/include/libcamera/internal/camera_manager.h\n> >> +++ b/include/libcamera/internal/camera_manager.h\n> >> @@ -65,7 +65,6 @@ private:\n> >> \tstd::unique_ptr enumerator_;\n> >> \n> >> \tstd::unique_ptr ipaManager_;\n> >> -\tProcessManager processManager_;\n> >> };\n> >> \n> >> } /* namespace libcamera */\n> >> diff --git a/include/libcamera/internal/process.h b/include/libcamera/internal/process.h\n> >> index 14391d60a..e600a49f8 100644\n> >> --- a/include/libcamera/internal/process.h\n> >> +++ b/include/libcamera/internal/process.h\n> >> @@ -7,7 +7,6 @@\n> >> \n> >> #pragma once\n> >> \n> >> -#include \n> >> #include \n> >> \n> >> #include \n> >> @@ -44,41 +43,14 @@ public:\n> >> private:\n> >> \tLIBCAMERA_DISABLE_COPY_AND_MOVE(Process)\n> >> \n> >> -\tint isolate();\n> >> -\tvoid died(int wstatus);\n> >> -\n> >> \tpid_t pid_;\n> >> \tenum ExitStatus exitStatus_;\n> >> \tint exitCode_;\n> >> \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();\n> >> -\n> >> -\tstd::list processes_;\n> >> -\n> >> -\tstruct sigaction oldsa_;\n> >> +\tUniqueFD pidfd_;\n> >> +\tstd::unique_ptr pidfdNotify_;\n> >> \n> >> -\tEventNotifier *sigEvent_;\n> >> -\tUniqueFD pipe_[2];\n> >> +\tvoid onPidfdNotify();\n> > \n> > We declare member functions before member variables.\n> > \n> >> };\n> >> \n> >> } /* namespace libcamera */\n> >> diff --git a/meson.build b/meson.build\n> >> index 00291d628..bd2c88dfa 100644\n> >> --- a/meson.build\n> >> +++ b/meson.build\n> >> @@ -265,7 +265,7 @@ subdir('Documentation')\n> >> subdir('test')\n> >> \n> >> if not meson.is_cross_build()\n> >> - kernel_version_req = '>= 5.0.0'\n> >> + kernel_version_req = '>= 5.4.0'\n> >> kernel_version = run_command('uname', '-r', check : true).stdout().strip()\n> >> if not kernel_version.version_compare(kernel_version_req)\n> >> warning('The current running kernel version @0@ is too old to run libcamera.'\n> >> diff --git a/src/libcamera/process.cpp b/src/libcamera/process.cpp\n> >> index 5fa813300..cadf5c43e 100644\n> >> --- a/src/libcamera/process.cpp\n> >> +++ b/src/libcamera/process.cpp\n> >> @@ -10,15 +10,19 @@\n> >> #include \n> >> #include \n> >> #include \n> >> -#include \n> >> +#include \n> >> #include \n> >> #include \n> >> #include \n> >> +#include \n> >> #include \n> >> #include \n> >> #include \n> >> #include \n> >> \n> >> +#include \n> >> +#include /* glibc only provides `P_PIDFD` in `sys/wait.h` since 2.36 */\n> >> +\n> >> #include \n> >> #include \n> >> #include \n> >> @@ -32,37 +36,8 @@ namespace libcamera {\n> >> \n> >> LOG_DEFINE_CATEGORY(Process)\n> >> \n> >> -/**\n> >> - * \\class ProcessManager\n> >> - * \\brief Manager of processes\n> >> - *\n> >> - * The ProcessManager singleton keeps track of all created Process instances,\n> >> - * and manages the signal handling involved in terminating processes.\n> >> - */\n> >> -\n> >> namespace {\n> >> \n> >> -void sigact(int signal, siginfo_t *info, void *ucontext)\n> >> -{\n> >> -#pragma GCC diagnostic push\n> >> -#pragma GCC diagnostic ignored \"-Wunused-result\"\n> >> -\t/*\n> >> -\t * We're in a signal handler so we can't log any message, and we need\n> >> -\t * to continue anyway.\n> >> -\t */\n> >> -\tchar data = 0;\n> >> -\twrite(ProcessManager::instance()->writePipe(), &data, sizeof(data));\n> >> -#pragma GCC diagnostic pop\n> >> -\n> >> -\tconst struct sigaction &oldsa = ProcessManager::instance()->oldsa();\n> >> -\tif (oldsa.sa_flags & SA_SIGINFO) {\n> >> -\t\toldsa.sa_sigaction(signal, info, ucontext);\n> >> -\t} else {\n> >> -\t\tif (oldsa.sa_handler != SIG_IGN && oldsa.sa_handler != SIG_DFL)\n> >> -\t\t\toldsa.sa_handler(signal);\n> >> -\t}\n> >> -}\n> >> -\n> >> void closeAllFdsExcept(Span fds)\n> >> {\n> >> \tstd::vector v(fds.begin(), fds.end());\n> >> @@ -113,129 +88,6 @@ void closeAllFdsExcept(Span fds)\n> >> \n> >> } /* namespace */\n> >> \n> >> -void ProcessManager::sighandler()\n> >> -{\n> >> -\tchar data;\n> >> -\tssize_t ret = read(pipe_[0].get(), &data, sizeof(data));\n> >> -\tif (ret < 0) {\n> >> -\t\tLOG(Process, Error)\n> >> -\t\t\t<< \"Failed to read byte from signal handler pipe\";\n> >> -\t\treturn;\n> >> -\t}\n> >> -\n> >> -\tfor (auto it = processes_.begin(); it != processes_.end();) {\n> >> -\t\tProcess *process = *it;\n> >> -\n> >> -\t\tint wstatus;\n> >> -\t\tpid_t pid = waitpid(process->pid_, &wstatus, WNOHANG);\n> >> -\t\tif (process->pid_ != pid) {\n> >> -\t\t\t++it;\n> >> -\t\t\tcontinue;\n> >> -\t\t}\n> >> -\n> >> -\t\tit = processes_.erase(it);\n> >> -\t\tprocess->died(wstatus);\n> >> -\t}\n> >> -}\n> >> -\n> >> -/**\n> >> - * \\brief Register process with process manager\n> >> - * \\param[in] proc Process to register\n> >> - *\n> >> - * This function registers the \\a proc with the process manager. It\n> >> - * shall be called by the parent process after successfully forking, in\n> >> - * order to let the parent signal process termination.\n> >> - */\n> >> -void ProcessManager::registerProcess(Process *proc)\n> >> -{\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> >> -\tmemset(&sa, 0, sizeof(sa));\n> >> -\tsa.sa_sigaction = &sigact;\n> >> -\tmemcpy(&sa.sa_mask, &oldsa_.sa_mask, sizeof(sa.sa_mask));\n> >> -\tsigaddset(&sa.sa_mask, SIGCHLD);\n> >> -\tsa.sa_flags = oldsa_.sa_flags | SA_SIGINFO;\n> >> -\n> >> -\tsigaction(SIGCHLD, &sa, NULL);\n> >> -\n> >> -\tint pipe[2];\n> >> -\tif (pipe2(pipe, O_CLOEXEC | O_DIRECT | O_NONBLOCK))\n> >> -\t\tLOG(Process, Fatal)\n> >> -\t\t\t<< \"Failed to initialize pipe for signal handling\";\n> >> -\n> >> -\tpipe_[0] = UniqueFD(pipe[0]);\n> >> -\tpipe_[1] = UniqueFD(pipe[1]);\n> >> -\n> >> -\tsigEvent_ = new EventNotifier(pipe_[0].get(), EventNotifier::Read);\n> >> -\tsigEvent_->activated.connect(this, &ProcessManager::sighandler);\n> >> -\n> >> -\tself_ = this;\n> >> -}\n> >> -\n> >> -ProcessManager::~ProcessManager()\n> >> -{\n> >> -\tsigaction(SIGCHLD, &oldsa_, NULL);\n> >> -\n> >> -\tdelete sigEvent_;\n> >> -\n> >> -\tself_ = nullptr;\n> >> -}\n> >> -\n> >> -/**\n> >> - * \\brief Retrieve the Process manager instance\n> >> - *\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> >> -\treturn self_;\n> >> -}\n> >> -\n> >> -/**\n> >> - * \\brief Retrieve the Process manager's write pipe\n> >> - *\n> >> - * This function is meant only to be used by the static signal handler.\n> >> - *\n> >> - * \\return Pipe for writing\n> >> - */\n> >> -int ProcessManager::writePipe() const\n> >> -{\n> >> -\treturn pipe_[1].get();\n> >> -}\n> >> -\n> >> -/**\n> >> - * \\brief Retrive the old signal action data\n> >> - *\n> >> - * This function is meant only to be used by the static signal handler.\n> >> - *\n> >> - * \\return The old signal action data\n> >> - */\n> >> -const struct sigaction &ProcessManager::oldsa() const\n> >> -{\n> >> -\treturn oldsa_;\n> >> -}\n> >> -\n> >> /**\n> >> * \\class Process\n> >> * \\brief Process object\n> >> @@ -286,8 +138,6 @@ int Process::start(const std::string &path,\n> >> \t\t Span args,\n> >> \t\t Span fds)\n> >> {\n> >> -\tint ret;\n> >> -\n> >> \tif (pid_ > 0)\n> >> \t\treturn -EBUSY;\n> >> \n> >> @@ -296,20 +146,26 @@ int Process::start(const std::string &path,\n> >> \t\t\treturn -EINVAL;\n> >> \t}\n> >> \n> >> -\tint childPid = fork();\n> >> -\tif (childPid == -1) {\n> >> -\t\tret = -errno;\n> >> +\tint pidfd;\n> >> +\tclone_args cargs = {};\n> >> +\n> >> +\tcargs.flags = CLONE_CLEAR_SIGHAND | CLONE_PIDFD | CLONE_NEWUSER | CLONE_NEWNET;\n> >> +\tcargs.pidfd = reinterpret_cast(&pidfd);\n> >> +\tcargs.exit_signal = SIGCHLD;\n> >> +\n> >> +\tlong childPid = syscall(SYS_clone3, &cargs, sizeof(cargs));\n> >> +\tif (childPid < 0) {\n> >> +\t\tint ret = -errno;\n> >> \t\tLOG(Process, Error) << \"Failed to fork: \" << strerror(-ret);\n> >> \t\treturn ret;\n> >> -\t} else if (childPid) {\n> >> -\t\tpid_ = childPid;\n> >> -\t\tProcessManager::instance()->registerProcess(this);\n> >> +\t}\n> >> \n> >> -\t\treturn 0;\n> >> +\tif (childPid) {\n> >> +\t\tpid_ = childPid;\n> >> +\t\tpidfd_ = UniqueFD(pidfd);\n> >> +\t\tpidfdNotify_ = std::make_unique(pidfd_.get(), EventNotifier::Type::Read);\n> >> +\t\tpidfdNotify_->activated.connect(this, &Process::onPidfdNotify);\n> >> \t} else {\n> >> -\t\tif (isolate())\n> >> -\t\t\t_exit(EXIT_FAILURE);\n> >> -\n> >> \t\tcloseAllFdsExcept(fds);\n> >> \n> >> \t\tconst char *file = utils::secure_getenv(\"LIBCAMERA_LOG_FILE\");\n> >> @@ -328,35 +184,40 @@ int Process::start(const std::string &path,\n> >> \n> >> \t\texit(EXIT_FAILURE);\n> >> \t}\n> >> -}\n> >> -\n> >> -int Process::isolate()\n> >> -{\n> >> -\tint ret = unshare(CLONE_NEWUSER | CLONE_NEWNET);\n> >> -\tif (ret) {\n> >> -\t\tret = -errno;\n> >> -\t\tLOG(Process, Error) << \"Failed to unshare execution context: \"\n> >> -\t\t\t\t << strerror(-ret);\n> >> -\t\treturn ret;\n> >> -\t}\n> >> \n> >> \treturn 0;\n> >> }\n> >> \n> >> -/**\n> >> - * \\brief SIGCHLD handler\n> >> - * \\param[in] wstatus The status as output by waitpid()\n> >> - *\n> >> - * This function is called when the process associated with Process terminates.\n> >> - * It emits the Process::finished signal.\n> >> - */\n> >> -void Process::died(int wstatus)\n> >> +void Process::onPidfdNotify()\n> >> {\n> >> -\tpid_ = -1;\n> >> -\texitStatus_ = WIFEXITED(wstatus) ? NormalExit : SignalExit;\n> >> -\texitCode_ = exitStatus_ == NormalExit ? WEXITSTATUS(wstatus) : -1;\n> >> +\tauto pidfdNotify = std::exchange(pidfdNotify_, {});\n> >> +\tauto pidfd = std::exchange(pidfd_, {});\n> >> +\tauto pid = std::exchange(pid_, -1);\n> >> +\n> >> +\tASSERT(pidfdNotify);\n> > \n> > Do we have to guard against this, or could we just write\n> > \n> > \tpidfdNotify_.reset();\n> > \n> > ?\n> > \n> >> +\tASSERT(pidfd.isValid());\n> >> +\tASSERT(pid > 0);\n> >> +\n> >> +\tsiginfo_t info;\n> >> +\n> >> +\tif (waitid(static_cast(P_PIDFD), pidfd.get(), &info, WNOHANG | WEXITED) >= 0) {\n> >> +\t\tASSERT(info.si_pid == pid);\n> >> \n> >> -\tfinished.emit(exitStatus_, exitCode_);\n> >> +\t\tLOG(Process, Debug)\n> >> +\t\t\t<< this << \"[\" << pid << \":\" << pidfd.get() << \"]\"\n> > \n> > Sounds like a candidate to inherit from Loggable and avoid duplication\n> > of this in log message. You would need to reset pid_ to -1 at the end of\n> > the function though. Except it should be done before emitting the\n> > finished signal, as there's no running_ variable anymore. Annoying... I\n> > wonder if we should keep running_, or if there's a cleaner way.\n> \n> I'll give it some thought because this indeed does not look ideal.\n> \n> Also, regrettably it seems chromeos does not have new enough linux headers.\n> I'm wondering if it would be possible to update the CI image to something newer?\n> I have tested release-R135-16209.B locally, and that appears to have the\n> necessary definitions.\n\nThat would require a recent image for Soraka, and I don't know if we can\nobtain one. Without that, we won't be able to run test on Chrome OS\nanymore.\n\n> > Reviewed-by: Laurent Pinchart \n> > \n> >> +\t\t\t<< \" code: \" << info.si_code\n> >> +\t\t\t<< \" status: \" << info.si_status;\n> >> +\n> >> +\t\texitStatus_ = info.si_code == CLD_EXITED ? Process::NormalExit : Process::SignalExit;\n> >> +\t\texitCode_ = info.si_code == CLD_EXITED ? info.si_status : -1;\n> >> +\n> >> +\t\tfinished.emit(exitStatus_, exitCode_);\n> >> +\t} else {\n> >> +\t\tint err = errno;\n> >> +\t\tLOG(Process, Warning)\n> >> +\t\t\t<< this << \"[\" << pid << \":\" << pidfd.get() << \"]\"\n> >> +\t\t\t<< \" waitid() failed: \" << strerror(err);\n> >> +\t}\n> >> }\n> >> \n> >> /**\n> >> @@ -394,8 +255,8 @@ void Process::died(int wstatus)\n> >> */\n> >> void Process::kill()\n> >> {\n> >> -\tif (pid_ > 0)\n> >> -\t\t::kill(pid_, SIGKILL);\n> >> +\tif (pidfd_.isValid())\n> >> +\t\tsyscall(SYS_pidfd_send_signal, pidfd_.get(), SIGKILL, nullptr, 0);\n> >> }\n> >> \n> >> } /* namespace libcamera */\n> >> diff --git a/test/ipc/unixsocket_ipc.cpp b/test/ipc/unixsocket_ipc.cpp\n> >> index df7d9c2b4..f3e3c09ef 100644\n> >> --- a/test/ipc/unixsocket_ipc.cpp\n> >> +++ b/test/ipc/unixsocket_ipc.cpp\n> >> @@ -209,8 +209,6 @@ protected:\n> >> \t}\n> >> \n> >> private:\n> >> -\tProcessManager processManager_;\n> >> -\n> >> \tunique_ptr ipc_;\n> >> };\n> >> \n> >> diff --git a/test/log/log_process.cpp b/test/log/log_process.cpp\n> >> index 9609e23d5..367b78803 100644\n> >> --- a/test/log/log_process.cpp\n> >> +++ b/test/log/log_process.cpp\n> >> @@ -137,8 +137,6 @@ private:\n> >> \t\texitCode_ = exitCode;\n> >> \t}\n> >> \n> >> -\tProcessManager processManager_;\n> >> -\n> >> \tProcess proc_;\n> >> \tProcess::ExitStatus exitStatus_ = Process::NotExited;\n> >> \tstring logPath_;\n> >> diff --git a/test/process/process_test.cpp b/test/process/process_test.cpp\n> >> index a88d8fef1..fe76666e6 100644\n> >> --- a/test/process/process_test.cpp\n> >> +++ b/test/process/process_test.cpp\n> >> @@ -86,8 +86,6 @@ private:\n> >> \t\texitCode_ = exitCode;\n> >> \t}\n> >> \n> >> -\tProcessManager processManager_;\n> >> -\n> >> \tProcess proc_;\n> >> \tenum Process::ExitStatus exitStatus_;\n> >> \tint exitCode_;","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 36D14C323E\n\tfor ;\n\tThu, 27 Mar 2025 23:46:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3EBAF68971;\n\tFri, 28 Mar 2025 00:45:59 +0100 (CET)","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 8EAE16894F\n\tfor ;\n\tFri, 28 Mar 2025 00:45:56 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4D4953DA;\n\tFri, 28 Mar 2025 00:44:07 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ZFbyZCM9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1743119047;\n\tbh=CJVDpbQWFueoaoOseVT45mRl3MfE/vTHRPT9TS+mRro=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ZFbyZCM99czIBFGpPv4Y3DWVKIbnWwTJWfHYkKBnLs7heSyfIAzcsNE3tguLw6pvh\n\tcsvNV/3mdw+wtdRuG6O1vM5mIy3TrhrSe75Ha0XU+/N2AATK4HokkZZ3trXIFW3VJ8\n\tSFR4mZ343K29oJE5UswLAAfur/vgGQJetj0mQOPI=","Date":"Fri, 28 Mar 2025 01:45:31 +0200","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v3 9/9] libcamera: process: Remove `ProcessManager`\n\tsingleton","Message-ID":"<20250327234531.GN4861@pendragon.ideasonboard.com>","References":"<20250325180821.1456154-1-barnabas.pocze@ideasonboard.com>\n\t<20250325180821.1456154-10-barnabas.pocze@ideasonboard.com>\n\t<20250326150325.GI8303@pendragon.ideasonboard.com>\n\t<6260c1de-296f-47e0-a608-787763355508@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<6260c1de-296f-47e0-a608-787763355508@ideasonboard.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":34024,"web_url":"https://patchwork.libcamera.org/comment/34024/","msgid":"<49234340-2aed-4080-a6c2-9e5ed875eb5f@ideasonboard.com>","date":"2025-04-24T10:56:03","subject":"Re: [RFC PATCH v3 9/9] libcamera: process: Remove `ProcessManager`\n\tsingleton","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n\n2025. 03. 26. 16:03 keltezéssel, Laurent Pinchart írta:\n> Hi Barnabás,\n> \n> Thank you for the patch.\n> \n> On Tue, Mar 25, 2025 at 07:08:21PM +0100, Barnabás Pőcze wrote:\n>> The `ProcessManager` is a singleton class to handle `SIGCHLD` signals\n>> and report the exit status to the particular `Process` instances.\n>>\n>> However, having a singleton in a library is not favourable and it is\n>> even less favourable if it installs a signal handler.\n>>\n>> Using pidfd it is possible to avoid the need for the signal handler;\n>> and the `Process` objects can watch their pidfd themselves, eliminating\n>> the need for the `ProcessManager` class altogether.\n>>\n>> `P_PIDFD` for `waitid()` was introduced in Linux 5.4, so this change\n>> raises the minimum supported kernel version. `clone3()`, `CLONE_PIDFD`,\n>> `pidfd_send_signal()` were all introduced earlier.\n>>\n>> Furthermore, the call to the `unshare()` system call can be removed\n>> as those options can be passed to `clone3()` directly.\n>>\n>> Signed-off-by: Barnabás Pőcze \n>> ---\n>> include/libcamera/internal/camera_manager.h | 1 -\n>> include/libcamera/internal/process.h | 34 +--\n>> meson.build | 2 +-\n>> src/libcamera/process.cpp | 241 +++++---------------\n>> test/ipc/unixsocket_ipc.cpp | 2 -\n>> test/log/log_process.cpp | 2 -\n>> test/process/process_test.cpp | 2 -\n>> 7 files changed, 55 insertions(+), 229 deletions(-)\n>>\n>> diff --git a/include/libcamera/internal/camera_manager.h b/include/libcamera/internal/camera_manager.h\n>> index 0150ca61f..5dfbe1f65 100644\n>> --- a/include/libcamera/internal/camera_manager.h\n>> +++ b/include/libcamera/internal/camera_manager.h\n>> @@ -65,7 +65,6 @@ private:\n>> \tstd::unique_ptr enumerator_;\n>> \n>> \tstd::unique_ptr ipaManager_;\n>> -\tProcessManager processManager_;\n>> };\n>> \n>> } /* namespace libcamera */\n>> diff --git a/include/libcamera/internal/process.h b/include/libcamera/internal/process.h\n>> index 14391d60a..e600a49f8 100644\n>> --- a/include/libcamera/internal/process.h\n>> +++ b/include/libcamera/internal/process.h\n>> @@ -7,7 +7,6 @@\n>> \n>> #pragma once\n>> \n>> -#include \n>> #include \n>> \n>> #include \n>> @@ -44,41 +43,14 @@ public:\n>> private:\n>> \tLIBCAMERA_DISABLE_COPY_AND_MOVE(Process)\n>> \n>> -\tint isolate();\n>> -\tvoid died(int wstatus);\n>> -\n>> \tpid_t pid_;\n>> \tenum ExitStatus exitStatus_;\n>> \tint exitCode_;\n>> \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();\n>> -\n>> -\tstd::list processes_;\n>> -\n>> -\tstruct sigaction oldsa_;\n>> +\tUniqueFD pidfd_;\n>> +\tstd::unique_ptr pidfdNotify_;\n>> \n>> -\tEventNotifier *sigEvent_;\n>> -\tUniqueFD pipe_[2];\n>> +\tvoid onPidfdNotify();\n> \n> We declare member functions before member variables.\n> \n>> };\n>> \n>> } /* namespace libcamera */\n>> diff --git a/meson.build b/meson.build\n>> index 00291d628..bd2c88dfa 100644\n>> --- a/meson.build\n>> +++ b/meson.build\n>> @@ -265,7 +265,7 @@ subdir('Documentation')\n>> subdir('test')\n>> \n>> if not meson.is_cross_build()\n>> - kernel_version_req = '>= 5.0.0'\n>> + kernel_version_req = '>= 5.4.0'\n>> kernel_version = run_command('uname', '-r', check : true).stdout().strip()\n>> if not kernel_version.version_compare(kernel_version_req)\n>> warning('The current running kernel version @0@ is too old to run libcamera.'\n>> diff --git a/src/libcamera/process.cpp b/src/libcamera/process.cpp\n>> index 5fa813300..cadf5c43e 100644\n>> --- a/src/libcamera/process.cpp\n>> +++ b/src/libcamera/process.cpp\n>> @@ -10,15 +10,19 @@\n>> #include \n>> #include \n>> #include \n>> -#include \n>> +#include \n>> #include \n>> #include \n>> #include \n>> +#include \n>> #include \n>> #include \n>> #include \n>> #include \n>> \n>> +#include \n>> +#include /* glibc only provides `P_PIDFD` in `sys/wait.h` since 2.36 */\n>> +\n>> #include \n>> #include \n>> #include \n>> @@ -32,37 +36,8 @@ namespace libcamera {\n>> \n>> LOG_DEFINE_CATEGORY(Process)\n>> \n>> -/**\n>> - * \\class ProcessManager\n>> - * \\brief Manager of processes\n>> - *\n>> - * The ProcessManager singleton keeps track of all created Process instances,\n>> - * and manages the signal handling involved in terminating processes.\n>> - */\n>> -\n>> namespace {\n>> \n>> -void sigact(int signal, siginfo_t *info, void *ucontext)\n>> -{\n>> -#pragma GCC diagnostic push\n>> -#pragma GCC diagnostic ignored \"-Wunused-result\"\n>> -\t/*\n>> -\t * We're in a signal handler so we can't log any message, and we need\n>> -\t * to continue anyway.\n>> -\t */\n>> -\tchar data = 0;\n>> -\twrite(ProcessManager::instance()->writePipe(), &data, sizeof(data));\n>> -#pragma GCC diagnostic pop\n>> -\n>> -\tconst struct sigaction &oldsa = ProcessManager::instance()->oldsa();\n>> -\tif (oldsa.sa_flags & SA_SIGINFO) {\n>> -\t\toldsa.sa_sigaction(signal, info, ucontext);\n>> -\t} else {\n>> -\t\tif (oldsa.sa_handler != SIG_IGN && oldsa.sa_handler != SIG_DFL)\n>> -\t\t\toldsa.sa_handler(signal);\n>> -\t}\n>> -}\n>> -\n>> void closeAllFdsExcept(Span fds)\n>> {\n>> \tstd::vector v(fds.begin(), fds.end());\n>> @@ -113,129 +88,6 @@ void closeAllFdsExcept(Span fds)\n>> \n>> } /* namespace */\n>> \n>> -void ProcessManager::sighandler()\n>> -{\n>> -\tchar data;\n>> -\tssize_t ret = read(pipe_[0].get(), &data, sizeof(data));\n>> -\tif (ret < 0) {\n>> -\t\tLOG(Process, Error)\n>> -\t\t\t<< \"Failed to read byte from signal handler pipe\";\n>> -\t\treturn;\n>> -\t}\n>> -\n>> -\tfor (auto it = processes_.begin(); it != processes_.end();) {\n>> -\t\tProcess *process = *it;\n>> -\n>> -\t\tint wstatus;\n>> -\t\tpid_t pid = waitpid(process->pid_, &wstatus, WNOHANG);\n>> -\t\tif (process->pid_ != pid) {\n>> -\t\t\t++it;\n>> -\t\t\tcontinue;\n>> -\t\t}\n>> -\n>> -\t\tit = processes_.erase(it);\n>> -\t\tprocess->died(wstatus);\n>> -\t}\n>> -}\n>> -\n>> -/**\n>> - * \\brief Register process with process manager\n>> - * \\param[in] proc Process to register\n>> - *\n>> - * This function registers the \\a proc with the process manager. It\n>> - * shall be called by the parent process after successfully forking, in\n>> - * order to let the parent signal process termination.\n>> - */\n>> -void ProcessManager::registerProcess(Process *proc)\n>> -{\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>> -\tmemset(&sa, 0, sizeof(sa));\n>> -\tsa.sa_sigaction = &sigact;\n>> -\tmemcpy(&sa.sa_mask, &oldsa_.sa_mask, sizeof(sa.sa_mask));\n>> -\tsigaddset(&sa.sa_mask, SIGCHLD);\n>> -\tsa.sa_flags = oldsa_.sa_flags | SA_SIGINFO;\n>> -\n>> -\tsigaction(SIGCHLD, &sa, NULL);\n>> -\n>> -\tint pipe[2];\n>> -\tif (pipe2(pipe, O_CLOEXEC | O_DIRECT | O_NONBLOCK))\n>> -\t\tLOG(Process, Fatal)\n>> -\t\t\t<< \"Failed to initialize pipe for signal handling\";\n>> -\n>> -\tpipe_[0] = UniqueFD(pipe[0]);\n>> -\tpipe_[1] = UniqueFD(pipe[1]);\n>> -\n>> -\tsigEvent_ = new EventNotifier(pipe_[0].get(), EventNotifier::Read);\n>> -\tsigEvent_->activated.connect(this, &ProcessManager::sighandler);\n>> -\n>> -\tself_ = this;\n>> -}\n>> -\n>> -ProcessManager::~ProcessManager()\n>> -{\n>> -\tsigaction(SIGCHLD, &oldsa_, NULL);\n>> -\n>> -\tdelete sigEvent_;\n>> -\n>> -\tself_ = nullptr;\n>> -}\n>> -\n>> -/**\n>> - * \\brief Retrieve the Process manager instance\n>> - *\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>> -\treturn self_;\n>> -}\n>> -\n>> -/**\n>> - * \\brief Retrieve the Process manager's write pipe\n>> - *\n>> - * This function is meant only to be used by the static signal handler.\n>> - *\n>> - * \\return Pipe for writing\n>> - */\n>> -int ProcessManager::writePipe() const\n>> -{\n>> -\treturn pipe_[1].get();\n>> -}\n>> -\n>> -/**\n>> - * \\brief Retrive the old signal action data\n>> - *\n>> - * This function is meant only to be used by the static signal handler.\n>> - *\n>> - * \\return The old signal action data\n>> - */\n>> -const struct sigaction &ProcessManager::oldsa() const\n>> -{\n>> -\treturn oldsa_;\n>> -}\n>> -\n>> /**\n>> * \\class Process\n>> * \\brief Process object\n>> @@ -286,8 +138,6 @@ int Process::start(const std::string &path,\n>> \t\t Span args,\n>> \t\t Span fds)\n>> {\n>> -\tint ret;\n>> -\n>> \tif (pid_ > 0)\n>> \t\treturn -EBUSY;\n>> \n>> @@ -296,20 +146,26 @@ int Process::start(const std::string &path,\n>> \t\t\treturn -EINVAL;\n>> \t}\n>> \n>> -\tint childPid = fork();\n>> -\tif (childPid == -1) {\n>> -\t\tret = -errno;\n>> +\tint pidfd;\n>> +\tclone_args cargs = {};\n>> +\n>> +\tcargs.flags = CLONE_CLEAR_SIGHAND | CLONE_PIDFD | CLONE_NEWUSER | CLONE_NEWNET;\n>> +\tcargs.pidfd = reinterpret_cast(&pidfd);\n>> +\tcargs.exit_signal = SIGCHLD;\n>> +\n>> +\tlong childPid = syscall(SYS_clone3, &cargs, sizeof(cargs));\n>> +\tif (childPid < 0) {\n>> +\t\tint ret = -errno;\n>> \t\tLOG(Process, Error) << \"Failed to fork: \" << strerror(-ret);\n>> \t\treturn ret;\n>> -\t} else if (childPid) {\n>> -\t\tpid_ = childPid;\n>> -\t\tProcessManager::instance()->registerProcess(this);\n>> +\t}\n>> \n>> -\t\treturn 0;\n>> +\tif (childPid) {\n>> +\t\tpid_ = childPid;\n>> +\t\tpidfd_ = UniqueFD(pidfd);\n>> +\t\tpidfdNotify_ = std::make_unique(pidfd_.get(), EventNotifier::Type::Read);\n>> +\t\tpidfdNotify_->activated.connect(this, &Process::onPidfdNotify);\n>> \t} else {\n>> -\t\tif (isolate())\n>> -\t\t\t_exit(EXIT_FAILURE);\n>> -\n>> \t\tcloseAllFdsExcept(fds);\n>> \n>> \t\tconst char *file = utils::secure_getenv(\"LIBCAMERA_LOG_FILE\");\n>> @@ -328,35 +184,40 @@ int Process::start(const std::string &path,\n>> \n>> \t\texit(EXIT_FAILURE);\n>> \t}\n>> -}\n>> -\n>> -int Process::isolate()\n>> -{\n>> -\tint ret = unshare(CLONE_NEWUSER | CLONE_NEWNET);\n>> -\tif (ret) {\n>> -\t\tret = -errno;\n>> -\t\tLOG(Process, Error) << \"Failed to unshare execution context: \"\n>> -\t\t\t\t << strerror(-ret);\n>> -\t\treturn ret;\n>> -\t}\n>> \n>> \treturn 0;\n>> }\n>> \n>> -/**\n>> - * \\brief SIGCHLD handler\n>> - * \\param[in] wstatus The status as output by waitpid()\n>> - *\n>> - * This function is called when the process associated with Process terminates.\n>> - * It emits the Process::finished signal.\n>> - */\n>> -void Process::died(int wstatus)\n>> +void Process::onPidfdNotify()\n>> {\n>> -\tpid_ = -1;\n>> -\texitStatus_ = WIFEXITED(wstatus) ? NormalExit : SignalExit;\n>> -\texitCode_ = exitStatus_ == NormalExit ? WEXITSTATUS(wstatus) : -1;\n>> +\tauto pidfdNotify = std::exchange(pidfdNotify_, {});\n>> +\tauto pidfd = std::exchange(pidfd_, {});\n>> +\tauto pid = std::exchange(pid_, -1);\n>> +\n>> +\tASSERT(pidfdNotify);\n> \n> Do we have to guard against this, or could we just write\n> \n> \tpidfdNotify_.reset();\n> \n> ?\n\nIt would be sufficient, but I like to check that the state of the object\nis what is expected (necessary) when this function is called.\n\n\n> \n>> +\tASSERT(pidfd.isValid());\n>> +\tASSERT(pid > 0);\n>> +\n>> +\tsiginfo_t info;\n>> +\n>> +\tif (waitid(static_cast(P_PIDFD), pidfd.get(), &info, WNOHANG | WEXITED) >= 0) {\n>> +\t\tASSERT(info.si_pid == pid);\n>> \n>> -\tfinished.emit(exitStatus_, exitCode_);\n>> +\t\tLOG(Process, Debug)\n>> +\t\t\t<< this << \"[\" << pid << \":\" << pidfd.get() << \"]\"\n> \n> Sounds like a candidate to inherit from Loggable and avoid duplication\n> of this in log message. You would need to reset pid_ to -1 at the end of\n> the function though. Except it should be done before emitting the\n> finished signal, as there's no running_ variable anymore. Annoying... I\n> wonder if we should keep running_, or if there's a cleaner way.\n\nEven with `running_`, I think the same problem applies to `pidFd_` because\nit has to be closed, but if the `finished` signal handler starts the process\nagain, then it must not be closed. So one would have to duplicate `pidFd_.reset()`\nin both branches.\n\nI hope the duplication in the log message is acceptable because it keeps the\nactual logic simpler (i.e. reset object state at the beginning unconditionally).\n\n\nRegards,\nBarnabás Pőcze\n\n\n> \n> Reviewed-by: Laurent Pinchart \n> \n>> +\t\t\t<< \" code: \" << info.si_code\n>> +\t\t\t<< \" status: \" << info.si_status;\n>> +\n>> +\t\texitStatus_ = info.si_code == CLD_EXITED ? Process::NormalExit : Process::SignalExit;\n>> +\t\texitCode_ = info.si_code == CLD_EXITED ? info.si_status : -1;\n>> +\n>> +\t\tfinished.emit(exitStatus_, exitCode_);\n>> +\t} else {\n>> +\t\tint err = errno;\n>> +\t\tLOG(Process, Warning)\n>> +\t\t\t<< this << \"[\" << pid << \":\" << pidfd.get() << \"]\"\n>> +\t\t\t<< \" waitid() failed: \" << strerror(err);\n>> +\t}\n>> }\n>> \n>> /**\n>> @@ -394,8 +255,8 @@ void Process::died(int wstatus)\n>> */\n>> void Process::kill()\n>> {\n>> -\tif (pid_ > 0)\n>> -\t\t::kill(pid_, SIGKILL);\n>> +\tif (pidfd_.isValid())\n>> +\t\tsyscall(SYS_pidfd_send_signal, pidfd_.get(), SIGKILL, nullptr, 0);\n>> }\n>> \n>> } /* namespace libcamera */\n>> diff --git a/test/ipc/unixsocket_ipc.cpp b/test/ipc/unixsocket_ipc.cpp\n>> index df7d9c2b4..f3e3c09ef 100644\n>> --- a/test/ipc/unixsocket_ipc.cpp\n>> +++ b/test/ipc/unixsocket_ipc.cpp\n>> @@ -209,8 +209,6 @@ protected:\n>> \t}\n>> \n>> private:\n>> -\tProcessManager processManager_;\n>> -\n>> \tunique_ptr ipc_;\n>> };\n>> \n>> diff --git a/test/log/log_process.cpp b/test/log/log_process.cpp\n>> index 9609e23d5..367b78803 100644\n>> --- a/test/log/log_process.cpp\n>> +++ b/test/log/log_process.cpp\n>> @@ -137,8 +137,6 @@ private:\n>> \t\texitCode_ = exitCode;\n>> \t}\n>> \n>> -\tProcessManager processManager_;\n>> -\n>> \tProcess proc_;\n>> \tProcess::ExitStatus exitStatus_ = Process::NotExited;\n>> \tstring logPath_;\n>> diff --git a/test/process/process_test.cpp b/test/process/process_test.cpp\n>> index a88d8fef1..fe76666e6 100644\n>> --- a/test/process/process_test.cpp\n>> +++ b/test/process/process_test.cpp\n>> @@ -86,8 +86,6 @@ private:\n>> \t\texitCode_ = exitCode;\n>> \t}\n>> \n>> -\tProcessManager processManager_;\n>> -\n>> \tProcess proc_;\n>> \tenum Process::ExitStatus exitStatus_;\n>> \tint exitCode_;\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 D083ABE08B\n\tfor ;\n\tThu, 24 Apr 2025 10:56:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 03EF868ACD;\n\tThu, 24 Apr 2025 12:56:10 +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 82AF968AC5\n\tfor ;\n\tThu, 24 Apr 2025 12:56:08 +0200 (CEST)","from [192.168.33.15] (185.221.143.16.nat.pool.zt.hu\n\t[185.221.143.16])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5CA52F01;\n\tThu, 24 Apr 2025 12:56:05 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ii01Xhlv\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1745492165;\n\tbh=Hx3dYpo3YZZg7JPJPyfzaSJpfzh3rAD+K9Ze1ZYD5ns=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=ii01XhlvODmikSoJAySXjTUX06hbmv5zhjbTNvhQdqjC2CbETKxLAbTOSqFbu221u\n\tL+AAUs2m6BY751LHeOzSW8H0nLauF3Mrq35KihXGaqoTgLQ/4wlH+2v+aJNpAqFzZW\n\tWYRv+p12COFycBRIf7rD8dhaoPZ7THFuFLARHTQ0=","Message-ID":"<49234340-2aed-4080-a6c2-9e5ed875eb5f@ideasonboard.com>","Date":"Thu, 24 Apr 2025 12:56:03 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [RFC PATCH v3 9/9] libcamera: process: Remove `ProcessManager`\n\tsingleton","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20250325180821.1456154-1-barnabas.pocze@ideasonboard.com>\n\t<20250325180821.1456154-10-barnabas.pocze@ideasonboard.com>\n\t<20250326150325.GI8303@pendragon.ideasonboard.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<20250326150325.GI8303@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":34025,"web_url":"https://patchwork.libcamera.org/comment/34025/","msgid":"<20250424111123.GB18085@pendragon.ideasonboard.com>","date":"2025-04-24T11:11:23","subject":"Re: [RFC PATCH v3 9/9] libcamera: process: Remove `ProcessManager`\n\tsingleton","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Thu, Apr 24, 2025 at 12:56:03PM +0200, Barnabás Pőcze wrote:\n> 2025. 03. 26. 16:03 keltezéssel, Laurent Pinchart írta:\n> > Hi Barnabás,\n> > \n> > Thank you for the patch.\n> > \n> > On Tue, Mar 25, 2025 at 07:08:21PM +0100, Barnabás Pőcze wrote:\n> >> The `ProcessManager` is a singleton class to handle `SIGCHLD` signals\n> >> and report the exit status to the particular `Process` instances.\n> >>\n> >> However, having a singleton in a library is not favourable and it is\n> >> even less favourable if it installs a signal handler.\n> >>\n> >> Using pidfd it is possible to avoid the need for the signal handler;\n> >> and the `Process` objects can watch their pidfd themselves, eliminating\n> >> the need for the `ProcessManager` class altogether.\n> >>\n> >> `P_PIDFD` for `waitid()` was introduced in Linux 5.4, so this change\n> >> raises the minimum supported kernel version. `clone3()`, `CLONE_PIDFD`,\n> >> `pidfd_send_signal()` were all introduced earlier.\n> >>\n> >> Furthermore, the call to the `unshare()` system call can be removed\n> >> as those options can be passed to `clone3()` directly.\n> >>\n> >> Signed-off-by: Barnabás Pőcze \n> >> ---\n> >> include/libcamera/internal/camera_manager.h | 1 -\n> >> include/libcamera/internal/process.h | 34 +--\n> >> meson.build | 2 +-\n> >> src/libcamera/process.cpp | 241 +++++---------------\n> >> test/ipc/unixsocket_ipc.cpp | 2 -\n> >> test/log/log_process.cpp | 2 -\n> >> test/process/process_test.cpp | 2 -\n> >> 7 files changed, 55 insertions(+), 229 deletions(-)\n> >>\n> >> diff --git a/include/libcamera/internal/camera_manager.h b/include/libcamera/internal/camera_manager.h\n> >> index 0150ca61f..5dfbe1f65 100644\n> >> --- a/include/libcamera/internal/camera_manager.h\n> >> +++ b/include/libcamera/internal/camera_manager.h\n> >> @@ -65,7 +65,6 @@ private:\n> >> \tstd::unique_ptr enumerator_;\n> >> \n> >> \tstd::unique_ptr ipaManager_;\n> >> -\tProcessManager processManager_;\n> >> };\n> >> \n> >> } /* namespace libcamera */\n> >> diff --git a/include/libcamera/internal/process.h b/include/libcamera/internal/process.h\n> >> index 14391d60a..e600a49f8 100644\n> >> --- a/include/libcamera/internal/process.h\n> >> +++ b/include/libcamera/internal/process.h\n> >> @@ -7,7 +7,6 @@\n> >> \n> >> #pragma once\n> >> \n> >> -#include \n> >> #include \n> >> \n> >> #include \n> >> @@ -44,41 +43,14 @@ public:\n> >> private:\n> >> \tLIBCAMERA_DISABLE_COPY_AND_MOVE(Process)\n> >> \n> >> -\tint isolate();\n> >> -\tvoid died(int wstatus);\n> >> -\n> >> \tpid_t pid_;\n> >> \tenum ExitStatus exitStatus_;\n> >> \tint exitCode_;\n> >> \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();\n> >> -\n> >> -\tstd::list processes_;\n> >> -\n> >> -\tstruct sigaction oldsa_;\n> >> +\tUniqueFD pidfd_;\n> >> +\tstd::unique_ptr pidfdNotify_;\n> >> \n> >> -\tEventNotifier *sigEvent_;\n> >> -\tUniqueFD pipe_[2];\n> >> +\tvoid onPidfdNotify();\n> > \n> > We declare member functions before member variables.\n> > \n> >> };\n> >> \n> >> } /* namespace libcamera */\n> >> diff --git a/meson.build b/meson.build\n> >> index 00291d628..bd2c88dfa 100644\n> >> --- a/meson.build\n> >> +++ b/meson.build\n> >> @@ -265,7 +265,7 @@ subdir('Documentation')\n> >> subdir('test')\n> >> \n> >> if not meson.is_cross_build()\n> >> - kernel_version_req = '>= 5.0.0'\n> >> + kernel_version_req = '>= 5.4.0'\n> >> kernel_version = run_command('uname', '-r', check : true).stdout().strip()\n> >> if not kernel_version.version_compare(kernel_version_req)\n> >> warning('The current running kernel version @0@ is too old to run libcamera.'\n> >> diff --git a/src/libcamera/process.cpp b/src/libcamera/process.cpp\n> >> index 5fa813300..cadf5c43e 100644\n> >> --- a/src/libcamera/process.cpp\n> >> +++ b/src/libcamera/process.cpp\n> >> @@ -10,15 +10,19 @@\n> >> #include \n> >> #include \n> >> #include \n> >> -#include \n> >> +#include \n> >> #include \n> >> #include \n> >> #include \n> >> +#include \n> >> #include \n> >> #include \n> >> #include \n> >> #include \n> >> \n> >> +#include \n> >> +#include /* glibc only provides `P_PIDFD` in `sys/wait.h` since 2.36 */\n> >> +\n> >> #include \n> >> #include \n> >> #include \n> >> @@ -32,37 +36,8 @@ namespace libcamera {\n> >> \n> >> LOG_DEFINE_CATEGORY(Process)\n> >> \n> >> -/**\n> >> - * \\class ProcessManager\n> >> - * \\brief Manager of processes\n> >> - *\n> >> - * The ProcessManager singleton keeps track of all created Process instances,\n> >> - * and manages the signal handling involved in terminating processes.\n> >> - */\n> >> -\n> >> namespace {\n> >> \n> >> -void sigact(int signal, siginfo_t *info, void *ucontext)\n> >> -{\n> >> -#pragma GCC diagnostic push\n> >> -#pragma GCC diagnostic ignored \"-Wunused-result\"\n> >> -\t/*\n> >> -\t * We're in a signal handler so we can't log any message, and we need\n> >> -\t * to continue anyway.\n> >> -\t */\n> >> -\tchar data = 0;\n> >> -\twrite(ProcessManager::instance()->writePipe(), &data, sizeof(data));\n> >> -#pragma GCC diagnostic pop\n> >> -\n> >> -\tconst struct sigaction &oldsa = ProcessManager::instance()->oldsa();\n> >> -\tif (oldsa.sa_flags & SA_SIGINFO) {\n> >> -\t\toldsa.sa_sigaction(signal, info, ucontext);\n> >> -\t} else {\n> >> -\t\tif (oldsa.sa_handler != SIG_IGN && oldsa.sa_handler != SIG_DFL)\n> >> -\t\t\toldsa.sa_handler(signal);\n> >> -\t}\n> >> -}\n> >> -\n> >> void closeAllFdsExcept(Span fds)\n> >> {\n> >> \tstd::vector v(fds.begin(), fds.end());\n> >> @@ -113,129 +88,6 @@ void closeAllFdsExcept(Span fds)\n> >> \n> >> } /* namespace */\n> >> \n> >> -void ProcessManager::sighandler()\n> >> -{\n> >> -\tchar data;\n> >> -\tssize_t ret = read(pipe_[0].get(), &data, sizeof(data));\n> >> -\tif (ret < 0) {\n> >> -\t\tLOG(Process, Error)\n> >> -\t\t\t<< \"Failed to read byte from signal handler pipe\";\n> >> -\t\treturn;\n> >> -\t}\n> >> -\n> >> -\tfor (auto it = processes_.begin(); it != processes_.end();) {\n> >> -\t\tProcess *process = *it;\n> >> -\n> >> -\t\tint wstatus;\n> >> -\t\tpid_t pid = waitpid(process->pid_, &wstatus, WNOHANG);\n> >> -\t\tif (process->pid_ != pid) {\n> >> -\t\t\t++it;\n> >> -\t\t\tcontinue;\n> >> -\t\t}\n> >> -\n> >> -\t\tit = processes_.erase(it);\n> >> -\t\tprocess->died(wstatus);\n> >> -\t}\n> >> -}\n> >> -\n> >> -/**\n> >> - * \\brief Register process with process manager\n> >> - * \\param[in] proc Process to register\n> >> - *\n> >> - * This function registers the \\a proc with the process manager. It\n> >> - * shall be called by the parent process after successfully forking, in\n> >> - * order to let the parent signal process termination.\n> >> - */\n> >> -void ProcessManager::registerProcess(Process *proc)\n> >> -{\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> >> -\tmemset(&sa, 0, sizeof(sa));\n> >> -\tsa.sa_sigaction = &sigact;\n> >> -\tmemcpy(&sa.sa_mask, &oldsa_.sa_mask, sizeof(sa.sa_mask));\n> >> -\tsigaddset(&sa.sa_mask, SIGCHLD);\n> >> -\tsa.sa_flags = oldsa_.sa_flags | SA_SIGINFO;\n> >> -\n> >> -\tsigaction(SIGCHLD, &sa, NULL);\n> >> -\n> >> -\tint pipe[2];\n> >> -\tif (pipe2(pipe, O_CLOEXEC | O_DIRECT | O_NONBLOCK))\n> >> -\t\tLOG(Process, Fatal)\n> >> -\t\t\t<< \"Failed to initialize pipe for signal handling\";\n> >> -\n> >> -\tpipe_[0] = UniqueFD(pipe[0]);\n> >> -\tpipe_[1] = UniqueFD(pipe[1]);\n> >> -\n> >> -\tsigEvent_ = new EventNotifier(pipe_[0].get(), EventNotifier::Read);\n> >> -\tsigEvent_->activated.connect(this, &ProcessManager::sighandler);\n> >> -\n> >> -\tself_ = this;\n> >> -}\n> >> -\n> >> -ProcessManager::~ProcessManager()\n> >> -{\n> >> -\tsigaction(SIGCHLD, &oldsa_, NULL);\n> >> -\n> >> -\tdelete sigEvent_;\n> >> -\n> >> -\tself_ = nullptr;\n> >> -}\n> >> -\n> >> -/**\n> >> - * \\brief Retrieve the Process manager instance\n> >> - *\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> >> -\treturn self_;\n> >> -}\n> >> -\n> >> -/**\n> >> - * \\brief Retrieve the Process manager's write pipe\n> >> - *\n> >> - * This function is meant only to be used by the static signal handler.\n> >> - *\n> >> - * \\return Pipe for writing\n> >> - */\n> >> -int ProcessManager::writePipe() const\n> >> -{\n> >> -\treturn pipe_[1].get();\n> >> -}\n> >> -\n> >> -/**\n> >> - * \\brief Retrive the old signal action data\n> >> - *\n> >> - * This function is meant only to be used by the static signal handler.\n> >> - *\n> >> - * \\return The old signal action data\n> >> - */\n> >> -const struct sigaction &ProcessManager::oldsa() const\n> >> -{\n> >> -\treturn oldsa_;\n> >> -}\n> >> -\n> >> /**\n> >> * \\class Process\n> >> * \\brief Process object\n> >> @@ -286,8 +138,6 @@ int Process::start(const std::string &path,\n> >> \t\t Span args,\n> >> \t\t Span fds)\n> >> {\n> >> -\tint ret;\n> >> -\n> >> \tif (pid_ > 0)\n> >> \t\treturn -EBUSY;\n> >> \n> >> @@ -296,20 +146,26 @@ int Process::start(const std::string &path,\n> >> \t\t\treturn -EINVAL;\n> >> \t}\n> >> \n> >> -\tint childPid = fork();\n> >> -\tif (childPid == -1) {\n> >> -\t\tret = -errno;\n> >> +\tint pidfd;\n> >> +\tclone_args cargs = {};\n> >> +\n> >> +\tcargs.flags = CLONE_CLEAR_SIGHAND | CLONE_PIDFD | CLONE_NEWUSER | CLONE_NEWNET;\n> >> +\tcargs.pidfd = reinterpret_cast(&pidfd);\n> >> +\tcargs.exit_signal = SIGCHLD;\n> >> +\n> >> +\tlong childPid = syscall(SYS_clone3, &cargs, sizeof(cargs));\n> >> +\tif (childPid < 0) {\n> >> +\t\tint ret = -errno;\n> >> \t\tLOG(Process, Error) << \"Failed to fork: \" << strerror(-ret);\n> >> \t\treturn ret;\n> >> -\t} else if (childPid) {\n> >> -\t\tpid_ = childPid;\n> >> -\t\tProcessManager::instance()->registerProcess(this);\n> >> +\t}\n> >> \n> >> -\t\treturn 0;\n> >> +\tif (childPid) {\n> >> +\t\tpid_ = childPid;\n> >> +\t\tpidfd_ = UniqueFD(pidfd);\n> >> +\t\tpidfdNotify_ = std::make_unique(pidfd_.get(), EventNotifier::Type::Read);\n> >> +\t\tpidfdNotify_->activated.connect(this, &Process::onPidfdNotify);\n> >> \t} else {\n> >> -\t\tif (isolate())\n> >> -\t\t\t_exit(EXIT_FAILURE);\n> >> -\n> >> \t\tcloseAllFdsExcept(fds);\n> >> \n> >> \t\tconst char *file = utils::secure_getenv(\"LIBCAMERA_LOG_FILE\");\n> >> @@ -328,35 +184,40 @@ int Process::start(const std::string &path,\n> >> \n> >> \t\texit(EXIT_FAILURE);\n> >> \t}\n> >> -}\n> >> -\n> >> -int Process::isolate()\n> >> -{\n> >> -\tint ret = unshare(CLONE_NEWUSER | CLONE_NEWNET);\n> >> -\tif (ret) {\n> >> -\t\tret = -errno;\n> >> -\t\tLOG(Process, Error) << \"Failed to unshare execution context: \"\n> >> -\t\t\t\t << strerror(-ret);\n> >> -\t\treturn ret;\n> >> -\t}\n> >> \n> >> \treturn 0;\n> >> }\n> >> \n> >> -/**\n> >> - * \\brief SIGCHLD handler\n> >> - * \\param[in] wstatus The status as output by waitpid()\n> >> - *\n> >> - * This function is called when the process associated with Process terminates.\n> >> - * It emits the Process::finished signal.\n> >> - */\n> >> -void Process::died(int wstatus)\n> >> +void Process::onPidfdNotify()\n> >> {\n> >> -\tpid_ = -1;\n> >> -\texitStatus_ = WIFEXITED(wstatus) ? NormalExit : SignalExit;\n> >> -\texitCode_ = exitStatus_ == NormalExit ? WEXITSTATUS(wstatus) : -1;\n> >> +\tauto pidfdNotify = std::exchange(pidfdNotify_, {});\n> >> +\tauto pidfd = std::exchange(pidfd_, {});\n> >> +\tauto pid = std::exchange(pid_, -1);\n> >> +\n> >> +\tASSERT(pidfdNotify);\n> > \n> > Do we have to guard against this, or could we just write\n> > \n> > \tpidfdNotify_.reset();\n> > \n> > ?\n> \n> It would be sufficient, but I like to check that the state of the object\n> is what is expected (necessary) when this function is called.\n\nI tend to avoid checks for things that really can't happen, but I\nunderstand it's a personal preference. \"Really can't happen\" also\ndepends very much on personal confidence levels, and perceived risks for\nfuture refactorings.\n\n> >> +\tASSERT(pidfd.isValid());\n> >> +\tASSERT(pid > 0);\n> >> +\n> >> +\tsiginfo_t info;\n> >> +\n> >> +\tif (waitid(static_cast(P_PIDFD), pidfd.get(), &info, WNOHANG | WEXITED) >= 0) {\n> >> +\t\tASSERT(info.si_pid == pid);\n> >> \n> >> -\tfinished.emit(exitStatus_, exitCode_);\n> >> +\t\tLOG(Process, Debug)\n> >> +\t\t\t<< this << \"[\" << pid << \":\" << pidfd.get() << \"]\"\n> > \n> > Sounds like a candidate to inherit from Loggable and avoid duplication\n> > of this in log message. You would need to reset pid_ to -1 at the end of\n> > the function though. Except it should be done before emitting the\n> > finished signal, as there's no running_ variable anymore. Annoying... I\n> > wonder if we should keep running_, or if there's a cleaner way.\n> \n> Even with `running_`, I think the same problem applies to `pidFd_` because\n> it has to be closed, but if the `finished` signal handler starts the process\n> again, then it must not be closed. So one would have to duplicate `pidFd_.reset()`\n> in both branches.\n> \n> I hope the duplication in the log message is acceptable because it keeps the\n> actual logic simpler (i.e. reset object state at the beginning unconditionally).\n\nIt would be nice if we could use Loggable, as other messages would then\nalso identify the Process instance. The cost would be a slightly more\ncomplicated logic in this function. It's not a huge deal though if it's\ntoo much effort, just something nice to have. \n\n> > Reviewed-by: Laurent Pinchart \n> > \n> >> +\t\t\t<< \" code: \" << info.si_code\n> >> +\t\t\t<< \" status: \" << info.si_status;\n> >> +\n> >> +\t\texitStatus_ = info.si_code == CLD_EXITED ? Process::NormalExit : Process::SignalExit;\n> >> +\t\texitCode_ = info.si_code == CLD_EXITED ? info.si_status : -1;\n> >> +\n> >> +\t\tfinished.emit(exitStatus_, exitCode_);\n> >> +\t} else {\n> >> +\t\tint err = errno;\n> >> +\t\tLOG(Process, Warning)\n> >> +\t\t\t<< this << \"[\" << pid << \":\" << pidfd.get() << \"]\"\n> >> +\t\t\t<< \" waitid() failed: \" << strerror(err);\n> >> +\t}\n> >> }\n> >> \n> >> /**\n> >> @@ -394,8 +255,8 @@ void Process::died(int wstatus)\n> >> */\n> >> void Process::kill()\n> >> {\n> >> -\tif (pid_ > 0)\n> >> -\t\t::kill(pid_, SIGKILL);\n> >> +\tif (pidfd_.isValid())\n> >> +\t\tsyscall(SYS_pidfd_send_signal, pidfd_.get(), SIGKILL, nullptr, 0);\n> >> }\n> >> \n> >> } /* namespace libcamera */\n> >> diff --git a/test/ipc/unixsocket_ipc.cpp b/test/ipc/unixsocket_ipc.cpp\n> >> index df7d9c2b4..f3e3c09ef 100644\n> >> --- a/test/ipc/unixsocket_ipc.cpp\n> >> +++ b/test/ipc/unixsocket_ipc.cpp\n> >> @@ -209,8 +209,6 @@ protected:\n> >> \t}\n> >> \n> >> private:\n> >> -\tProcessManager processManager_;\n> >> -\n> >> \tunique_ptr ipc_;\n> >> };\n> >> \n> >> diff --git a/test/log/log_process.cpp b/test/log/log_process.cpp\n> >> index 9609e23d5..367b78803 100644\n> >> --- a/test/log/log_process.cpp\n> >> +++ b/test/log/log_process.cpp\n> >> @@ -137,8 +137,6 @@ private:\n> >> \t\texitCode_ = exitCode;\n> >> \t}\n> >> \n> >> -\tProcessManager processManager_;\n> >> -\n> >> \tProcess proc_;\n> >> \tProcess::ExitStatus exitStatus_ = Process::NotExited;\n> >> \tstring logPath_;\n> >> diff --git a/test/process/process_test.cpp b/test/process/process_test.cpp\n> >> index a88d8fef1..fe76666e6 100644\n> >> --- a/test/process/process_test.cpp\n> >> +++ b/test/process/process_test.cpp\n> >> @@ -86,8 +86,6 @@ private:\n> >> \t\texitCode_ = exitCode;\n> >> \t}\n> >> \n> >> -\tProcessManager processManager_;\n> >> -\n> >> \tProcess proc_;\n> >> \tenum Process::ExitStatus exitStatus_;\n> >> \tint exitCode_;","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 DCA27C327D\n\tfor ;\n\tThu, 24 Apr 2025 11:11:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E8F5168ACD;\n\tThu, 24 Apr 2025 13:11:28 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 08D4F68AC5\n\tfor ;\n\tThu, 24 Apr 2025 13:11:27 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 82DA116A;\n\tThu, 24 Apr 2025 13:11:24 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ECmPbaDi\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1745493084;\n\tbh=jy0+aaqHNUbhGPVawLtY9ll0M1bk33p74tbbYQky7wQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ECmPbaDiLxuOfbw/a6oTNgaplPMC4Vx5mWK2pHbvUhadGLgI0zRZ1y+EZubSiGoRF\n\tdweKcM4GSjhepWgr0/ZakjjAyoovvRR7veZAVrP73HISl1gKv/HJnanNjuhweg7uqm\n\tZKr6LI6WHP2vtuQADAkci1rijVJGfjnowZifYToE=","Date":"Thu, 24 Apr 2025 14:11:23 +0300","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v3 9/9] libcamera: process: Remove `ProcessManager`\n\tsingleton","Message-ID":"<20250424111123.GB18085@pendragon.ideasonboard.com>","References":"<20250325180821.1456154-1-barnabas.pocze@ideasonboard.com>\n\t<20250325180821.1456154-10-barnabas.pocze@ideasonboard.com>\n\t<20250326150325.GI8303@pendragon.ideasonboard.com>\n\t<49234340-2aed-4080-a6c2-9e5ed875eb5f@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<49234340-2aed-4080-a6c2-9e5ed875eb5f@ideasonboard.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]