[{"id":35261,"web_url":"https://patchwork.libcamera.org/comment/35261/","msgid":"<630587ad-e1ff-4dba-a21f-0e3cab3ca11b@ideasonboard.com>","date":"2025-07-31T17:26:39","subject":"Re: [PATCH v6 6/6] 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\n2025. 07. 28. 13:36 keltezéssel, Barnabás Pőcze írta:\n> The `ProcessManager` is a singleton class to handle `SIGCHLD` signals\n> and report the exit status to the particular `Process` instance.\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> Reviewed-by: Laurent Pinchart \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 | 246 +++++---------------\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, 61 insertions(+), 228 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 e55d11fa3..27d4b9258 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> @@ -45,41 +44,14 @@ public:\n> private:\n> \tLIBCAMERA_DISABLE_COPY_AND_MOVE(Process)\n> \n> -\tint isolate();\n> -\tvoid died(int wstatus);\n> +\tvoid onPidfdNotify();\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> -\n> -\tEventNotifier *sigEvent_;\n> -\tUniqueFD pipe_[2];\n> +\tUniqueFD pidfd_;\n> +\tstd::unique_ptr pidfdNotify_;\n> };\n> \n> } /* namespace libcamera */\n> diff --git a/meson.build b/meson.build\n> index 6fc4504ab..47efdfcd4 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 ce6e4c380..3090d7568 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(std::vector v)\n> {\n> \tsort(v.begin(), v.end());\n> @@ -117,129 +92,6 @@ void closeAllFdsExcept(std::vector v)\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> @@ -290,8 +142,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> @@ -300,20 +150,29 @@ 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> +\tclone_args cargs = {};\n> +\tint pidfd;\n> +\n> +\tcargs.flags = 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}\n> +\n> +\tif (childPid) {\n> \t\tpid_ = childPid;\n> -\t\tProcessManager::instance()->registerProcess(this);\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> \n> -\t\treturn 0;\n> +\t\tLOG(Process, Debug) << this << \"[\" << childPid << ':' << pidfd << \"]\"\n> +\t\t\t\t << \" forked\";\n> \t} else {\n> -\t\tif (isolate())\n> -\t\t\t_exit(EXIT_FAILURE);\n> -\n> \t\tstd::vector v(fds.begin(), fds.end());\n> \t\tv.push_back(STDERR_FILENO);\n> \t\tcloseAllFdsExcept(std::move(v));\n> @@ -346,35 +205,44 @@ int Process::start(const std::string &path,\n> \n> \t\t_exit(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> +\tASSERT(pidfd.isValid());\n> +\tASSERT(pid > 0);\n> +\n> +\tsiginfo_t info;\n> +\n> +\t/*\n> +\t * `static_cast` is needed because `P_PIDFD` is not defined in `sys/wait.h` if the C standard library\n> +\t * is old enough. So `P_PIDFD` is taken from `linux/wait.h`, where it is just an integer #define.\n> +\t */\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> +\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\nI am not entirely satisfied with this part. There are two things:\n\n (a) could `Process` derive from `Loggable` and put the pid and pidfd in the log\n message prefix? the problematic part there is restarting the process from\n the \"finished\" signal handlers because at that point those fields need to\n be \"unset\";\n (b) and probably more importantly, what should happen if `waitid()` fails? I see\n a couple options:\n (1) abort\n (2) log and ignore\n (3) consider the process \"finished\"\n (1) emit the \"finished\" signal\n (2) do not emit the \"finished\" signal\n\nThe current beaviour is (3.2). (1) is probably too extreme. (2) keeps the pidfd in the\nevent loop so it is likely possible that it'll keep signaling completion and spamming\nthe log.\n\n\nRegards,\nBarnabás Pőcze\n\n\n\n> }\n> \n> /**\n> @@ -412,8 +280,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 D60E3BDCC1\n\tfor ;\n\tThu, 31 Jul 2025 17:26:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E1E2B6921A;\n\tThu, 31 Jul 2025 19:26:45 +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 6159D69154\n\tfor ;\n\tThu, 31 Jul 2025 19:26:44 +0200 (CEST)","from [192.168.33.14] (185.221.140.213.nat.pool.zt.hu\n\t[185.221.140.213])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E01B71741;\n\tThu, 31 Jul 2025 19:25:59 +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=\"rZoqjRCs\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1753982760;\n\tbh=rnj60E23ND1zkAEg4JqvQI1bhPRXC/Zm9skxmqaA460=;\n\th=Date:Subject:From:To:Cc:References:In-Reply-To:From;\n\tb=rZoqjRCstjhYyZgqCeNyPXmnXvMiLpt+jJu/coFNHcv+9ReUJjANntCROdkt4bt65\n\tNCxunIRtaZ8uz25AsQVbrkfNaVRxvUpvY1xwgL5a96BQ1blzrF5De8Tlcp5ZXfFA6R\n\tf3sUoT7ss+nqPRU1Y4nj+ziqyNko3QMh26SFnhyE=","Message-ID":"<630587ad-e1ff-4dba-a21f-0e3cab3ca11b@ideasonboard.com>","Date":"Thu, 31 Jul 2025 19:26:39 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v6 6/6] libcamera: process: Remove `ProcessManager`\n\tsingleton","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","To":"libcamera-devel@lists.libcamera.org","Cc":"Laurent Pinchart ","References":"<20250728113641.238256-1-barnabas.pocze@ideasonboard.com>\n\t<20250728113641.238256-7-barnabas.pocze@ideasonboard.com>","Content-Language":"en-US, hu-HU","In-Reply-To":"<20250728113641.238256-7-barnabas.pocze@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":35271,"web_url":"https://patchwork.libcamera.org/comment/35271/","msgid":"<5f70d5a0-d300-4e4c-bfa5-626f807b7efa@ideasonboard.com>","date":"2025-08-04T13:29:52","subject":"Re: [PATCH v6 6/6] 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":"2025. 07. 31. 19:26 keltezéssel, Barnabás Pőcze írta:\n> Hi\n> \n> 2025. 07. 28. 13:36 keltezéssel, Barnabás Pőcze írta:\n>> The `ProcessManager` is a singleton class to handle `SIGCHLD` signals\n>> and report the exit status to the particular `Process` instance.\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>> Reviewed-by: Laurent Pinchart \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                   | 246 +++++---------------\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, 61 insertions(+), 228 deletions(-)\n>>\n>> [...]\n>> diff --git a/src/libcamera/process.cpp b/src/libcamera/process.cpp\n>> index ce6e4c380..3090d7568 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>> +#include \n>> +#include /* glibc only provides `P_PIDFD` in `sys/wait.h` since 2.36 */\n>> +\n>>   #include \n>>   #include \n>>   #include \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>> -    pid_ = -1;\n>> -    exitStatus_ = WIFEXITED(wstatus) ? NormalExit : SignalExit;\n>> -    exitCode_ = exitStatus_ == NormalExit ? WEXITSTATUS(wstatus) : -1;\n>> +    auto pidfdNotify = std::exchange(pidfdNotify_, {});\n>> +    auto pidfd = std::exchange(pidfd_, {});\n>> +    auto pid = std::exchange(pid_, -1);\n>> +\n>> +    ASSERT(pidfdNotify);\n>> +    ASSERT(pidfd.isValid());\n>> +    ASSERT(pid > 0);\n>> +\n>> +    siginfo_t info;\n>> +\n>> +    /*\n>> +     * `static_cast` is needed because `P_PIDFD` is not defined in `sys/wait.h` if the C standard library\n>> +     * is old enough. So `P_PIDFD` is taken from `linux/wait.h`, where it is just an integer #define.\n>> +     */\n>> +    if (waitid(static_cast(P_PIDFD), pidfd.get(), &info, WNOHANG | WEXITED) >= 0) {\n>> +        ASSERT(info.si_pid == pid);\n>> -    finished.emit(exitStatus_, exitCode_);\n>> +        LOG(Process, Debug)\n>> +            << this << \"[\" << pid << ':' << pidfd.get() << \"]\"\n>> +            << \" code: \" << info.si_code\n>> +            << \" status: \" << info.si_status;\n>> +\n>> +        exitStatus_ = info.si_code == CLD_EXITED ? Process::NormalExit : Process::SignalExit;\n>> +        exitCode_ = info.si_code == CLD_EXITED ? info.si_status : -1;\n>> +\n>> +        finished.emit(exitStatus_, exitCode_);\n>> +    } else {\n>> +        int err = errno;\n>> +        LOG(Process, Warning)\n>> +            << this << \"[\" << pid << \":\" << pidfd.get() << \"]\"\n>> +            << \" waitid() failed: \" << strerror(err);\n>> +    }\n> \n> I am not entirely satisfied with this part. There are two things:\n> \n>   (a) could `Process` derive from `Loggable` and put the pid and pidfd in the log\n>       message prefix? the problematic part there is restarting the process from\n>       the \"finished\" signal handlers because at that point those fields need to\n>       be \"unset\";\n>   (b) and probably more importantly, what should happen if `waitid()` fails? I see\n>       a couple options:\n>         (1) abort\n>         (2) log and ignore\n>         (3) consider the process \"finished\"\n>           (1) emit the \"finished\" signal\n>           (2) do not emit the \"finished\" signal\n> \n> The current beaviour is (3.2). (1) is probably too extreme. (2) keeps the pidfd in the\n> event loop so it is likely possible that it'll keep signaling completion and spamming\n> the log.\n\nAfter some thinking I came to the conclusion that the current approach is acceptable,\nand if unforeseen practical issues arise, then it can be modified accordingly.\nSo I am planning to merge the remaining changes soon.\n\n\n> \n> \n> Regards,\n> Barnabás Pőcze\n> \n> \n> \n>>   }\n>>   /**\n>> @@ -412,8 +280,8 @@ void Process::died(int wstatus)\n>>    */\n>>   void Process::kill()\n>>   {\n>> -    if (pid_ > 0)\n>> -        ::kill(pid_, SIGKILL);\n>> +    if (pidfd_.isValid())\n>> +        syscall(SYS_pidfd_send_signal, pidfd_.get(), SIGKILL, nullptr, 0);\n>>   }\n>>   } /* namespace libcamera */\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 EB505BDCC1\n\tfor ;\n\tMon, 4 Aug 2025 13:29:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 37FA76920D;\n\tMon, 4 Aug 2025 15:29:58 +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 9DFA26920D\n\tfor ;\n\tMon, 4 Aug 2025 15:29:56 +0200 (CEST)","from [192.168.33.13] (185.182.215.213.nat.pool.zt.hu\n\t[185.182.215.213])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5C2A47F5;\n\tMon, 4 Aug 2025 15:29:09 +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=\"K2jmcvur\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1754314149;\n\tbh=CMZBwq+bXXt/LFyJSiHLrDSvkEmp+ZkzA2rV7oDXLhQ=;\n\th=Date:Subject:From:To:Cc:References:In-Reply-To:From;\n\tb=K2jmcvurbBnRvrD6BHyo7CPjNderJvgq/FFErO9gb+3WHbWkiAJsjWCAh4MWXlbEU\n\tWzbRDZkwnm20l6P3PvM3cBzZMohwrUOav9Qjg7eDztfralpORUL+Fp/VdUufFjP7Lz\n\tiEtvUmeW5y+mz8gBfsx64CeWxB6wwNOeYOQW32Lk=","Message-ID":"<5f70d5a0-d300-4e4c-bfa5-626f807b7efa@ideasonboard.com>","Date":"Mon, 4 Aug 2025 15:29:52 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v6 6/6] libcamera: process: Remove `ProcessManager`\n\tsingleton","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","To":"libcamera-devel@lists.libcamera.org","Cc":"Laurent Pinchart ","References":"<20250728113641.238256-1-barnabas.pocze@ideasonboard.com>\n\t<20250728113641.238256-7-barnabas.pocze@ideasonboard.com>\n\t<630587ad-e1ff-4dba-a21f-0e3cab3ca11b@ideasonboard.com>","Content-Language":"en-US, hu-HU","In-Reply-To":"<630587ad-e1ff-4dba-a21f-0e3cab3ca11b@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":35276,"web_url":"https://patchwork.libcamera.org/comment/35276/","msgid":"<20250804204327.GG30355@pendragon.ideasonboard.com>","date":"2025-08-04T20:43:27","subject":"Re: [PATCH v6 6/6] 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 Mon, Aug 04, 2025 at 03:29:52PM +0200, Barnabás Pőcze wrote:\n> 2025. 07. 31. 19:26 keltezéssel, Barnabás Pőcze írta:\n> > 2025. 07. 28. 13:36 keltezéssel, Barnabás Pőcze írta:\n> >> The `ProcessManager` is a singleton class to handle `SIGCHLD` signals\n> >> and report the exit status to the particular `Process` instance.\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> >> Reviewed-by: Laurent Pinchart \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                   | 246 +++++---------------\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, 61 insertions(+), 228 deletions(-)\n> >>\n> >> [...]\n> >> diff --git a/src/libcamera/process.cpp b/src/libcamera/process.cpp\n> >> index ce6e4c380..3090d7568 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> >> +#include \n> >> +#include /* glibc only provides `P_PIDFD` in `sys/wait.h` since 2.36 */\n> >> +\n> >>   #include \n> >>   #include \n> >>   #include \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> >> -    pid_ = -1;\n> >> -    exitStatus_ = WIFEXITED(wstatus) ? NormalExit : SignalExit;\n> >> -    exitCode_ = exitStatus_ == NormalExit ? WEXITSTATUS(wstatus) : -1;\n> >> +    auto pidfdNotify = std::exchange(pidfdNotify_, {});\n> >> +    auto pidfd = std::exchange(pidfd_, {});\n> >> +    auto pid = std::exchange(pid_, -1);\n> >> +\n> >> +    ASSERT(pidfdNotify);\n> >> +    ASSERT(pidfd.isValid());\n> >> +    ASSERT(pid > 0);\n> >> +\n> >> +    siginfo_t info;\n> >> +\n> >> +    /*\n> >> +     * `static_cast` is needed because `P_PIDFD` is not defined in `sys/wait.h` if the C standard library\n> >> +     * is old enough. So `P_PIDFD` is taken from `linux/wait.h`, where it is just an integer #define.\n> >> +     */\n> >> +    if (waitid(static_cast(P_PIDFD), pidfd.get(), &info, WNOHANG | WEXITED) >= 0) {\n> >> +        ASSERT(info.si_pid == pid);\n> >> -    finished.emit(exitStatus_, exitCode_);\n> >> +        LOG(Process, Debug)\n> >> +            << this << \"[\" << pid << ':' << pidfd.get() << \"]\"\n> >> +            << \" code: \" << info.si_code\n> >> +            << \" status: \" << info.si_status;\n> >> +\n> >> +        exitStatus_ = info.si_code == CLD_EXITED ? Process::NormalExit : Process::SignalExit;\n> >> +        exitCode_ = info.si_code == CLD_EXITED ? info.si_status : -1;\n> >> +\n> >> +        finished.emit(exitStatus_, exitCode_);\n> >> +    } else {\n> >> +        int err = errno;\n> >> +        LOG(Process, Warning)\n> >> +            << this << \"[\" << pid << \":\" << pidfd.get() << \"]\"\n> >> +            << \" waitid() failed: \" << strerror(err);\n> >> +    }\n> > \n> > I am not entirely satisfied with this part. There are two things:\n> > \n> >   (a) could `Process` derive from `Loggable` and put the pid and pidfd in the log\n> >       message prefix? the problematic part there is restarting the process from\n> >       the \"finished\" signal handlers because at that point those fields need to\n> >       be \"unset\";\n\nI think I proposed that at some point, and you convinced me that it was\nprobably not worth it given the issue in the finished signal handlers\n:-)\n\n> >   (b) and probably more importantly, what should happen if `waitid()` fails? I see\n> >       a couple options:\n> >         (1) abort\n> >         (2) log and ignore\n> >         (3) consider the process \"finished\"\n> >           (1) emit the \"finished\" signal\n> >           (2) do not emit the \"finished\" signal\n> > \n> > The current beaviour is (3.2). (1) is probably too extreme. (2) keeps the pidfd in the\n> > event loop so it is likely possible that it'll keep signaling completion and spamming\n> > the log.\n> \n> After some thinking I came to the conclusion that the current approach is acceptable,\n> and if unforeseen practical issues arise, then it can be modified accordingly.\n> So I am planning to merge the remaining changes soon.\n\nAck. If waitid() fails I think we're in serious trouble anyway, it's\nhard to decide how to best recover from the issue as we don't foresee it\nhappening in the first place.\n\nEventually I'd like to experiment with IPA module isolation through a\ndaemon, which would remove the need to spawn processes.\n\n> >>   }\n> >>   /**\n> >> @@ -412,8 +280,8 @@ void Process::died(int wstatus)\n> >>    */\n> >>   void Process::kill()\n> >>   {\n> >> -    if (pid_ > 0)\n> >> -        ::kill(pid_, SIGKILL);\n> >> +    if (pidfd_.isValid())\n> >> +        syscall(SYS_pidfd_send_signal, pidfd_.get(), SIGKILL, nullptr, 0);\n> >>   }\n> >>   } /* namespace libcamera */\n> > \n> > [...]","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 30CF5BDCC1\n\tfor ;\n\tMon, 4 Aug 2025 20:43:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 91CBE6920D;\n\tMon, 4 Aug 2025 22:43:42 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AC5A96920D\n\tfor ;\n\tMon, 4 Aug 2025 22:43:41 +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 UTF8SMTPSA id 41BBA446;\n\tMon, 4 Aug 2025 22:42:54 +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=\"iCnWoPon\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1754340174;\n\tbh=eP+Ht1WcwMVQNq6vMb1fsz0PD3e1j6NsznaTSo+ONQQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=iCnWoPonpVzeTro/6TRJRkbZCZinfNCD1+Wc6NJq/S7Jre6lk0rCYRvbwtcLQalDm\n\tHjoBRqNEbciosN3Ya0PcV4ZG/TsZHvpiN/K7MZTOgWlakewS1gJ6cSN66ST6gn9tha\n\tY4jqovUy56d623ofmKrO9iOXpN9ERjJT2amhELuQ=","Date":"Mon, 4 Aug 2025 23:43:27 +0300","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v6 6/6] libcamera: process: Remove `ProcessManager`\n\tsingleton","Message-ID":"<20250804204327.GG30355@pendragon.ideasonboard.com>","References":"<20250728113641.238256-1-barnabas.pocze@ideasonboard.com>\n\t<20250728113641.238256-7-barnabas.pocze@ideasonboard.com>\n\t<630587ad-e1ff-4dba-a21f-0e3cab3ca11b@ideasonboard.com>\n\t<5f70d5a0-d300-4e4c-bfa5-626f807b7efa@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<5f70d5a0-d300-4e4c-bfa5-626f807b7efa@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":35281,"web_url":"https://patchwork.libcamera.org/comment/35281/","msgid":"<01d32016-c727-470b-8bbd-aff7332a48f5@ideasonboard.com>","date":"2025-08-05T10:21:34","subject":"Re: [PATCH v6 6/6] 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":"2025. 08. 04. 22:43 keltezéssel, Laurent Pinchart írta:\n> On Mon, Aug 04, 2025 at 03:29:52PM +0200, Barnabás Pőcze wrote:\n>> 2025. 07. 31. 19:26 keltezéssel, Barnabás Pőcze írta:\n>>> 2025. 07. 28. 13:36 keltezéssel, Barnabás Pőcze írta:\n>>>> The `ProcessManager` is a singleton class to handle `SIGCHLD` signals\n>>>> and report the exit status to the particular `Process` instance.\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>>>> Reviewed-by: Laurent Pinchart \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                   | 246 +++++---------------\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, 61 insertions(+), 228 deletions(-)\n>>>>\n>>>> [...]\n>>>> diff --git a/src/libcamera/process.cpp b/src/libcamera/process.cpp\n>>>> index ce6e4c380..3090d7568 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>>>> +#include \n>>>> +#include /* glibc only provides `P_PIDFD` in `sys/wait.h` since 2.36 */\n>>>> +\n>>>>   #include \n>>>>   #include \n>>>>   #include \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>>>> -    pid_ = -1;\n>>>> -    exitStatus_ = WIFEXITED(wstatus) ? NormalExit : SignalExit;\n>>>> -    exitCode_ = exitStatus_ == NormalExit ? WEXITSTATUS(wstatus) : -1;\n>>>> +    auto pidfdNotify = std::exchange(pidfdNotify_, {});\n>>>> +    auto pidfd = std::exchange(pidfd_, {});\n>>>> +    auto pid = std::exchange(pid_, -1);\n>>>> +\n>>>> +    ASSERT(pidfdNotify);\n>>>> +    ASSERT(pidfd.isValid());\n>>>> +    ASSERT(pid > 0);\n>>>> +\n>>>> +    siginfo_t info;\n>>>> +\n>>>> +    /*\n>>>> +     * `static_cast` is needed because `P_PIDFD` is not defined in `sys/wait.h` if the C standard library\n>>>> +     * is old enough. So `P_PIDFD` is taken from `linux/wait.h`, where it is just an integer #define.\n>>>> +     */\n>>>> +    if (waitid(static_cast(P_PIDFD), pidfd.get(), &info, WNOHANG | WEXITED) >= 0) {\n>>>> +        ASSERT(info.si_pid == pid);\n>>>> -    finished.emit(exitStatus_, exitCode_);\n>>>> +        LOG(Process, Debug)\n>>>> +            << this << \"[\" << pid << ':' << pidfd.get() << \"]\"\n>>>> +            << \" code: \" << info.si_code\n>>>> +            << \" status: \" << info.si_status;\n>>>> +\n>>>> +        exitStatus_ = info.si_code == CLD_EXITED ? Process::NormalExit : Process::SignalExit;\n>>>> +        exitCode_ = info.si_code == CLD_EXITED ? info.si_status : -1;\n>>>> +\n>>>> +        finished.emit(exitStatus_, exitCode_);\n>>>> +    } else {\n>>>> +        int err = errno;\n>>>> +        LOG(Process, Warning)\n>>>> +            << this << \"[\" << pid << \":\" << pidfd.get() << \"]\"\n>>>> +            << \" waitid() failed: \" << strerror(err);\n>>>> +    }\n>>>\n>>> I am not entirely satisfied with this part. There are two things:\n>>>\n>>>   (a) could `Process` derive from `Loggable` and put the pid and pidfd in the log\n>>>       message prefix? the problematic part there is restarting the process from\n>>>       the \"finished\" signal handlers because at that point those fields need to\n>>>       be \"unset\";\n> \n> I think I proposed that at some point, and you convinced me that it was\n> probably not worth it given the issue in the finished signal handlers\n> :-)\n> \n>>>   (b) and probably more importantly, what should happen if `waitid()` fails? I see\n>>>       a couple options:\n>>>         (1) abort\n>>>         (2) log and ignore\n>>>         (3) consider the process \"finished\"\n>>>           (1) emit the \"finished\" signal\n>>>           (2) do not emit the \"finished\" signal\n>>>\n>>> The current beaviour is (3.2). (1) is probably too extreme. (2) keeps the pidfd in the\n>>> event loop so it is likely possible that it'll keep signaling completion and spamming\n>>> the log.\n>>\n>> After some thinking I came to the conclusion that the current approach is acceptable,\n>> and if unforeseen practical issues arise, then it can be modified accordingly.\n>> So I am planning to merge the remaining changes soon.\n> \n> Ack. If waitid() fails I think we're in serious trouble anyway, it's\n> hard to decide how to best recover from the issue as we don't foresee it\n> happening in the first place.\n> \n> Eventually I'd like to experiment with IPA module isolation through a\n> daemon, which would remove the need to spawn processes.\n\nHmm... I would have assumed that the same (or similar) isolation would\nbe desired in that case as well, no?\n\n\n> \n>>>>   }\n>>>>   /**\n>>>> @@ -412,8 +280,8 @@ void Process::died(int wstatus)\n>>>>    */\n>>>>   void Process::kill()\n>>>>   {\n>>>> -    if (pid_ > 0)\n>>>> -        ::kill(pid_, SIGKILL);\n>>>> +    if (pidfd_.isValid())\n>>>> +        syscall(SYS_pidfd_send_signal, pidfd_.get(), SIGKILL, nullptr, 0);\n>>>>   }\n>>>>   } /* namespace libcamera */\n>>>\n>>> [...]\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 707FEBDCC1\n\tfor ;\n\tTue, 5 Aug 2025 10:21:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 56FAD6921E;\n\tTue, 5 Aug 2025 12:21:44 +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 DD13C69210\n\tfor ;\n\tTue, 5 Aug 2025 12:21:42 +0200 (CEST)","from [192.168.33.10] (185.182.215.213.nat.pool.zt.hu\n\t[185.182.215.213])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7E3451775;\n\tTue, 5 Aug 2025 12:20:53 +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=\"ZpOyDJ3g\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1754389255;\n\tbh=K8QATI9ZURySd6M7zP6JUZQkrEv/rYi+xXmdM7PZbeQ=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=ZpOyDJ3gWAMxCb8YWE0M2K2JbZuHgTTc1wIQxoHsC9CIViu5j3yftaF0yVDVUQpZj\n\tni9U1o9sBo1VDfYGnXPzg6j/YWoK24z6KaJLsZJHiryy8wclhCeU/0xXTpXoxVWOnT\n\tivTSdw3dwS31T8OEXeiNLD/flM8dcaVUKsiUuXdk=","Message-ID":"<01d32016-c727-470b-8bbd-aff7332a48f5@ideasonboard.com>","Date":"Tue, 5 Aug 2025 12:21:34 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v6 6/6] libcamera: process: Remove `ProcessManager`\n\tsingleton","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20250728113641.238256-1-barnabas.pocze@ideasonboard.com>\n\t<20250728113641.238256-7-barnabas.pocze@ideasonboard.com>\n\t<630587ad-e1ff-4dba-a21f-0e3cab3ca11b@ideasonboard.com>\n\t<5f70d5a0-d300-4e4c-bfa5-626f807b7efa@ideasonboard.com>\n\t<20250804204327.GG30355@pendragon.ideasonboard.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<20250804204327.GG30355@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":35282,"web_url":"https://patchwork.libcamera.org/comment/35282/","msgid":"<20250805102758.GA16330@pendragon.ideasonboard.com>","date":"2025-08-05T10:27:58","subject":"Re: [PATCH v6 6/6] 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 Tue, Aug 05, 2025 at 12:21:34PM +0200, Barnabás Pőcze wrote:\n> 2025. 08. 04. 22:43 keltezéssel, Laurent Pinchart írta:\n> > On Mon, Aug 04, 2025 at 03:29:52PM +0200, Barnabás Pőcze wrote:\n> >> 2025. 07. 31. 19:26 keltezéssel, Barnabás Pőcze írta:\n> >>> 2025. 07. 28. 13:36 keltezéssel, Barnabás Pőcze írta:\n> >>>> The `ProcessManager` is a singleton class to handle `SIGCHLD` signals\n> >>>> and report the exit status to the particular `Process` instance.\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> >>>> Reviewed-by: Laurent Pinchart \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                   | 246 +++++---------------\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, 61 insertions(+), 228 deletions(-)\n> >>>>\n> >>>> [...]\n> >>>> diff --git a/src/libcamera/process.cpp b/src/libcamera/process.cpp\n> >>>> index ce6e4c380..3090d7568 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> >>>> +#include \n> >>>> +#include /* glibc only provides `P_PIDFD` in `sys/wait.h` since 2.36 */\n> >>>> +\n> >>>>   #include \n> >>>>   #include \n> >>>>   #include \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> >>>> -    pid_ = -1;\n> >>>> -    exitStatus_ = WIFEXITED(wstatus) ? NormalExit : SignalExit;\n> >>>> -    exitCode_ = exitStatus_ == NormalExit ? WEXITSTATUS(wstatus) : -1;\n> >>>> +    auto pidfdNotify = std::exchange(pidfdNotify_, {});\n> >>>> +    auto pidfd = std::exchange(pidfd_, {});\n> >>>> +    auto pid = std::exchange(pid_, -1);\n> >>>> +\n> >>>> +    ASSERT(pidfdNotify);\n> >>>> +    ASSERT(pidfd.isValid());\n> >>>> +    ASSERT(pid > 0);\n> >>>> +\n> >>>> +    siginfo_t info;\n> >>>> +\n> >>>> +    /*\n> >>>> +     * `static_cast` is needed because `P_PIDFD` is not defined in `sys/wait.h` if the C standard library\n> >>>> +     * is old enough. So `P_PIDFD` is taken from `linux/wait.h`, where it is just an integer #define.\n> >>>> +     */\n> >>>> +    if (waitid(static_cast(P_PIDFD), pidfd.get(), &info, WNOHANG | WEXITED) >= 0) {\n> >>>> +        ASSERT(info.si_pid == pid);\n> >>>> -    finished.emit(exitStatus_, exitCode_);\n> >>>> +        LOG(Process, Debug)\n> >>>> +            << this << \"[\" << pid << ':' << pidfd.get() << \"]\"\n> >>>> +            << \" code: \" << info.si_code\n> >>>> +            << \" status: \" << info.si_status;\n> >>>> +\n> >>>> +        exitStatus_ = info.si_code == CLD_EXITED ? Process::NormalExit : Process::SignalExit;\n> >>>> +        exitCode_ = info.si_code == CLD_EXITED ? info.si_status : -1;\n> >>>> +\n> >>>> +        finished.emit(exitStatus_, exitCode_);\n> >>>> +    } else {\n> >>>> +        int err = errno;\n> >>>> +        LOG(Process, Warning)\n> >>>> +            << this << \"[\" << pid << \":\" << pidfd.get() << \"]\"\n> >>>> +            << \" waitid() failed: \" << strerror(err);\n> >>>> +    }\n> >>>\n> >>> I am not entirely satisfied with this part. There are two things:\n> >>>\n> >>>   (a) could `Process` derive from `Loggable` and put the pid and pidfd in the log\n> >>>       message prefix? the problematic part there is restarting the process from\n> >>>       the \"finished\" signal handlers because at that point those fields need to\n> >>>       be \"unset\";\n> > \n> > I think I proposed that at some point, and you convinced me that it was\n> > probably not worth it given the issue in the finished signal handlers\n> > :-)\n> > \n> >>>   (b) and probably more importantly, what should happen if `waitid()` fails? I see\n> >>>       a couple options:\n> >>>         (1) abort\n> >>>         (2) log and ignore\n> >>>         (3) consider the process \"finished\"\n> >>>           (1) emit the \"finished\" signal\n> >>>           (2) do not emit the \"finished\" signal\n> >>>\n> >>> The current beaviour is (3.2). (1) is probably too extreme. (2) keeps the pidfd in the\n> >>> event loop so it is likely possible that it'll keep signaling completion and spamming\n> >>> the log.\n> >>\n> >> After some thinking I came to the conclusion that the current approach is acceptable,\n> >> and if unforeseen practical issues arise, then it can be modified accordingly.\n> >> So I am planning to merge the remaining changes soon.\n> > \n> > Ack. If waitid() fails I think we're in serious trouble anyway, it's\n> > hard to decide how to best recover from the issue as we don't foresee it\n> > happening in the first place.\n> > \n> > Eventually I'd like to experiment with IPA module isolation through a\n> > daemon, which would remove the need to spawn processes.\n> \n> Hmm... I would have assumed that the same (or similar) isolation would\n> be desired in that case as well, no?\n\nIt will, but not as part of libcamera itself. I expect we'll make use of\noff-the-shelf isolation solutions then (but we'll still need to write a\ndaemon, which will likely still need to spawn processes).\n\n> >>>>   }\n> >>>>   /**\n> >>>> @@ -412,8 +280,8 @@ void Process::died(int wstatus)\n> >>>>    */\n> >>>>   void Process::kill()\n> >>>>   {\n> >>>> -    if (pid_ > 0)\n> >>>> -        ::kill(pid_, SIGKILL);\n> >>>> +    if (pidfd_.isValid())\n> >>>> +        syscall(SYS_pidfd_send_signal, pidfd_.get(), SIGKILL, nullptr, 0);\n> >>>>   }\n> >>>>   } /* namespace libcamera */\n> >>>\n> >>> [...]","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id A5A47BE086\n\tfor ;\n\tTue, 5 Aug 2025 10:28:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BEB496921A;\n\tTue, 5 Aug 2025 12:28:14 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DEABE69210\n\tfor ;\n\tTue, 5 Aug 2025 12:28:12 +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 UTF8SMTPSA id 098EE1775; \n\tTue, 5 Aug 2025 12:27: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=\"PIK8XKEl\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1754389645;\n\tbh=/evaGnESFDvbhfQ/V5aFUq9zkXGN8CgZc+EjPZPVBRw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=PIK8XKEluOFLeP3WO66w7iBZlLrt1BV+CT6khMRk1m5qdwfaXlcp/2fP6DlgiIl/c\n\tzYBamfOJpMW6eHJDEzsS/z2nIPpfc9bS6OuEPcGymN2sBrCVrqvpjBZ+ZumGKnJxrP\n\tlErKspLPOEI1pwGW90H+mW7fghNQbJrjBA+AO8Xs=","Date":"Tue, 5 Aug 2025 13:27:58 +0300","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v6 6/6] libcamera: process: Remove `ProcessManager`\n\tsingleton","Message-ID":"<20250805102758.GA16330@pendragon.ideasonboard.com>","References":"<20250728113641.238256-1-barnabas.pocze@ideasonboard.com>\n\t<20250728113641.238256-7-barnabas.pocze@ideasonboard.com>\n\t<630587ad-e1ff-4dba-a21f-0e3cab3ca11b@ideasonboard.com>\n\t<5f70d5a0-d300-4e4c-bfa5-626f807b7efa@ideasonboard.com>\n\t<20250804204327.GG30355@pendragon.ideasonboard.com>\n\t<01d32016-c727-470b-8bbd-aff7332a48f5@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<01d32016-c727-470b-8bbd-aff7332a48f5@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\" "}}]