Patch Detail
Show a patch.
GET /api/patches/23251/?format=api
{ "id": 23251, "url": "https://patchwork.libcamera.org/api/patches/23251/?format=api", "web_url": "https://patchwork.libcamera.org/patch/23251/", "project": { "id": 1, "url": "https://patchwork.libcamera.org/api/projects/1/?format=api", "name": "libcamera", "link_name": "libcamera", "list_id": "libcamera_core", "list_email": "libcamera-devel@lists.libcamera.org", "web_url": "", "scm_url": "", "webscm_url": "" }, "msgid": "<20250424112203.445351-10-barnabas.pocze@ideasonboard.com>", "date": "2025-04-24T11:22:02", "name": "[RFC,v4,9/9] libcamera: process: Remove `ProcessManager` singleton", "commit_ref": null, "pull_url": null, "state": "superseded", "archived": false, "hash": "37de2927fe98c2061c7803f738a4a39cf8aaa671", "submitter": { "id": 216, "url": "https://patchwork.libcamera.org/api/people/216/?format=api", "name": "Barnabás Pőcze", "email": "barnabas.pocze@ideasonboard.com" }, "delegate": null, "mbox": "https://patchwork.libcamera.org/patch/23251/mbox/", "series": [ { "id": 5144, "url": "https://patchwork.libcamera.org/api/series/5144/?format=api", "web_url": "https://patchwork.libcamera.org/project/libcamera/list/?series=5144", "date": "2025-04-24T11:21:53", "name": "libcamera: process: Remove `ProcessManager` singleton", "version": 4, "mbox": "https://patchwork.libcamera.org/series/5144/mbox/" } ], "comments": "https://patchwork.libcamera.org/api/patches/23251/comments/", "check": "pending", "checks": "https://patchwork.libcamera.org/api/patches/23251/checks/", "tags": {}, "headers": { "Return-Path": "<libcamera-devel-bounces@lists.libcamera.org>", "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 C1933C3324\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu, 24 Apr 2025 11:22:29 +0000 (UTC)", "from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7601168AD7;\n\tThu, 24 Apr 2025 13:22:24 +0200 (CEST)", "from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 70F1668AD8\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 24 Apr 2025 13:22:09 +0200 (CEST)", "from pb-laptop.local (185.221.143.16.nat.pool.zt.hu\n\t[185.221.143.16])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 685C316A\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 24 Apr 2025 13:22:07 +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=\"eC/KueJZ\"; dkim-atps=neutral", "DKIM-Signature": "v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1745493727;\n\tbh=dNQ7Uo2BUTUD18/Av4Yqlj30bUMQ3mo9KsMR0e/491g=;\n\th=From:To:Subject:Date:In-Reply-To:References:From;\n\tb=eC/KueJZrECT1QUtqY7ui33KJUIuzv5iSbjzRkgt1/ZzIp/VkfU2uM1q3W6eh4Av5\n\tzCUCitMyOOTCRl/uu3fQbxS83aXINclQa5xCJVygnT3ekL3ziJVXGC3xQl7EC0/12T\n\t/FzayGJihu6+BfVljvpjDXx9c4kYiELeD5fJ7yDk=", "From": "=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= <barnabas.pocze@ideasonboard.com>", "To": "libcamera-devel@lists.libcamera.org", "Subject": "[RFC PATCH v4 9/9] libcamera: process: Remove `ProcessManager`\n\tsingleton", "Date": "Thu, 24 Apr 2025 13:22:02 +0200", "Message-ID": "<20250424112203.445351-10-barnabas.pocze@ideasonboard.com>", "X-Mailer": "git-send-email 2.49.0", "In-Reply-To": "<20250424112203.445351-1-barnabas.pocze@ideasonboard.com>", "References": "<20250424112203.445351-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": "<libcamera-devel.lists.libcamera.org>", "List-Unsubscribe": "<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>", "List-Archive": "<https://lists.libcamera.org/pipermail/libcamera-devel/>", "List-Post": "<mailto:libcamera-devel@lists.libcamera.org>", "List-Help": "<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>", "List-Subscribe": "<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>", "Errors-To": "libcamera-devel-bounces@lists.libcamera.org", "Sender": "\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>" }, "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 <barnabas.pocze@ideasonboard.com>\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<DeviceEnumerator> enumerator_;\n \n \tstd::unique_ptr<IPAManager> 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 <signal.h>\n #include <string>\n \n #include <libcamera/base/class.h>\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<Process *> processes_;\n-\n-\tstruct sigaction oldsa_;\n-\n-\tEventNotifier *sigEvent_;\n-\tUniqueFD pipe_[2];\n+\tUniqueFD pidfd_;\n+\tstd::unique_ptr<EventNotifier> pidfdNotify_;\n };\n \n } /* namespace libcamera */\ndiff --git a/meson.build b/meson.build\nindex 1525bdf4a..83c4b87de 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 42b56374d..124677038 100644\n--- a/src/libcamera/process.cpp\n+++ b/src/libcamera/process.cpp\n@@ -10,15 +10,19 @@\n #include <algorithm>\n #include <dirent.h>\n #include <fcntl.h>\n-#include <list>\n+#include <sched.h>\n #include <signal.h>\n #include <string.h>\n #include <sys/socket.h>\n+#include <sys/syscall.h>\n #include <sys/types.h>\n #include <sys/wait.h>\n #include <unistd.h>\n #include <vector>\n \n+#include <linux/sched.h>\n+#include <linux/wait.h> /* glibc only provides `P_PIDFD` in `sys/wait.h` since 2.36 */\n+\n #include <libcamera/base/event_notifier.h>\n #include <libcamera/base/log.h>\n #include <libcamera/base/utils.h>\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<const int> fds)\n {\n \tstd::vector<int> v(fds.begin(), fds.end());\n@@ -118,129 +93,6 @@ void closeAllFdsExcept(Span<const int> 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@@ -291,8 +143,6 @@ int Process::start(const std::string &path,\n \t\t Span<const std::string> args,\n \t\t Span<const int> fds)\n {\n-\tint ret;\n-\n \tif (pid_ > 0)\n \t\treturn -EBUSY;\n \n@@ -301,20 +151,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_CLEAR_SIGHAND | CLONE_PIDFD | CLONE_NEWUSER | CLONE_NEWNET;\n+\tcargs.pidfd = reinterpret_cast<uintptr_t>(&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<EventNotifier>(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\tcloseAllFdsExcept(fds);\n \n \t\tconst char *file = utils::secure_getenv(\"LIBCAMERA_LOG_FILE\");\n@@ -333,35 +192,44 @@ 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+\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<idtype_t>(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@@ -399,8 +267,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<IPCPipeUnixSocket> 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": [ "RFC", "v4", "9/9" ] }