[{"id":33129,"web_url":"https://patchwork.libcamera.org/comment/33129/","msgid":"<20250121235119.GA28510@pendragon.ideasonboard.com>","date":"2025-01-21T23:51:19","subject":"Re: [RFC PATCH v2] libcamera: process: Remove `ProcessManager`\n\tsingleton","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Barnabás,\n\nThank you for the patch.\n\nOn Tue, Jan 21, 2025 at 06:57:44PM +0000, Barnabás Pőcze wrote:\n> The `ProcessManager` is a singleton class to handle `SIGCHLD` signals\n> and report the exit status to the particular `Process` instances.\n> \n> However, having a singleton in a library is not favourable and it is\n> even less favourable if it installs a signal handler.\n> \n> Using pidfd it is possible to avoid the need for the signal handler;\n> and the `Process` objects can watch their pidfd themselves, eliminating\n> the need for the `ProcessManager` class altogether.\n\nThis patch does a few other things: it replaces vector arguments to\nstart() with spans, and make the closeAllFdsExcept() member function\nglobal. Splitting it in separate patches will make it easier to review.\n\nAdditionally, usage of clone3() allows dropping the unshare() call, and\nthat would be useful to mention in the commit message to facilitate\nreviews.\n\n> `clone3()` was introduced in Linux 5.3, so this change raises the\n> minimum supported kernel version.\n\nYou should update the kernel_version_req variable in the main\nmeson.build file.\n\n> \n> Signed-off-by: Barnabás Pőcze \n> ---\n> include/libcamera/internal/camera_manager.h | 1 -\n> include/libcamera/internal/process.h | 39 +--\n> src/libcamera/process.cpp | 315 ++++++--------------\n> test/ipc/unixsocket_ipc.cpp | 2 -\n> test/log/log_process.cpp | 2 -\n> test/process/process_test.cpp | 2 -\n> 6 files changed, 91 insertions(+), 270 deletions(-)\n\nVery nice simplification overall :-)\n\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 b1d07a5a5..9d476f3bf 100644\n> --- a/include/libcamera/internal/process.h\n> +++ b/include/libcamera/internal/process.h\n> @@ -12,6 +12,7 @@\n> #include \n> \n> #include \n> +#include \n> #include \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 &args = std::vector(),\n> -\t\t const std::vector &fds = std::vector());\n> +\t\t Span args = {},\n> +\t\t Span fds = {});\n> \n> \tExitStatus exitStatus() const { return exitStatus_; }\n> \tint exitCode() const { return exitCode_; }\n> @@ -42,43 +43,13 @@ public:\n> \tSignal finished;\n> \n> private:\n> -\tvoid closeAllFdsExcept(const std::vector &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 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/src/libcamera/process.cpp b/src/libcamera/process.cpp\n> index bc9833f41..2b059ed92 100644\n> --- a/src/libcamera/process.cpp\n> +++ b/src/libcamera/process.cpp\n> @@ -19,6 +19,11 @@\n> #include \n> #include \n> \n> +#include \n> +#include \n> +#include \n> +#include \n\nThose headers should be added to the previous group of headers. You're\nduplicating unistd.h here.\n\n> +\n> #include \n> #include \n> #include \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 fds)\n\nAny particular reason not to keep this as a member function ?\n\n> +{\n> +\tstd::vector 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 &args,\n> -\t\t const std::vector &fds)\n> +\t\t Span args,\n> +\t\t Span 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(&pidfd);\n> +\tcargs.exit_signal = SIGCHLD;\n\nDo we need to deliver SIGCHLD to the parent, now that we use the pidfd\nto be notified of termination of the child ?\n\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\nMaybe s/fork/clone/\n\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(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\nIs it worth checking this ? The man page tells si_signo is always set to\nSIGCHLD.\n\n> +\t\t\t\tASSERT(info.si_pid == pid_);\n\nSomething would be seriously wrong if this assertion fails, I'm not sure\nit's needed.\n\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\nLet's not split strings in the middle, and add missing spaces:\n\n\t\t\t\t\t<< this << \"[\" << pid_ << \":\" << pidfd_.get()\n\t\t\t\t\t<< \"] code: \" << info.si_code << \" status: \"\n\t\t\t\t\t<< info.si_status;\n\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_.reset();\n\nCould all this be a member function instead of a lambda please ? Using a\nlambda function make the code more difficult to read.\n\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(argv));\n\nAll of this could also be done in a separate patch for misc cleanups.\n\n> \n> \t\texit(EXIT_FAILURE);\n> \t}\n> -}\n> -\n> -void Process::closeAllFdsExcept(const std::vector &fds)\n> -{\n> -\tstd::vector 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> 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 e9f5e7e9b..75de9563c 100644\n> --- a/test/process/process_test.cpp\n> +++ b/test/process/process_test.cpp\n> @@ -87,8 +87,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 2C99FC3213\n\tfor ;\n\tTue, 21 Jan 2025 23:51:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2E03468558;\n\tWed, 22 Jan 2025 00:51:30 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E7D4661879\n\tfor ;\n\tWed, 22 Jan 2025 00:51:27 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CA2E466B;\n\tWed, 22 Jan 2025 00:50:24 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Na6bWNXx\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1737503425;\n\tbh=hVCO2Si9iD/HeQ59SJqUJ6O2ArIFmzeBjpzDpNUTvC8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Na6bWNXxKezH4v3WkjjUOrQn3RHeyaUD2T4Y9HG8NcX3bDU+2kKUJZKm+viEkJyMD\n\tNfBvUtGkjNL/F10FfEX5VvmdPc04ZufpoF3Scq0XDeT5cjanJ+HqvMpoJAfmAoKR1i\n\tuwBPotBW5Ifx5/j4NeP1D7Q6RiqCpAB7sswT9pEQ=","Date":"Wed, 22 Jan 2025 01:51:19 +0200","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v2] libcamera: process: Remove `ProcessManager`\n\tsingleton","Message-ID":"<20250121235119.GA28510@pendragon.ideasonboard.com>","References":"<20250121185741.302256-1-pobrn@protonmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20250121185741.302256-1-pobrn@protonmail.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":33130,"web_url":"https://patchwork.libcamera.org/comment/33130/","msgid":"","date":"2025-01-22T08:53:26","subject":"Re: [RFC PATCH v2] libcamera: process: Remove `ProcessManager`\n\tsingleton","submitter":{"id":133,"url":"https://patchwork.libcamera.org/api/people/133/","name":"Pőcze Barnabás","email":"pobrn@protonmail.com"},"content":"2025. január 22., szerda 0:51 keltezéssel, Laurent Pinchart írta:\n\n> Hi Barnabás,\n> \n> Thank you for the patch.\n> \n> On Tue, Jan 21, 2025 at 06:57:44PM +0000, Barnabás Pőcze wrote:\n> > The `ProcessManager` is a singleton class to handle `SIGCHLD` signals\n> > and report the exit status to the particular `Process` instances.\n> >\n> > However, having a singleton in a library is not favourable and it is\n> > even less favourable if it installs a signal handler.\n> >\n> > Using pidfd it is possible to avoid the need for the signal handler;\n> > and the `Process` objects can watch their pidfd themselves, eliminating\n> > the need for the `ProcessManager` class altogether.\n> \n> This patch does a few other things: it replaces vector arguments to\n> start() with spans, and make the closeAllFdsExcept() member function\n> global. Splitting it in separate patches will make it easier to review.\n> \n> Additionally, usage of clone3() allows dropping the unshare() call, and\n> that would be useful to mention in the commit message to facilitate\n> reviews.\n> \n> > `clone3()` was introduced in Linux 5.3, so this change raises the\n> > minimum supported kernel version.\n> \n> You should update the kernel_version_req variable in the main\n> meson.build file.\n\nSorry, yes, I know it is far from perfect, I just wanted to see if this approach\nis acceptable (with the kernel version requirement increase, etc.) before I\nspend more time on this.\n\n\nRegards,\nBarnabás Pőcze\n\n\n> \n> >\n> > Signed-off-by: Barnabás Pőcze \n> > ---\n> > include/libcamera/internal/camera_manager.h | 1 -\n> > include/libcamera/internal/process.h | 39 +--\n> > src/libcamera/process.cpp | 315 ++++++--------------\n> > test/ipc/unixsocket_ipc.cpp | 2 -\n> > test/log/log_process.cpp | 2 -\n> > test/process/process_test.cpp | 2 -\n> > 6 files changed, 91 insertions(+), 270 deletions(-)\n> \n> Very nice simplification overall :-)\n> \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 b1d07a5a5..9d476f3bf 100644\n> > --- a/include/libcamera/internal/process.h\n> > +++ b/include/libcamera/internal/process.h\n> > @@ -12,6 +12,7 @@\n> > #include \n> >\n> > #include \n> > +#include \n> > #include \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 &args = std::vector(),\n> > -\t\t const std::vector &fds = std::vector());\n> > +\t\t Span args = {},\n> > +\t\t Span fds = {});\n> >\n> > \tExitStatus exitStatus() const { return exitStatus_; }\n> > \tint exitCode() const { return exitCode_; }\n> > @@ -42,43 +43,13 @@ public:\n> > \tSignal finished;\n> >\n> > private:\n> > -\tvoid closeAllFdsExcept(const std::vector &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 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/src/libcamera/process.cpp b/src/libcamera/process.cpp\n> > index bc9833f41..2b059ed92 100644\n> > --- a/src/libcamera/process.cpp\n> > +++ b/src/libcamera/process.cpp\n> > @@ -19,6 +19,11 @@\n> > #include \n> > #include \n> >\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> \n> Those headers should be added to the previous group of headers. You're\n> duplicating unistd.h here.\n> \n> > +\n> > #include \n> > #include \n> > #include \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 fds)\n> \n> Any particular reason not to keep this as a member function ?\n> \n> > +{\n> > +\tstd::vector 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 &args,\n> > -\t\t const std::vector &fds)\n> > +\t\t Span args,\n> > +\t\t Span 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(&pidfd);\n> > +\tcargs.exit_signal = SIGCHLD;\n> \n> Do we need to deliver SIGCHLD to the parent, now that we use the pidfd\n> to be notified of termination of the child ?\n> \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> \n> Maybe s/fork/clone/\n> \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(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> \n> Is it worth checking this ? The man page tells si_signo is always set to\n> SIGCHLD.\n> \n> > +\t\t\t\tASSERT(info.si_pid == pid_);\n> \n> Something would be seriously wrong if this assertion fails, I'm not sure\n> it's needed.\n> \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> Let's not split strings in the middle, and add missing spaces:\n> \n> \t\t\t\t\t<< this << \"[\" << pid_ << \":\" << pidfd_.get()\n> \t\t\t\t\t<< \"] code: \" << info.si_code << \" status: \"\n> \t\t\t\t\t<< info.si_status;\n> \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_.reset();\n> \n> Could all this be a member function instead of a lambda please ? Using a\n> lambda function make the code more difficult to read.\n> \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(argv));\n> \n> All of this could also be done in a separate patch for misc cleanups.\n> \n> >\n> > \t\texit(EXIT_FAILURE);\n> > \t}\n> > -}\n> > -\n> > -void Process::closeAllFdsExcept(const std::vector &fds)\n> > -{\n> > -\tstd::vector 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> > 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 e9f5e7e9b..75de9563c 100644\n> > --- a/test/process/process_test.cpp\n> > +++ b/test/process/process_test.cpp\n> > @@ -87,8 +87,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> \n> --\n> Regards,\n> \n> Laurent Pinchart\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 D1FA6C3200\n\tfor ;\n\tWed, 22 Jan 2025 08:53:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B2F8A68558;\n\tWed, 22 Jan 2025 09:53:34 +0100 (CET)","from mail-4322.protonmail.ch (mail-4322.protonmail.ch\n\t[185.70.43.22])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6F9CD6187C\n\tfor ;\n\tWed, 22 Jan 2025 09:53:32 +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=\"BVkCKsfo\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1737536011; x=1737795211;\n\tbh=YmbJOCtEEQDyCinLNb9FGL4t7wfY4Ltd6mibMeL4a2g=;\n\th=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References:\n\tFeedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID:\n\tMessage-ID:BIMI-Selector:List-Unsubscribe:List-Unsubscribe-Post;\n\tb=BVkCKsfoW6TmKMi8dSZX9vGOqS9ASPcBVMrCGAw5qZwpXoXCUl2Hd22pw6Kqy3T8P\n\ta/MpKY0YJqLlCmq1dX94+UiJZ2BLcOzbpD8YYYa8/zTkzVuHYIVSqbgN2tDxVEttvU\n\thmX4NbQ5v3wDUicA+eD1jwQsj170RSoadrtr6IpSP5BIqo2rRtro1Dq66axq1AglhH\n\tqVzWqUUry+3LmJ1nb6whgTWtvC7G7f6xwzoffLZdUEUT+YO9rOeqM2D2vj16O8Y0tJ\n\tvGNTz/qfKL8boSrZiLajnK8dPaIyd7p5fT0kEHjzDq3s6AsZ90czdOXYJF41MG/CFH\n\tsM2S90+3LLNEA==","Date":"Wed, 22 Jan 2025 08:53:26 +0000","To":"Laurent Pinchart ","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v2] libcamera: process: Remove `ProcessManager`\n\tsingleton","Message-ID":"","In-Reply-To":"<20250121235119.GA28510@pendragon.ideasonboard.com>","References":"<20250121185741.302256-1-pobrn@protonmail.com>\n\t<20250121235119.GA28510@pendragon.ideasonboard.com>","Feedback-ID":"20568564:user:proton","X-Pm-Message-ID":"e05f30d759255f9c04913e6e5f043be4b3288bc0","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":"","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":33136,"web_url":"https://patchwork.libcamera.org/comment/33136/","msgid":"<20250122101731.GC28510@pendragon.ideasonboard.com>","date":"2025-01-22T10:17:31","subject":"Re: [RFC PATCH v2] 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 Wed, Jan 22, 2025 at 08:53:26AM +0000, Barnabás Pőcze wrote:\n> 2025. január 22., szerda 0:51 keltezéssel, Laurent Pinchart írta:\n> > On Tue, Jan 21, 2025 at 06:57:44PM +0000, Barnabás Pőcze wrote:\n> > > The `ProcessManager` is a singleton class to handle `SIGCHLD` signals\n> > > and report the exit status to the particular `Process` instances.\n> > >\n> > > However, having a singleton in a library is not favourable and it is\n> > > even less favourable if it installs a signal handler.\n> > >\n> > > Using pidfd it is possible to avoid the need for the signal handler;\n> > > and the `Process` objects can watch their pidfd themselves, eliminating\n> > > the need for the `ProcessManager` class altogether.\n> > \n> > This patch does a few other things: it replaces vector arguments to\n> > start() with spans, and make the closeAllFdsExcept() member function\n> > global. Splitting it in separate patches will make it easier to review.\n> > \n> > Additionally, usage of clone3() allows dropping the unshare() call, and\n> > that would be useful to mention in the commit message to facilitate\n> > reviews.\n> > \n> > > `clone3()` was introduced in Linux 5.3, so this change raises the\n> > > minimum supported kernel version.\n> > \n> > You should update the kernel_version_req variable in the main\n> > meson.build file.\n> \n> Sorry, yes, I know it is far from perfect, I just wanted to see if this approach\n> is acceptable (with the kernel version requirement increase, etc.) before I\n> spend more time on this.\n\nI like this patch very much :-) I would be very happy to see the\nProcessManager singleton before removed.\n\n> > > Signed-off-by: Barnabás Pőcze \n> > > ---\n> > > include/libcamera/internal/camera_manager.h | 1 -\n> > > include/libcamera/internal/process.h | 39 +--\n> > > src/libcamera/process.cpp | 315 ++++++--------------\n> > > test/ipc/unixsocket_ipc.cpp | 2 -\n> > > test/log/log_process.cpp | 2 -\n> > > test/process/process_test.cpp | 2 -\n> > > 6 files changed, 91 insertions(+), 270 deletions(-)\n> > \n> > Very nice simplification overall :-)\n> > \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 b1d07a5a5..9d476f3bf 100644\n> > > --- a/include/libcamera/internal/process.h\n> > > +++ b/include/libcamera/internal/process.h\n> > > @@ -12,6 +12,7 @@\n> > > #include \n> > >\n> > > #include \n> > > +#include \n> > > #include \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 &args = std::vector(),\n> > > -\t\t const std::vector &fds = std::vector());\n> > > +\t\t Span args = {},\n> > > +\t\t Span fds = {});\n> > >\n> > > \tExitStatus exitStatus() const { return exitStatus_; }\n> > > \tint exitCode() const { return exitCode_; }\n> > > @@ -42,43 +43,13 @@ public:\n> > > \tSignal finished;\n> > >\n> > > private:\n> > > -\tvoid closeAllFdsExcept(const std::vector &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 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/src/libcamera/process.cpp b/src/libcamera/process.cpp\n> > > index bc9833f41..2b059ed92 100644\n> > > --- a/src/libcamera/process.cpp\n> > > +++ b/src/libcamera/process.cpp\n> > > @@ -19,6 +19,11 @@\n> > > #include \n> > > #include \n> > >\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > \n> > Those headers should be added to the previous group of headers. You're\n> > duplicating unistd.h here.\n> > \n> > > +\n> > > #include \n> > > #include \n> > > #include \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 fds)\n> > \n> > Any particular reason not to keep this as a member function ?\n> > \n> > > +{\n> > > +\tstd::vector 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 &args,\n> > > -\t\t const std::vector &fds)\n> > > +\t\t Span args,\n> > > +\t\t Span 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(&pidfd);\n> > > +\tcargs.exit_signal = SIGCHLD;\n> > \n> > Do we need to deliver SIGCHLD to the parent, now that we use the pidfd\n> > to be notified of termination of the child ?\n> > \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> > \n> > Maybe s/fork/clone/\n> > \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(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> > \n> > Is it worth checking this ? The man page tells si_signo is always set to\n> > SIGCHLD.\n> > \n> > > +\t\t\t\tASSERT(info.si_pid == pid_);\n> > \n> > Something would be seriously wrong if this assertion fails, I'm not sure\n> > it's needed.\n> > \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> > Let's not split strings in the middle, and add missing spaces:\n> > \n> > \t\t\t\t\t<< this << \"[\" << pid_ << \":\" << pidfd_.get()\n> > \t\t\t\t\t<< \"] code: \" << info.si_code << \" status: \"\n> > \t\t\t\t\t<< info.si_status;\n> > \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_.reset();\n> > \n> > Could all this be a member function instead of a lambda please ? Using a\n> > lambda function make the code more difficult to read.\n> > \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(argv));\n> > \n> > All of this could also be done in a separate patch for misc cleanups.\n> > \n> > >\n> > > \t\texit(EXIT_FAILURE);\n> > > \t}\n> > > -}\n> > > -\n> > > -void Process::closeAllFdsExcept(const std::vector &fds)\n> > > -{\n> > > -\tstd::vector 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> > > 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 e9f5e7e9b..75de9563c 100644\n> > > --- a/test/process/process_test.cpp\n> > > +++ b/test/process/process_test.cpp\n> > > @@ -87,8 +87,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 DC6D9BD16B\n\tfor ;\n\tWed, 22 Jan 2025 10:17:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A57F36855A;\n\tWed, 22 Jan 2025 11:17:43 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 188C86187C\n\tfor ;\n\tWed, 22 Jan 2025 11:17:41 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 42597C16;\n\tWed, 22 Jan 2025 11:16:37 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"J0CXf7Lf\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1737540997;\n\tbh=GrM70aRXtGKiEpOXd7rPGegJUnfkSP3/ic+QvadfFRA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=J0CXf7Lfl+JoJNnVS/wRLbHefwgKiDkhmvSpHZxjdKZuFP5L3AiEN6oMcSLQBPZiP\n\t0CKQUUqaxF3LmuN2LEXRop9qnq5l4i5hLsWZ9inDqT3m98vfO3BjwfLcvhBJdkTIbM\n\tC7ksx3iM2gvxz+F4iIyMtO1FaJsGRqE9I6rRgU2E=","Date":"Wed, 22 Jan 2025 12:17:31 +0200","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v2] libcamera: process: Remove `ProcessManager`\n\tsingleton","Message-ID":"<20250122101731.GC28510@pendragon.ideasonboard.com>","References":"<20250121185741.302256-1-pobrn@protonmail.com>\n\t<20250121235119.GA28510@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","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":33689,"web_url":"https://patchwork.libcamera.org/comment/33689/","msgid":"<29a32ed6-8712-468b-98cb-31b2532579ca@ideasonboard.com>","date":"2025-03-24T11:46:05","subject":"Re: [RFC PATCH v2] 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. 01. 22. 0:51 keltezéssel, Laurent Pinchart írta:\n> Hi Barnabás,\n> \n> Thank you for the patch.\n> \n> On Tue, Jan 21, 2025 at 06:57:44PM +0000, Barnabás Pőcze wrote:\n>> The `ProcessManager` is a singleton class to handle `SIGCHLD` signals\n>> and report the exit status to the particular `Process` instances.\n>>\n>> However, having a singleton in a library is not favourable and it is\n>> even less favourable if it installs a signal handler.\n>>\n>> Using pidfd it is possible to avoid the need for the signal handler;\n>> and the `Process` objects can watch their pidfd themselves, eliminating\n>> the need for the `ProcessManager` class altogether.\n> \n> This patch does a few other things: it replaces vector arguments to\n> start() with spans, and make the closeAllFdsExcept() member function\n> global. Splitting it in separate patches will make it easier to review.\n> \n> Additionally, usage of clone3() allows dropping the unshare() call, and\n> that would be useful to mention in the commit message to facilitate\n> reviews.\n> \n>> `clone3()` was introduced in Linux 5.3, so this change raises the\n>> minimum supported kernel version.\n> \n> You should update the kernel_version_req variable in the main\n> meson.build file.\n> \n>>\n>> Signed-off-by: Barnabás Pőcze \n>> ---\n>> include/libcamera/internal/camera_manager.h | 1 -\n>> include/libcamera/internal/process.h | 39 +--\n>> src/libcamera/process.cpp | 315 ++++++--------------\n>> test/ipc/unixsocket_ipc.cpp | 2 -\n>> test/log/log_process.cpp | 2 -\n>> test/process/process_test.cpp | 2 -\n>> 6 files changed, 91 insertions(+), 270 deletions(-)\n> \n> Very nice simplification overall :-)\n> \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 b1d07a5a5..9d476f3bf 100644\n>> --- a/include/libcamera/internal/process.h\n>> +++ b/include/libcamera/internal/process.h\n>> @@ -12,6 +12,7 @@\n>> #include \n>> \n>> #include \n>> +#include \n>> #include \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 &args = std::vector(),\n>> -\t\t const std::vector &fds = std::vector());\n>> +\t\t Span args = {},\n>> +\t\t Span fds = {});\n>> \n>> \tExitStatus exitStatus() const { return exitStatus_; }\n>> \tint exitCode() const { return exitCode_; }\n>> @@ -42,43 +43,13 @@ public:\n>> \tSignal finished;\n>> \n>> private:\n>> -\tvoid closeAllFdsExcept(const std::vector &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 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/src/libcamera/process.cpp b/src/libcamera/process.cpp\n>> index bc9833f41..2b059ed92 100644\n>> --- a/src/libcamera/process.cpp\n>> +++ b/src/libcamera/process.cpp\n>> @@ -19,6 +19,11 @@\n>> #include \n>> #include \n>> \n>> +#include \n>> +#include \n>> +#include \n>> +#include \n> \n> Those headers should be added to the previous group of headers. You're\n> duplicating unistd.h here.\n> \n>> +\n>> #include \n>> #include \n>> #include \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 fds)\n> \n> Any particular reason not to keep this as a member function ?\n> \n>> +{\n>> +\tstd::vector 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 &args,\n>> -\t\t const std::vector &fds)\n>> +\t\t Span args,\n>> +\t\t Span 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(&pidfd);\n>> +\tcargs.exit_signal = SIGCHLD;\n> \n> Do we need to deliver SIGCHLD to the parent, now that we use the pidfd\n> to be notified of termination of the child ?\n\nI found that setting that is seemingly needed for gdb to work as expected,\nbut I did not look further into it. glibc's `pidfd_spawn()` also sets it.\n\n\n> \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> \n> Maybe s/fork/clone/\n\nI left it as \"fork\" because my assumption was that more people are familiar\nwith the concept of \"fork\", so I thought the potential error message will be\neasier to understand.\n\n\n> \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(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> \n> Is it worth checking this ? The man page tells si_signo is always set to\n> SIGCHLD.\n> \n>> +\t\t\t\tASSERT(info.si_pid == pid_);\n> \n> Something would be seriously wrong if this assertion fails, I'm not sure\n> it's needed.\n\nI suppose that's fair, but I'd like to keep at least the second one.\n\n\n> \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> Let's not split strings in the middle, and add missing spaces:\n> \n> \t\t\t\t\t<< this << \"[\" << pid_ << \":\" << pidfd_.get()\n> \t\t\t\t\t<< \"] code: \" << info.si_code << \" status: \"\n> \t\t\t\t\t<< info.si_status;\n> \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_.reset();\n> \n> Could all this be a member function instead of a lambda please ? Using a\n> lambda function make the code more difficult to read.\n\nI like lambdas a lot. :( My \"issue\" is that private member functions bleed\ninto the public interface, e.g. triggering recompilations on changes that are\ncompletely private and irrelevant to the outside. This was also the motivation\nfor moving `closeAllFdsExcept()`.\n\n\n> \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(argv));\n> \n> All of this could also be done in a separate patch for misc cleanups.\n> \n>> \n>> \t\texit(EXIT_FAILURE);\n>> \t}\n>> -}\n>> -\n>> -void Process::closeAllFdsExcept(const std::vector &fds)\n>> -{\n>> -\tstd::vector 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>> 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 e9f5e7e9b..75de9563c 100644\n>> --- a/test/process/process_test.cpp\n>> +++ b/test/process/process_test.cpp\n>> @@ -87,8 +87,6 @@ private:\n>> \t\texitCode_ = exitCode;\n>> \t}\n>> \n>> -\tProcessManager processManager_;\n>> -\n>> \tProcess proc_;\n>> \tenum Process::ExitStatus exitStatus_;\n>> \tint exitCode_;\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id B097AC0DA4\n\tfor ;\n\tMon, 24 Mar 2025 11:46:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9BC6868945;\n\tMon, 24 Mar 2025 12:46:11 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 04D966893F\n\tfor ;\n\tMon, 24 Mar 2025 12:46:10 +0100 (CET)","from [192.168.33.23] (185.221.143.221.nat.pool.zt.hu\n\t[185.221.143.221])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3797C455;\n\tMon, 24 Mar 2025 12:44:23 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"NpmcR59T\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1742816663;\n\tbh=sACbqO5kbJWT0LqCTWYJxp+4lePGlPfDLmwd6dmkltg=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=NpmcR59TrcpkjWsOt8eL7O+rA2Ibcr/brUhp3clXYaV3AvqqFyhme4I6P4tmhJm3N\n\tL/uEAUIWr7IYv6igIJntX0VC7Y/D0WKyr9TFLQQDRV2+olPcbEOqPZiNIVgrVWn4gS\n\tOwcWO/AewPfKSo7B8leWz5mupUt7DQGjoDOJvk0g=","Message-ID":"<29a32ed6-8712-468b-98cb-31b2532579ca@ideasonboard.com>","Date":"Mon, 24 Mar 2025 12:46:05 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [RFC PATCH v2] libcamera: process: Remove `ProcessManager`\n\tsingleton","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20250121185741.302256-1-pobrn@protonmail.com>\n\t<20250121235119.GA28510@pendragon.ideasonboard.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<20250121235119.GA28510@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":33692,"web_url":"https://patchwork.libcamera.org/comment/33692/","msgid":"<20250324154123.GG5113@pendragon.ideasonboard.com>","date":"2025-03-24T15:41:23","subject":"Re: [RFC PATCH v2] libcamera: process: Remove `ProcessManager`\n\tsingleton","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Barnabás,\n\nOn Mon, Mar 24, 2025 at 12:46:05PM +0100, Barnabás Pőcze wrote:\n> 2025. 01. 22. 0:51 keltezéssel, Laurent Pinchart írta:\n> > On Tue, Jan 21, 2025 at 06:57:44PM +0000, Barnabás Pőcze wrote:\n> >> The `ProcessManager` is a singleton class to handle `SIGCHLD` signals\n> >> and report the exit status to the particular `Process` instances.\n> >>\n> >> However, having a singleton in a library is not favourable and it is\n> >> even less favourable if it installs a signal handler.\n> >>\n> >> Using pidfd it is possible to avoid the need for the signal handler;\n> >> and the `Process` objects can watch their pidfd themselves, eliminating\n> >> the need for the `ProcessManager` class altogether.\n> > \n> > This patch does a few other things: it replaces vector arguments to\n> > start() with spans, and make the closeAllFdsExcept() member function\n> > global. Splitting it in separate patches will make it easier to review.\n> > \n> > Additionally, usage of clone3() allows dropping the unshare() call, and\n> > that would be useful to mention in the commit message to facilitate\n> > reviews.\n> > \n> >> `clone3()` was introduced in Linux 5.3, so this change raises the\n> >> minimum supported kernel version.\n> > \n> > You should update the kernel_version_req variable in the main\n> > meson.build file.\n> > \n> >>\n> >> Signed-off-by: Barnabás Pőcze \n> >> ---\n> >> include/libcamera/internal/camera_manager.h | 1 -\n> >> include/libcamera/internal/process.h | 39 +--\n> >> src/libcamera/process.cpp | 315 ++++++--------------\n> >> test/ipc/unixsocket_ipc.cpp | 2 -\n> >> test/log/log_process.cpp | 2 -\n> >> test/process/process_test.cpp | 2 -\n> >> 6 files changed, 91 insertions(+), 270 deletions(-)\n> > \n> > Very nice simplification overall :-)\n> > \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 b1d07a5a5..9d476f3bf 100644\n> >> --- a/include/libcamera/internal/process.h\n> >> +++ b/include/libcamera/internal/process.h\n> >> @@ -12,6 +12,7 @@\n> >> #include \n> >> \n> >> #include \n> >> +#include \n> >> #include \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 &args = std::vector(),\n> >> -\t\t const std::vector &fds = std::vector());\n> >> +\t\t Span args = {},\n> >> +\t\t Span fds = {});\n> >> \n> >> \tExitStatus exitStatus() const { return exitStatus_; }\n> >> \tint exitCode() const { return exitCode_; }\n> >> @@ -42,43 +43,13 @@ public:\n> >> \tSignal finished;\n> >> \n> >> private:\n> >> -\tvoid closeAllFdsExcept(const std::vector &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 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/src/libcamera/process.cpp b/src/libcamera/process.cpp\n> >> index bc9833f41..2b059ed92 100644\n> >> --- a/src/libcamera/process.cpp\n> >> +++ b/src/libcamera/process.cpp\n> >> @@ -19,6 +19,11 @@\n> >> #include \n> >> #include \n> >> \n> >> +#include \n> >> +#include \n> >> +#include \n> >> +#include \n> > \n> > Those headers should be added to the previous group of headers. You're\n> > duplicating unistd.h here.\n> > \n> >> +\n> >> #include \n> >> #include \n> >> #include \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 fds)\n> > \n> > Any particular reason not to keep this as a member function ?\n> > \n> >> +{\n> >> +\tstd::vector 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 &args,\n> >> -\t\t const std::vector &fds)\n> >> +\t\t Span args,\n> >> +\t\t Span 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(&pidfd);\n> >> +\tcargs.exit_signal = SIGCHLD;\n> > \n> > Do we need to deliver SIGCHLD to the parent, now that we use the pidfd\n> > to be notified of termination of the child ?\n> \n> I found that setting that is seemingly needed for gdb to work as expected,\n> but I did not look further into it. glibc's `pidfd_spawn()` also sets it.\n\nI was wondering if we could omit it to avoid applications receiving the\nsignal unexpectedly for the processes we spawn, but if it breaks\nsomething, we can keep it.\n\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> > \n> > Maybe s/fork/clone/\n> \n> I left it as \"fork\" because my assumption was that more people are familiar\n> with the concept of \"fork\", so I thought the potential error message will be\n> easier to understand.\n> \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(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> > \n> > Is it worth checking this ? The man page tells si_signo is always set to\n> > SIGCHLD.\n> > \n> >> +\t\t\t\tASSERT(info.si_pid == pid_);\n> > \n> > Something would be seriously wrong if this assertion fails, I'm not sure\n> > it's needed.\n> \n> I suppose that's fair, but I'd like to keep at least the second one.\n\nOut of curiosity, do you expect it could ever happen ?\n\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> > Let's not split strings in the middle, and add missing spaces:\n> > \n> > \t\t\t\t\t<< this << \"[\" << pid_ << \":\" << pidfd_.get()\n> > \t\t\t\t\t<< \"] code: \" << info.si_code << \" status: \"\n> > \t\t\t\t\t<< info.si_status;\n> > \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_.reset();\n> > \n> > Could all this be a member function instead of a lambda please ? Using a\n> > lambda function make the code more difficult to read.\n> \n> I like lambdas a lot. :( My \"issue\" is that private member functions bleed\n> into the public interface, e.g. triggering recompilations on changes that are\n> completely private and irrelevant to the outside. This was also the motivation\n> for moving `closeAllFdsExcept()`.\n\nI understand that, especially for public APIs, but in this case the\nimpact on readability far outweights the gains we would get from\navoiding recompilation in my opinion.\n\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(argv));\n> > \n> > All of this could also be done in a separate patch for misc cleanups.\n> > \n> >> \n> >> \t\texit(EXIT_FAILURE);\n> >> \t}\n> >> -}\n> >> -\n> >> -void Process::closeAllFdsExcept(const std::vector &fds)\n> >> -{\n> >> -\tstd::vector 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> >> 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 e9f5e7e9b..75de9563c 100644\n> >> --- a/test/process/process_test.cpp\n> >> +++ b/test/process/process_test.cpp\n> >> @@ -87,8 +87,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 253C4C0DA4\n\tfor ;\n\tMon, 24 Mar 2025 15:41:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 437A768951;\n\tMon, 24 Mar 2025 16:41:47 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7A5236893F\n\tfor ;\n\tMon, 24 Mar 2025 16:41:46 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B55DF455;\n\tMon, 24 Mar 2025 16:39:59 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"hOcpXsu2\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1742830799;\n\tbh=yWv8Q9ePwqfCa8kuWHaBLt3JozrG4PX6+WXghIWAI5k=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=hOcpXsu2U7DemaxCJp0QtAbqwU7OnN3LVrdwkV68cxW1idzzEdyUKxIMcl0/2PpeF\n\tLP7r4SGJQI4CIZdCNve/vQVKW+vMewQ/Z2quC+FN9UzFGfBgYfSiDTP39lYAqd+2mt\n\tDNeIwSJ/C7TZ7JeL63wErk78IyD2G4MFW6WZ1DaQ=","Date":"Mon, 24 Mar 2025 17:41:23 +0200","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v2] libcamera: process: Remove `ProcessManager`\n\tsingleton","Message-ID":"<20250324154123.GG5113@pendragon.ideasonboard.com>","References":"<20250121185741.302256-1-pobrn@protonmail.com>\n\t<20250121235119.GA28510@pendragon.ideasonboard.com>\n\t<29a32ed6-8712-468b-98cb-31b2532579ca@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<29a32ed6-8712-468b-98cb-31b2532579ca@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":33693,"web_url":"https://patchwork.libcamera.org/comment/33693/","msgid":"","date":"2025-03-24T15:50:47","subject":"Re: [RFC PATCH v2] libcamera: process: Remove `ProcessManager`\n\tsingleton","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n\n2025. 03. 24. 16:41 keltezéssel, Laurent Pinchart írta:\n> Hi Barnabás,\n> \n> On Mon, Mar 24, 2025 at 12:46:05PM +0100, Barnabás Pőcze wrote:\n>> 2025. 01. 22. 0:51 keltezéssel, Laurent Pinchart írta:\n>>> On Tue, Jan 21, 2025 at 06:57:44PM +0000, Barnabás Pőcze wrote:\n>>>> The `ProcessManager` is a singleton class to handle `SIGCHLD` signals\n>>>> and report the exit status to the particular `Process` instances.\n>>>>\n>>>> However, having a singleton in a library is not favourable and it is\n>>>> even less favourable if it installs a signal handler.\n>>>>\n>>>> Using pidfd it is possible to avoid the need for the signal handler;\n>>>> and the `Process` objects can watch their pidfd themselves, eliminating\n>>>> the need for the `ProcessManager` class altogether.\n>>>\n>>> This patch does a few other things: it replaces vector arguments to\n>>> start() with spans, and make the closeAllFdsExcept() member function\n>>> global. Splitting it in separate patches will make it easier to review.\n>>>\n>>> Additionally, usage of clone3() allows dropping the unshare() call, and\n>>> that would be useful to mention in the commit message to facilitate\n>>> reviews.\n>>>\n>>>> `clone3()` was introduced in Linux 5.3, so this change raises the\n>>>> minimum supported kernel version.\n>>>\n>>> You should update the kernel_version_req variable in the main\n>>> meson.build file.\n>>>\n>>>>\n>>>> Signed-off-by: Barnabás Pőcze \n>>>> ---\n>>>> include/libcamera/internal/camera_manager.h | 1 -\n>>>> include/libcamera/internal/process.h | 39 +--\n>>>> src/libcamera/process.cpp | 315 ++++++--------------\n>>>> test/ipc/unixsocket_ipc.cpp | 2 -\n>>>> test/log/log_process.cpp | 2 -\n>>>> test/process/process_test.cpp | 2 -\n>>>> 6 files changed, 91 insertions(+), 270 deletions(-)\n> [...]\n>>>> +\tif (childPid) {\n>>>> \t\trunning_ = true;\n>>>> +\t\tpid_ = childPid;\n>>>> +\t\tpidfd_ = UniqueFD(pidfd);\n>>>> +\t\tpidfdNotify_ = std::make_unique(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>>>\n>>> Is it worth checking this ? The man page tells si_signo is always set to\n>>> SIGCHLD.\n>>>\n>>>> +\t\t\t\tASSERT(info.si_pid == pid_);\n>>>\n>>> Something would be seriously wrong if this assertion fails, I'm not sure\n>>> it's needed.\n>>\n>> I suppose that's fair, but I'd like to keep at least the second one.\n> \n> Out of curiosity, do you expect it could ever happen ?\n\nI generally do not expect an assertion to ever trigger during normal operations.\nSame applies here, unless there is some kind of memory error, file descriptor\nmishap, kernel bug, etc. I wouldn't expect `info.si_pid == pid_` to ever fail.\n\n\n> [...]\n\nRegards,\nBarnabás Pőcze","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 621C4C3213\n\tfor ;\n\tMon, 24 Mar 2025 15:50:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7D42168951;\n\tMon, 24 Mar 2025 16:50:53 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AEF256893F\n\tfor ;\n\tMon, 24 Mar 2025 16:50:51 +0100 (CET)","from [192.168.33.27] (185.221.143.221.nat.pool.zt.hu\n\t[185.221.143.221])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 000C4455;\n\tMon, 24 Mar 2025 16:49:04 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"WMC7vGq+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1742831345;\n\tbh=8/dS2eSNZdNnFy9yekQHT7nSgZiUw4rSSznR2nq6T3A=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=WMC7vGq+DRBEa9Fw8WOAzKBqK8BHXuozARNeY+jwBQQOSFGWBL2QMBDIGGh34j0qO\n\tYM+LKo17qqB7P5OLgx+5Z1mZSLLFCIT/cWEACH4jZdbMgecHOaJS7oTqnIiSxpkNnI\n\tdnOCMFifWYJ+3Ik3kKRh/dh1p1/ln6jG40LzCTpM=","Message-ID":"","Date":"Mon, 24 Mar 2025 16:50:47 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [RFC PATCH v2] libcamera: process: Remove `ProcessManager`\n\tsingleton","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20250121185741.302256-1-pobrn@protonmail.com>\n\t<20250121235119.GA28510@pendragon.ideasonboard.com>\n\t<29a32ed6-8712-468b-98cb-31b2532579ca@ideasonboard.com>\n\t<20250324154123.GG5113@pendragon.ideasonboard.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<20250324154123.GG5113@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\" "}}]