Patch Detail
Show a patch.
GET /api/1.1/patches/22522/?format=api
{ "id": 22522, "url": "https://patchwork.libcamera.org/api/1.1/patches/22522/?format=api", "web_url": "https://patchwork.libcamera.org/patch/22522/", "project": { "id": 1, "url": "https://patchwork.libcamera.org/api/1.1/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": "<20250111113443.167822-1-pobrn@protonmail.com>", "date": "2025-01-11T11:34:45", "name": "[RFC,v1] libcamera: process: Remove `ProcessManager` singleton", "commit_ref": null, "pull_url": null, "state": "superseded", "archived": false, "hash": "1d86628675b4cc20646244170ac3c084289ced5b", "submitter": { "id": 133, "url": "https://patchwork.libcamera.org/api/1.1/people/133/?format=api", "name": "Pőcze Barnabás", "email": "pobrn@protonmail.com" }, "delegate": null, "mbox": "https://patchwork.libcamera.org/patch/22522/mbox/", "series": [ { "id": 4942, "url": "https://patchwork.libcamera.org/api/1.1/series/4942/?format=api", "web_url": "https://patchwork.libcamera.org/project/libcamera/list/?series=4942", "date": "2025-01-11T11:34:45", "name": "[RFC,v1] libcamera: process: Remove `ProcessManager` singleton", "version": 1, "mbox": "https://patchwork.libcamera.org/series/4942/mbox/" } ], "comments": "https://patchwork.libcamera.org/api/patches/22522/comments/", "check": "pending", "checks": "https://patchwork.libcamera.org/api/patches/22522/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 16056C32DA\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSat, 11 Jan 2025 11:34:52 +0000 (UTC)", "from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F2850684EA;\n\tSat, 11 Jan 2025 12:34:50 +0100 (CET)", "from mail-40133.protonmail.ch (mail-40133.protonmail.ch\n\t[185.70.40.133])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D1901684D0\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSat, 11 Jan 2025 12:34:49 +0100 (CET)" ], "Authentication-Results": "lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=protonmail.com header.i=@protonmail.com\n\theader.b=\"wTDCcpr5\"; dkim-atps=neutral", "DKIM-Signature": "v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1736595289; x=1736854489;\n\tbh=TlXSy6oHcX32SClKZlPKvo6Apnjobmg9Zvd56Tnt3+s=;\n\th=Date:To:From:Subject:Message-ID:Feedback-ID:From:To:Cc:Date:\n\tSubject:Reply-To:Feedback-ID:Message-ID:BIMI-Selector:\n\tList-Unsubscribe:List-Unsubscribe-Post;\n\tb=wTDCcpr5vZyX4P1M+Nqe95k32x2M4WY5U+0rZPdcU7K9c+jRx59gNO2RLVjM7Xo3P\n\tfA1B5zWIHLJ1VT4fCPNV6ad1afqcRnIcIFzjwV0/f+bab6m7MY0qeCnL62WSQ/YSvw\n\t5ZP3jaAufobRwuzhIN5S2mDXn5lbkq7BaMPqQRGTqfflDyflnEhbDadmzbRZ4LJJ8N\n\tHz8/X66gB+2Kw5Mao2U8RNw/cv/9SRj18q3gO9ufAgY06d0xQTm06NxAnDaQFNmdLE\n\tgFvtsGeAwt+KjKVLGLB8XaenGbZVrGzkmk4RArIhd+d23nCpTaurAziVffLIXwECnG\n\tluR2mJPVM0kLg==", "Date": "Sat, 11 Jan 2025 11:34:45 +0000", "To": "libcamera-devel@lists.libcamera.org", "From": "=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= <pobrn@protonmail.com>", "Subject": "[RFC PATCH v1] libcamera: process: Remove `ProcessManager` singleton", "Message-ID": "<20250111113443.167822-1-pobrn@protonmail.com>", "Feedback-ID": "20568564:user:proton", "X-Pm-Message-ID": "e3a9e08e791b58a78e9404bb66324364fd40b67d", "MIME-Version": "1.0", "Content-Type": "text/plain; charset=utf-8", "Content-Transfer-Encoding": "quoted-printable", "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` instances.\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`clone3()` was introduced in Linux 5.3, so this change raises the\nminimum supported kernel version.\n\nSigned-off-by: Barnabás Pőcze <pobrn@protonmail.com>\n---\n include/libcamera/internal/process.h | 39 +---\n src/libcamera/process.cpp | 315 ++++++++-------------------\n 2 files changed, 91 insertions(+), 263 deletions(-)", "diff": "diff --git a/include/libcamera/internal/process.h b/include/libcamera/internal/process.h\nindex b1d07a5a5..9d476f3bf 100644\n--- a/include/libcamera/internal/process.h\n+++ b/include/libcamera/internal/process.h\n@@ -12,6 +12,7 @@\n #include <vector>\n \n #include <libcamera/base/signal.h>\n+#include <libcamera/base/span.h>\n #include <libcamera/base/unique_fd.h>\n \n namespace libcamera {\n@@ -31,8 +32,8 @@ public:\n \t~Process();\n \n \tint start(const std::string &path,\n-\t\t const std::vector<std::string> &args = std::vector<std::string>(),\n-\t\t const std::vector<int> &fds = std::vector<int>());\n+\t\t Span<const std::string> args = {},\n+\t\t Span<const int> fds = {});\n \n \tExitStatus exitStatus() const { return exitStatus_; }\n \tint exitCode() const { return exitCode_; }\n@@ -42,43 +43,13 @@ public:\n \tSignal<enum ExitStatus, int> finished;\n \n private:\n-\tvoid closeAllFdsExcept(const std::vector<int> &fds);\n-\tint isolate();\n-\tvoid died(int wstatus);\n-\n \tpid_t pid_;\n \tbool running_;\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/src/libcamera/process.cpp b/src/libcamera/process.cpp\nindex bc9833f41..e828be2ae 100644\n--- a/src/libcamera/process.cpp\n+++ b/src/libcamera/process.cpp\n@@ -19,6 +19,11 @@\n #include <unistd.h>\n #include <vector>\n \n+#include <linux/sched.h>\n+#include <sched.h>\n+#include <sys/syscall.h>\n+#include <unistd.h>\n+\n #include <libcamera/base/event_notifier.h>\n #include <libcamera/base/log.h>\n #include <libcamera/base/utils.h>\n@@ -32,162 +37,6 @@ 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-} /* 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@@ -218,6 +67,36 @@ Process::~Process()\n \t/* \\todo wait for child process to exit */\n }\n \n+namespace {\n+\n+void closeAllFdsExcept(Span<const int> fds)\n+{\n+\tstd::vector<int> v(fds.begin(), fds.end());\n+\tsort(v.begin(), v.end());\n+\n+\tDIR *dir = opendir(\"/proc/self/fd\");\n+\tif (!dir)\n+\t\treturn;\n+\n+\tint dfd = dirfd(dir);\n+\n+\tstruct dirent *ent;\n+\twhile ((ent = readdir(dir)) != nullptr) {\n+\t\tchar *endp;\n+\t\tint fd = strtoul(ent->d_name, &endp, 10);\n+\t\tif (*endp)\n+\t\t\tcontinue;\n+\n+\t\tif (fd >= 0 && fd != dfd &&\n+\t\t !std::binary_search(v.begin(), v.end(), fd))\n+\t\t\tclose(fd);\n+\t}\n+\n+\tclosedir(dir);\n+}\n+\n+} /* namespace */\n+\n /**\n * \\brief Fork and exec a process, and close fds\n * \\param[in] path Path to executable\n@@ -235,104 +114,82 @@ Process::~Process()\n * or a negative error code otherwise\n */\n int Process::start(const std::string &path,\n-\t\t const std::vector<std::string> &args,\n-\t\t const std::vector<int> &fds)\n+\t\t Span<const std::string> args,\n+\t\t Span<const int> fds)\n {\n-\tint ret;\n-\n \tif (running_)\n \t\treturn 0;\n \n-\tint childPid = fork();\n-\tif (childPid == -1) {\n-\t\tret = -errno;\n+\tint pidfd;\n+\t::clone_args cargs = {};\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\tpid_ = childPid;\n-\t\tProcessManager::instance()->registerProcess(this);\n+\t}\n \n+\tif (childPid) {\n \t\trunning_ = true;\n+\t\tpid_ = childPid;\n+\t\tpidfd_ = UniqueFD(pidfd);\n+\t\tpidfdNotify_ = std::make_unique<EventNotifier>(pidfd_.get(), EventNotifier::Type::Read);\n \n-\t\treturn 0;\n-\t} else {\n-\t\tif (isolate())\n-\t\t\t_exit(EXIT_FAILURE);\n+\t\tpidfdNotify_->activated.connect(this, [this] {\n+\t\t\t::siginfo_t info;\n+\n+\t\t\trunning_ = false;\n+\n+\t\t\tif (::waitid(P_PIDFD, pidfd_.get(), &info, WNOHANG | WEXITED) >= 0) {\n+\t\t\t\tASSERT(info.si_signo == SIGCHLD);\n+\t\t\t\tASSERT(info.si_pid == pid_);\n+\n+\t\t\t\tLOG(Process, Debug)\n+\t\t\t\t\t<< this << \"[\" << pid_ << \":\" << pidfd_.get() << \"] \"\n+\t\t\t\t\t\"code:\" << info.si_code << \" status:\" << info.si_status;\n+\n+\t\t\t\texitStatus_ = info.si_code == CLD_EXITED ? Process::NormalExit : Process::SignalExit;\n+\t\t\t\texitCode_ = info.si_code == CLD_EXITED ? info.si_status : -1;\n+\n+\t\t\t\tfinished.emit(exitStatus_, exitCode_);\n+\t\t\t} else {\n+\t\t\t\tint err = errno;\n+\t\t\t\tLOG(Process, Warning)\n+\t\t\t\t\t<< this << \"[\" << pid_ << \":\" << pidfd_.get() << \"] \"\n+\t\t\t\t\t\"waitid() failed: \" << strerror(err);\n+\t\t\t}\n \n+\t\t\tpid_ = -1;\n+\t\t\tpidfd_.reset();\n+\t\t\tpidfdNotify_.release();\n+\t\t});\n+\t} else {\n \t\tcloseAllFdsExcept(fds);\n \n \t\tconst char *file = utils::secure_getenv(\"LIBCAMERA_LOG_FILE\");\n \t\tif (file && strcmp(file, \"syslog\"))\n \t\t\tunsetenv(\"LIBCAMERA_LOG_FILE\");\n \n-\t\tconst char **argv = new const char *[args.size() + 2];\n-\t\tunsigned int len = args.size();\n+\t\tconst size_t len = args.size();\n+\t\tconst char **argv = new const char *[len + 2];\n \t\targv[0] = path.c_str();\n-\t\tfor (unsigned int i = 0; i < len; i++)\n+\t\tfor (size_t i = 0; i < len; i++)\n \t\t\targv[i + 1] = args[i].c_str();\n \t\targv[len + 1] = nullptr;\n \n-\t\texecv(path.c_str(), (char **)argv);\n+\t\texecv(path.c_str(), const_cast<char * const *>(argv));\n \n \t\texit(EXIT_FAILURE);\n \t}\n-}\n-\n-void Process::closeAllFdsExcept(const std::vector<int> &fds)\n-{\n-\tstd::vector<int> v(fds);\n-\tsort(v.begin(), v.end());\n-\n-\tDIR *dir = opendir(\"/proc/self/fd\");\n-\tif (!dir)\n-\t\treturn;\n-\n-\tint dfd = dirfd(dir);\n-\n-\tstruct dirent *ent;\n-\twhile ((ent = readdir(dir)) != nullptr) {\n-\t\tchar *endp;\n-\t\tint fd = strtoul(ent->d_name, &endp, 10);\n-\t\tif (*endp)\n-\t\t\tcontinue;\n-\n-\t\tif (fd >= 0 && fd != dfd &&\n-\t\t !std::binary_search(v.begin(), v.end(), fd))\n-\t\t\tclose(fd);\n-\t}\n-\n-\tclosedir(dir);\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-{\n-\trunning_ = false;\n-\texitStatus_ = WIFEXITED(wstatus) ? NormalExit : SignalExit;\n-\texitCode_ = exitStatus_ == NormalExit ? WEXITSTATUS(wstatus) : -1;\n-\n-\tfinished.emit(exitStatus_, exitCode_);\n-}\n-\n /**\n * \\fn Process::exitStatus()\n * \\brief Retrieve the exit status of the process\n@@ -368,8 +225,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\t::syscall(SYS_pidfd_send_signal, pidfd_.get(), SIGKILL, nullptr, 0);\n }\n \n } /* namespace libcamera */\n", "prefixes": [ "RFC", "v1" ] }