{"id":23996,"url":"https://patchwork.libcamera.org/api/patches/23996/?format=json","web_url":"https://patchwork.libcamera.org/patch/23996/","project":{"id":1,"url":"https://patchwork.libcamera.org/api/projects/1/?format=json","name":"libcamera","link_name":"libcamera","list_id":"libcamera_core","list_email":"libcamera-devel@lists.libcamera.org","web_url":"","scm_url":"","webscm_url":""},"msgid":"<20250728113641.238256-7-barnabas.pocze@ideasonboard.com>","date":"2025-07-28T11:36:41","name":"[v6,6/6] libcamera: process: Remove `ProcessManager` singleton","commit_ref":null,"pull_url":null,"state":"accepted","archived":false,"hash":"d41d79943233a1889fe366de445b2645c2237c44","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/?format=json","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"delegate":null,"mbox":"https://patchwork.libcamera.org/patch/23996/mbox/","series":[{"id":5342,"url":"https://patchwork.libcamera.org/api/series/5342/?format=json","web_url":"https://patchwork.libcamera.org/project/libcamera/list/?series=5342","date":"2025-07-28T11:36:35","name":"libcamera: process: Remove `ProcessManager` singleton","version":6,"mbox":"https://patchwork.libcamera.org/series/5342/mbox/"}],"comments":"https://patchwork.libcamera.org/api/patches/23996/comments/","check":"pending","checks":"https://patchwork.libcamera.org/api/patches/23996/checks/","tags":{},"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 445B8C332D\n\tfor ;\n\tMon, 28 Jul 2025 11:37:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 77AE569166;\n\tMon, 28 Jul 2025 13:37:01 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 54B4869156\n\tfor ;\n\tMon, 28 Jul 2025 13:36:47 +0200 (CEST)","from pb-laptop.local (185.221.140.39.nat.pool.zt.hu\n\t[185.221.140.39])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5D61AF04;\n\tMon, 28 Jul 2025 13:36: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=\"lUO94To7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1753702565;\n\tbh=HdGVmHtZmISTLUWBO6S9V+zq0ItNCWWBxgMgHs1unZI=;\n\th=From:To:Cc:Subject:Date:In-Reply-To:References:From;\n\tb=lUO94To7I6dUF1YUK8D5dOg7Qf6GrYnsMofvY/1//2tfIYWkBOYvaLluqtPkgutyv\n\tTHsHm0O6NUP0axXM1qo3ZhROs3FYdD1eYFvG4qcQyWIIuAEzc4ZwfARf0/xbzibQNX\n\t0o0zZftlzBbMkwNZkzV+5wwNIHlfFxGtXUMyzmBo=","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","To":"libcamera-devel@lists.libcamera.org","Cc":"Laurent Pinchart ","Subject":"[PATCH v6 6/6] libcamera: process: Remove `ProcessManager` singleton","Date":"Mon, 28 Jul 2025 13:36:41 +0200","Message-ID":"<20250728113641.238256-7-barnabas.pocze@ideasonboard.com>","X-Mailer":"git-send-email 2.50.1","In-Reply-To":"<20250728113641.238256-1-barnabas.pocze@ideasonboard.com>","References":"<20250728113641.238256-1-barnabas.pocze@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=UTF-8","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\" "},"content":"The `ProcessManager` is a singleton class to handle `SIGCHLD` signals\nand report the exit status to the particular `Process` instance.\n\nHowever, having a singleton in a library is not favourable and it is\neven less favourable if it installs a signal handler.\n\nUsing pidfd it is possible to avoid the need for the signal handler;\nand the `Process` objects can watch their pidfd themselves, eliminating\nthe need for the `ProcessManager` class altogether.\n\n`P_PIDFD` for `waitid()` was introduced in Linux 5.4, so this change\nraises the minimum supported kernel version. `clone3()`, `CLONE_PIDFD`,\n`pidfd_send_signal()` were all introduced earlier.\n\nFurthermore, the call to the `unshare()` system call can be removed\nas those options can be passed to `clone3()` directly.\n\nSigned-off-by: Barnabás Pőcze \nReviewed-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(-)","diff":"diff --git a/include/libcamera/internal/camera_manager.h b/include/libcamera/internal/camera_manager.h\nindex 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 */\ndiff --git a/include/libcamera/internal/process.h b/include/libcamera/internal/process.h\nindex 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 */\ndiff --git a/meson.build b/meson.build\nindex 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.'\ndiff --git a/src/libcamera/process.cpp b/src/libcamera/process.cpp\nindex 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 }\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 */\ndiff --git a/test/ipc/unixsocket_ipc.cpp b/test/ipc/unixsocket_ipc.cpp\nindex 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 \ndiff --git a/test/log/log_process.cpp b/test/log/log_process.cpp\nindex 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_;\ndiff --git a/test/process/process_test.cpp b/test/process/process_test.cpp\nindex 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","prefixes":["v6","6/6"]}