[{"id":34056,"web_url":"https://patchwork.libcamera.org/comment/34056/","msgid":"<174567407593.1742020.12024741917742167629@ping.linuxembedded.co.uk>","date":"2025-04-26T13:27:55","subject":"Re: [RFC PATCH v5 9/9] libcamera: process: Remove `ProcessManager`\n\tsingleton","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Barnabás Pőcze (2025-04-24 12:41:03)\n> The `ProcessManager` is a singleton class to handle `SIGCHLD` signals\n> and report the exit status to the particular `Process` instance.\n> \n> However, having a singleton in a library is not favourable and it is\n> even less favourable if it installs a signal handler.\n\nDo we still have the segfault signal handler? I thought we had some\ncustom handler to print a backtrace somewhere...\n\nI guess that's separate.\n\n> Using pidfd it is possible to avoid the need for the signal handler;\n> and the `Process` objects can watch their pidfd themselves, eliminating\n> the need for the `ProcessManager` class altogether.\n> \n> `P_PIDFD` for `waitid()` was introduced in Linux 5.4, so this change\n> raises the minimum supported kernel version. `clone3()`, `CLONE_PIDFD`,\n> `pidfd_send_signal()` were all introduced earlier.\n> \n> Furthermore, the call to the `unshare()` system call can be removed\n> as those options can be passed to `clone3()` directly.\n\nI know we've had issues in CI tests before with unshare() so I think\nthat being removed sounds good, except I expect the same permission\nrequirements of unshare() on the clone3() so I guess it won't change\nmuch.\n\nIs there anything to explore here that the process we launch can have\ncustom restrictions/jails added? (or is that where we would have to\nhandle things in a separate daemon context?)\n\nAnyway, that's a lot of code removed, which if it doesn't mean a loss of\nfunctionality is a good thing.\n\n\n> Signed-off-by: Barnabás Pőcze \n> Reviewed-by: Laurent Pinchart \n> ---\n> include/libcamera/internal/camera_manager.h | 1 -\n> include/libcamera/internal/process.h | 34 +--\n> meson.build | 2 +-\n> src/libcamera/process.cpp | 245 +++++---------------\n> test/ipc/unixsocket_ipc.cpp | 2 -\n> test/log/log_process.cpp | 2 -\n> test/process/process_test.cpp | 2 -\n> 7 files changed, 60 insertions(+), 228 deletions(-)\n> \n> diff --git a/include/libcamera/internal/camera_manager.h b/include/libcamera/internal/camera_manager.h\n> index 0150ca61f..5dfbe1f65 100644\n> --- a/include/libcamera/internal/camera_manager.h\n> +++ b/include/libcamera/internal/camera_manager.h\n> @@ -65,7 +65,6 @@ private:\n> std::unique_ptr enumerator_;\n> \n> std::unique_ptr ipaManager_;\n> - ProcessManager processManager_;\n> };\n> \n> } /* namespace libcamera */\n> diff --git a/include/libcamera/internal/process.h b/include/libcamera/internal/process.h\n> index e55d11fa3..27d4b9258 100644\n> --- a/include/libcamera/internal/process.h\n> +++ b/include/libcamera/internal/process.h\n> @@ -7,7 +7,6 @@\n> \n> #pragma once\n> \n> -#include \n> #include \n> \n> #include \n> @@ -45,41 +44,14 @@ public:\n> private:\n> LIBCAMERA_DISABLE_COPY_AND_MOVE(Process)\n> \n> - int isolate();\n> - void died(int wstatus);\n> + void onPidfdNotify();\n> \n> pid_t pid_;\n> enum ExitStatus exitStatus_;\n> int exitCode_;\n> \n> - friend class ProcessManager;\n> -};\n> -\n> -class ProcessManager\n> -{\n> -public:\n> - ProcessManager();\n> - ~ProcessManager();\n> -\n> - void registerProcess(Process *proc);\n> -\n> - static ProcessManager *instance();\n> -\n> - int writePipe() const;\n> -\n> - const struct sigaction &oldsa() const;\n> -\n> -private:\n> - static ProcessManager *self_;\n> -\n> - void sighandler();\n> -\n> - std::list processes_;\n> -\n> - struct sigaction oldsa_;\n> -\n> - EventNotifier *sigEvent_;\n> - UniqueFD pipe_[2];\n> + UniqueFD pidfd_;\n> + std::unique_ptr pidfdNotify_;\n> };\n> \n> } /* namespace libcamera */\n> diff --git a/meson.build b/meson.build\n> index ae13727eb..3c895ae9e 100644\n> --- a/meson.build\n> +++ b/meson.build\n> @@ -265,7 +265,7 @@ subdir('Documentation')\n> subdir('test')\n> \n> if not meson.is_cross_build()\n> - kernel_version_req = '>= 5.0.0'\n> + kernel_version_req = '>= 5.4.0'\n> kernel_version = run_command('uname', '-r', check : true).stdout().strip()\n> if not kernel_version.version_compare(kernel_version_req)\n> warning('The current running kernel version @0@ is too old to run libcamera.'\n> diff --git a/src/libcamera/process.cpp b/src/libcamera/process.cpp\n> index 42b56374d..b01c78034 100644\n> --- a/src/libcamera/process.cpp\n> +++ b/src/libcamera/process.cpp\n> @@ -10,15 +10,18 @@\n> #include \n> #include \n> #include \n> -#include \n> #include \n> #include \n> #include \n> +#include \n> #include \n> #include \n> #include \n> #include \n> \n> +#include \n> +#include /* glibc only provides `P_PIDFD` in `sys/wait.h` since 2.36 */\n> +\n> #include \n> #include \n> #include \n> @@ -32,37 +35,8 @@ namespace libcamera {\n> \n> LOG_DEFINE_CATEGORY(Process)\n> \n> -/**\n> - * \\class ProcessManager\n> - * \\brief Manager of processes\n> - *\n> - * The ProcessManager singleton keeps track of all created Process instances,\n> - * and manages the signal handling involved in terminating processes.\n> - */\n> -\n> namespace {\n> \n> -void sigact(int signal, siginfo_t *info, void *ucontext)\n> -{\n> -#pragma GCC diagnostic push\n> -#pragma GCC diagnostic ignored \"-Wunused-result\"\n> - /*\n> - * We're in a signal handler so we can't log any message, and we need\n> - * to continue anyway.\n> - */\n> - char data = 0;\n> - write(ProcessManager::instance()->writePipe(), &data, sizeof(data));\n> -#pragma GCC diagnostic pop\n> -\n> - const struct sigaction &oldsa = ProcessManager::instance()->oldsa();\n> - if (oldsa.sa_flags & SA_SIGINFO) {\n> - oldsa.sa_sigaction(signal, info, ucontext);\n> - } else {\n> - if (oldsa.sa_handler != SIG_IGN && oldsa.sa_handler != SIG_DFL)\n> - oldsa.sa_handler(signal);\n> - }\n> -}\n> -\n> void closeAllFdsExcept(Span fds)\n> {\n> std::vector v(fds.begin(), fds.end());\n> @@ -118,129 +92,6 @@ void closeAllFdsExcept(Span fds)\n> \n> } /* namespace */\n> \n> -void ProcessManager::sighandler()\n> -{\n> - char data;\n> - ssize_t ret = read(pipe_[0].get(), &data, sizeof(data));\n> - if (ret < 0) {\n> - LOG(Process, Error)\n> - << \"Failed to read byte from signal handler pipe\";\n> - return;\n> - }\n> -\n> - for (auto it = processes_.begin(); it != processes_.end();) {\n> - Process *process = *it;\n> -\n> - int wstatus;\n> - pid_t pid = waitpid(process->pid_, &wstatus, WNOHANG);\n> - if (process->pid_ != pid) {\n> - ++it;\n> - continue;\n> - }\n> -\n> - it = processes_.erase(it);\n> - process->died(wstatus);\n> - }\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> - processes_.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> - if (self_)\n> - LOG(Process, Fatal)\n> - << \"Multiple ProcessManager objects are not allowed\";\n> -\n> - sigaction(SIGCHLD, NULL, &oldsa_);\n> -\n> - struct sigaction sa;\n> - memset(&sa, 0, sizeof(sa));\n> - sa.sa_sigaction = &sigact;\n> - memcpy(&sa.sa_mask, &oldsa_.sa_mask, sizeof(sa.sa_mask));\n> - sigaddset(&sa.sa_mask, SIGCHLD);\n> - sa.sa_flags = oldsa_.sa_flags | SA_SIGINFO;\n> -\n> - sigaction(SIGCHLD, &sa, NULL);\n> -\n> - int pipe[2];\n> - if (pipe2(pipe, O_CLOEXEC | O_DIRECT | O_NONBLOCK))\n> - LOG(Process, Fatal)\n> - << \"Failed to initialize pipe for signal handling\";\n> -\n> - pipe_[0] = UniqueFD(pipe[0]);\n> - pipe_[1] = UniqueFD(pipe[1]);\n> -\n> - sigEvent_ = new EventNotifier(pipe_[0].get(), EventNotifier::Read);\n> - sigEvent_->activated.connect(this, &ProcessManager::sighandler);\n> -\n> - self_ = this;\n> -}\n> -\n> -ProcessManager::~ProcessManager()\n> -{\n> - sigaction(SIGCHLD, &oldsa_, NULL);\n> -\n> - delete sigEvent_;\n> -\n> - self_ = 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> - return 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> - return 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> - return oldsa_;\n> -}\n> -\n> /**\n> * \\class Process\n> * \\brief Process object\n> @@ -291,8 +142,6 @@ int Process::start(const std::string &path,\n> Span args,\n> Span fds)\n> {\n> - int ret;\n> -\n> if (pid_ > 0)\n> return -EBUSY;\n> \n> @@ -301,20 +150,29 @@ int Process::start(const std::string &path,\n> return -EINVAL;\n> }\n> \n> - int childPid = fork();\n> - if (childPid == -1) {\n> - ret = -errno;\n> + clone_args cargs = {};\n> + int pidfd;\n> +\n> + cargs.flags = CLONE_CLEAR_SIGHAND | CLONE_PIDFD | CLONE_NEWUSER | CLONE_NEWNET;\n> + cargs.pidfd = reinterpret_cast(&pidfd);\n> + cargs.exit_signal = SIGCHLD;\n> +\n> + long childPid = syscall(SYS_clone3, &cargs, sizeof(cargs));\n> + if (childPid < 0) {\n> + int ret = -errno;\n> LOG(Process, Error) << \"Failed to fork: \" << strerror(-ret);\n> return ret;\n> - } else if (childPid) {\n> + }\n> +\n> + if (childPid) {\n> pid_ = childPid;\n> - ProcessManager::instance()->registerProcess(this);\n> + pidfd_ = UniqueFD(pidfd);\n> + pidfdNotify_ = std::make_unique(pidfd_.get(), EventNotifier::Type::Read);\n> + pidfdNotify_->activated.connect(this, &Process::onPidfdNotify);\n> \n> - return 0;\n> + LOG(Process, Debug) << this << \"[\" << childPid << ':' << pidfd << \"]\"\n> + << \" forked\";\n> } else {\n> - if (isolate())\n> - _exit(EXIT_FAILURE);\n> -\n> closeAllFdsExcept(fds);\n> \n> const char *file = utils::secure_getenv(\"LIBCAMERA_LOG_FILE\");\n> @@ -333,35 +191,44 @@ int Process::start(const std::string &path,\n> \n> exit(EXIT_FAILURE);\n> }\n> -}\n> -\n> -int Process::isolate()\n> -{\n> - int ret = unshare(CLONE_NEWUSER | CLONE_NEWNET);\n> - if (ret) {\n> - ret = -errno;\n> - LOG(Process, Error) << \"Failed to unshare execution context: \"\n> - << strerror(-ret);\n> - return ret;\n> - }\n> \n> return 0;\n> }\n> \n> -/**\n> - * \\brief SIGCHLD handler\n> - * \\param[in] wstatus The status as output by waitpid()\n> - *\n> - * This function is called when the process associated with Process terminates.\n> - * It emits the Process::finished signal.\n> - */\n> -void Process::died(int wstatus)\n> +void Process::onPidfdNotify()\n> {\n> - pid_ = -1;\n> - exitStatus_ = WIFEXITED(wstatus) ? NormalExit : SignalExit;\n> - exitCode_ = exitStatus_ == NormalExit ? WEXITSTATUS(wstatus) : -1;\n> + auto pidfdNotify = std::exchange(pidfdNotify_, {});\n> + auto pidfd = std::exchange(pidfd_, {});\n> + auto pid = std::exchange(pid_, -1);\n> +\n> + ASSERT(pidfdNotify);\n> + ASSERT(pidfd.isValid());\n> + ASSERT(pid > 0);\n> +\n> + siginfo_t info;\n> +\n> + /*\n> + * `static_cast` is needed because `P_PIDFD` is not defined in `sys/wait.h` if the C standard library\n> + * is old enough. So `P_PIDFD` is taken from `linux/wait.h`, where it is just an integer #define.\n> + */\n> + if (waitid(static_cast(P_PIDFD), pidfd.get(), &info, WNOHANG | WEXITED) >= 0) {\n> + ASSERT(info.si_pid == pid);\n> \n> - finished.emit(exitStatus_, exitCode_);\n> + LOG(Process, Debug)\n> + << this << \"[\" << pid << ':' << pidfd.get() << \"]\"\n> + << \" code: \" << info.si_code\n> + << \" status: \" << info.si_status;\n> +\n> + exitStatus_ = info.si_code == CLD_EXITED ? Process::NormalExit : Process::SignalExit;\n> + exitCode_ = info.si_code == CLD_EXITED ? info.si_status : -1;\n> +\n> + finished.emit(exitStatus_, exitCode_);\n> + } else {\n> + int err = errno;\n> + LOG(Process, Warning)\n> + << this << \"[\" << pid << \":\" << pidfd.get() << \"]\"\n> + << \" waitid() failed: \" << strerror(err);\n\nI wonder if there's any extra way we can get custom messages from the\nIPA's over to the main process to report /why/ they shut down in event\nof failures.\n\nSo frequently when IPA's are isolated - do we get difficult to support\nissues from users who's IPA isn't running - and we don't report in\nlibcamera any thing about /why/\n\nAnyway - I don't see anything wrong with this patch so :\n\nReviewed-by: Kieran Bingham \n\n> + }\n> }\n> \n> /**\n> @@ -399,8 +266,8 @@ void Process::died(int wstatus)\n> */\n> void Process::kill()\n> {\n> - if (pid_ > 0)\n> - ::kill(pid_, SIGKILL);\n> + if (pidfd_.isValid())\n> + syscall(SYS_pidfd_send_signal, pidfd_.get(), SIGKILL, nullptr, 0);\n> }\n> \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> }\n> \n> private:\n> - ProcessManager processManager_;\n> -\n> unique_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> exitCode_ = exitCode;\n> }\n> \n> - ProcessManager processManager_;\n> -\n> Process proc_;\n> Process::ExitStatus exitStatus_ = Process::NotExited;\n> string logPath_;\n> diff --git a/test/process/process_test.cpp b/test/process/process_test.cpp\n> index a88d8fef1..fe76666e6 100644\n> --- a/test/process/process_test.cpp\n> +++ b/test/process/process_test.cpp\n> @@ -86,8 +86,6 @@ private:\n> exitCode_ = exitCode;\n> }\n> \n> - ProcessManager processManager_;\n> -\n> Process proc_;\n> enum Process::ExitStatus exitStatus_;\n> int exitCode_;\n> -- \n> 2.49.0\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 87306C327D\n\tfor ;\n\tSat, 26 Apr 2025 13:28:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A645A68ACD;\n\tSat, 26 Apr 2025 15:28:00 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 16BE9617E3\n\tfor ;\n\tSat, 26 Apr 2025 15:27:59 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5BF47735;\n\tSat, 26 Apr 2025 15:27:55 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"HGy6zjCp\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1745674075;\n\tbh=L8BXArbqiefacYdn1bavdOcd3VF2smRPqrfAfKStib8=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=HGy6zjCpCOE2ai8xN9swpr7vNzu0pBbIibz7MkpDNse6dNnA0agQ3a6SBImQL4qUi\n\tTF2KGMKNxLQ6If3ZMfvOv1gKL2T8EZxqfSgqe/snnfAspphGXMxk9ZGog06gfpqqW5\n\tEx3/zpmUI50lA2CvX/RQNyTcLmNMfWpFLPoHZVBg=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250424114103.451395-10-barnabas.pocze@ideasonboard.com>","References":"<20250424114103.451395-1-barnabas.pocze@ideasonboard.com>\n\t<20250424114103.451395-10-barnabas.pocze@ideasonboard.com>","Subject":"Re: [RFC PATCH v5 9/9] libcamera: process: Remove `ProcessManager`\n\tsingleton","From":"Kieran Bingham ","Cc":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Sat, 26 Apr 2025 14:27:55 +0100","Message-ID":"<174567407593.1742020.12024741917742167629@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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":34058,"web_url":"https://patchwork.libcamera.org/comment/34058/","msgid":"<20250426164231.GC21390@pendragon.ideasonboard.com>","date":"2025-04-26T16:42:31","subject":"Re: [RFC PATCH v5 9/9] 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 Sat, Apr 26, 2025 at 02:27:55PM +0100, Kieran Bingham wrote:\n> Quoting Barnabás Pőcze (2025-04-24 12:41:03)\n> > The `ProcessManager` is a singleton class to handle `SIGCHLD` signals\n> > and report the exit status to the particular `Process` instance.\n> > \n> > However, having a singleton in a library is not favourable and it is\n> > even less favourable if it installs a signal handler.\n> \n> Do we still have the segfault signal handler? I thought we had some\n> custom handler to print a backtrace somewhere...\n\nThat's for ASSERT() and the fatal log level. We don't install a signal\nhandler.\n\n> I guess that's separate.\n> \n> > Using pidfd it is possible to avoid the need for the signal handler;\n> > and the `Process` objects can watch their pidfd themselves, eliminating\n> > the need for the `ProcessManager` class altogether.\n> > \n> > `P_PIDFD` for `waitid()` was introduced in Linux 5.4, so this change\n> > raises the minimum supported kernel version. `clone3()`, `CLONE_PIDFD`,\n> > `pidfd_send_signal()` were all introduced earlier.\n> > \n> > Furthermore, the call to the `unshare()` system call can be removed\n> > as those options can be passed to `clone3()` directly.\n> \n> I know we've had issues in CI tests before with unshare() so I think\n> that being removed sounds good, except I expect the same permission\n> requirements of unshare() on the clone3() so I guess it won't change\n> much.\n> \n> Is there anything to explore here that the process we launch can have\n> custom restrictions/jails added? (or is that where we would have to\n> handle things in a separate daemon context?)\n\nIt could be done here, but I think we should move it to a daemon.\n\n> Anyway, that's a lot of code removed, which if it doesn't mean a loss of\n> functionality is a good thing.\n> \n> > Signed-off-by: Barnabás Pőcze \n> > Reviewed-by: Laurent Pinchart \n> > ---\n> > include/libcamera/internal/camera_manager.h | 1 -\n> > include/libcamera/internal/process.h | 34 +--\n> > meson.build | 2 +-\n> > src/libcamera/process.cpp | 245 +++++---------------\n> > test/ipc/unixsocket_ipc.cpp | 2 -\n> > test/log/log_process.cpp | 2 -\n> > test/process/process_test.cpp | 2 -\n> > 7 files changed, 60 insertions(+), 228 deletions(-)\n> > \n> > diff --git a/include/libcamera/internal/camera_manager.h b/include/libcamera/internal/camera_manager.h\n> > index 0150ca61f..5dfbe1f65 100644\n> > --- a/include/libcamera/internal/camera_manager.h\n> > +++ b/include/libcamera/internal/camera_manager.h\n> > @@ -65,7 +65,6 @@ private:\n> > std::unique_ptr enumerator_;\n> > \n> > std::unique_ptr ipaManager_;\n> > - ProcessManager processManager_;\n> > };\n> > \n> > } /* namespace libcamera */\n> > diff --git a/include/libcamera/internal/process.h b/include/libcamera/internal/process.h\n> > index e55d11fa3..27d4b9258 100644\n> > --- a/include/libcamera/internal/process.h\n> > +++ b/include/libcamera/internal/process.h\n> > @@ -7,7 +7,6 @@\n> > \n> > #pragma once\n> > \n> > -#include \n> > #include \n> > \n> > #include \n> > @@ -45,41 +44,14 @@ public:\n> > private:\n> > LIBCAMERA_DISABLE_COPY_AND_MOVE(Process)\n> > \n> > - int isolate();\n> > - void died(int wstatus);\n> > + void onPidfdNotify();\n> > \n> > pid_t pid_;\n> > enum ExitStatus exitStatus_;\n> > int exitCode_;\n> > \n> > - friend class ProcessManager;\n> > -};\n> > -\n> > -class ProcessManager\n> > -{\n> > -public:\n> > - ProcessManager();\n> > - ~ProcessManager();\n> > -\n> > - void registerProcess(Process *proc);\n> > -\n> > - static ProcessManager *instance();\n> > -\n> > - int writePipe() const;\n> > -\n> > - const struct sigaction &oldsa() const;\n> > -\n> > -private:\n> > - static ProcessManager *self_;\n> > -\n> > - void sighandler();\n> > -\n> > - std::list processes_;\n> > -\n> > - struct sigaction oldsa_;\n> > -\n> > - EventNotifier *sigEvent_;\n> > - UniqueFD pipe_[2];\n> > + UniqueFD pidfd_;\n> > + std::unique_ptr pidfdNotify_;\n> > };\n> > \n> > } /* namespace libcamera */\n> > diff --git a/meson.build b/meson.build\n> > index ae13727eb..3c895ae9e 100644\n> > --- a/meson.build\n> > +++ b/meson.build\n> > @@ -265,7 +265,7 @@ subdir('Documentation')\n> > subdir('test')\n> > \n> > if not meson.is_cross_build()\n> > - kernel_version_req = '>= 5.0.0'\n> > + kernel_version_req = '>= 5.4.0'\n> > kernel_version = run_command('uname', '-r', check : true).stdout().strip()\n> > if not kernel_version.version_compare(kernel_version_req)\n> > warning('The current running kernel version @0@ is too old to run libcamera.'\n> > diff --git a/src/libcamera/process.cpp b/src/libcamera/process.cpp\n> > index 42b56374d..b01c78034 100644\n> > --- a/src/libcamera/process.cpp\n> > +++ b/src/libcamera/process.cpp\n> > @@ -10,15 +10,18 @@\n> > #include \n> > #include \n> > #include \n> > -#include \n> > #include \n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> > #include \n> > #include \n> > \n> > +#include \n> > +#include /* glibc only provides `P_PIDFD` in `sys/wait.h` since 2.36 */\n> > +\n> > #include \n> > #include \n> > #include \n> > @@ -32,37 +35,8 @@ namespace libcamera {\n> > \n> > LOG_DEFINE_CATEGORY(Process)\n> > \n> > -/**\n> > - * \\class ProcessManager\n> > - * \\brief Manager of processes\n> > - *\n> > - * The ProcessManager singleton keeps track of all created Process instances,\n> > - * and manages the signal handling involved in terminating processes.\n> > - */\n> > -\n> > namespace {\n> > \n> > -void sigact(int signal, siginfo_t *info, void *ucontext)\n> > -{\n> > -#pragma GCC diagnostic push\n> > -#pragma GCC diagnostic ignored \"-Wunused-result\"\n> > - /*\n> > - * We're in a signal handler so we can't log any message, and we need\n> > - * to continue anyway.\n> > - */\n> > - char data = 0;\n> > - write(ProcessManager::instance()->writePipe(), &data, sizeof(data));\n> > -#pragma GCC diagnostic pop\n> > -\n> > - const struct sigaction &oldsa = ProcessManager::instance()->oldsa();\n> > - if (oldsa.sa_flags & SA_SIGINFO) {\n> > - oldsa.sa_sigaction(signal, info, ucontext);\n> > - } else {\n> > - if (oldsa.sa_handler != SIG_IGN && oldsa.sa_handler != SIG_DFL)\n> > - oldsa.sa_handler(signal);\n> > - }\n> > -}\n> > -\n> > void closeAllFdsExcept(Span fds)\n> > {\n> > std::vector v(fds.begin(), fds.end());\n> > @@ -118,129 +92,6 @@ void closeAllFdsExcept(Span fds)\n> > \n> > } /* namespace */\n> > \n> > -void ProcessManager::sighandler()\n> > -{\n> > - char data;\n> > - ssize_t ret = read(pipe_[0].get(), &data, sizeof(data));\n> > - if (ret < 0) {\n> > - LOG(Process, Error)\n> > - << \"Failed to read byte from signal handler pipe\";\n> > - return;\n> > - }\n> > -\n> > - for (auto it = processes_.begin(); it != processes_.end();) {\n> > - Process *process = *it;\n> > -\n> > - int wstatus;\n> > - pid_t pid = waitpid(process->pid_, &wstatus, WNOHANG);\n> > - if (process->pid_ != pid) {\n> > - ++it;\n> > - continue;\n> > - }\n> > -\n> > - it = processes_.erase(it);\n> > - process->died(wstatus);\n> > - }\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> > - processes_.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> > - if (self_)\n> > - LOG(Process, Fatal)\n> > - << \"Multiple ProcessManager objects are not allowed\";\n> > -\n> > - sigaction(SIGCHLD, NULL, &oldsa_);\n> > -\n> > - struct sigaction sa;\n> > - memset(&sa, 0, sizeof(sa));\n> > - sa.sa_sigaction = &sigact;\n> > - memcpy(&sa.sa_mask, &oldsa_.sa_mask, sizeof(sa.sa_mask));\n> > - sigaddset(&sa.sa_mask, SIGCHLD);\n> > - sa.sa_flags = oldsa_.sa_flags | SA_SIGINFO;\n> > -\n> > - sigaction(SIGCHLD, &sa, NULL);\n> > -\n> > - int pipe[2];\n> > - if (pipe2(pipe, O_CLOEXEC | O_DIRECT | O_NONBLOCK))\n> > - LOG(Process, Fatal)\n> > - << \"Failed to initialize pipe for signal handling\";\n> > -\n> > - pipe_[0] = UniqueFD(pipe[0]);\n> > - pipe_[1] = UniqueFD(pipe[1]);\n> > -\n> > - sigEvent_ = new EventNotifier(pipe_[0].get(), EventNotifier::Read);\n> > - sigEvent_->activated.connect(this, &ProcessManager::sighandler);\n> > -\n> > - self_ = this;\n> > -}\n> > -\n> > -ProcessManager::~ProcessManager()\n> > -{\n> > - sigaction(SIGCHLD, &oldsa_, NULL);\n> > -\n> > - delete sigEvent_;\n> > -\n> > - self_ = 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> > - return 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> > - return 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> > - return oldsa_;\n> > -}\n> > -\n> > /**\n> > * \\class Process\n> > * \\brief Process object\n> > @@ -291,8 +142,6 @@ int Process::start(const std::string &path,\n> > Span args,\n> > Span fds)\n> > {\n> > - int ret;\n> > -\n> > if (pid_ > 0)\n> > return -EBUSY;\n> > \n> > @@ -301,20 +150,29 @@ int Process::start(const std::string &path,\n> > return -EINVAL;\n> > }\n> > \n> > - int childPid = fork();\n> > - if (childPid == -1) {\n> > - ret = -errno;\n> > + clone_args cargs = {};\n> > + int pidfd;\n> > +\n> > + cargs.flags = CLONE_CLEAR_SIGHAND | CLONE_PIDFD | CLONE_NEWUSER | CLONE_NEWNET;\n> > + cargs.pidfd = reinterpret_cast(&pidfd);\n> > + cargs.exit_signal = SIGCHLD;\n> > +\n> > + long childPid = syscall(SYS_clone3, &cargs, sizeof(cargs));\n> > + if (childPid < 0) {\n> > + int ret = -errno;\n> > LOG(Process, Error) << \"Failed to fork: \" << strerror(-ret);\n> > return ret;\n> > - } else if (childPid) {\n> > + }\n> > +\n> > + if (childPid) {\n> > pid_ = childPid;\n> > - ProcessManager::instance()->registerProcess(this);\n> > + pidfd_ = UniqueFD(pidfd);\n> > + pidfdNotify_ = std::make_unique(pidfd_.get(), EventNotifier::Type::Read);\n> > + pidfdNotify_->activated.connect(this, &Process::onPidfdNotify);\n> > \n> > - return 0;\n> > + LOG(Process, Debug) << this << \"[\" << childPid << ':' << pidfd << \"]\"\n> > + << \" forked\";\n> > } else {\n> > - if (isolate())\n> > - _exit(EXIT_FAILURE);\n> > -\n> > closeAllFdsExcept(fds);\n> > \n> > const char *file = utils::secure_getenv(\"LIBCAMERA_LOG_FILE\");\n> > @@ -333,35 +191,44 @@ int Process::start(const std::string &path,\n> > \n> > exit(EXIT_FAILURE);\n> > }\n> > -}\n> > -\n> > -int Process::isolate()\n> > -{\n> > - int ret = unshare(CLONE_NEWUSER | CLONE_NEWNET);\n> > - if (ret) {\n> > - ret = -errno;\n> > - LOG(Process, Error) << \"Failed to unshare execution context: \"\n> > - << strerror(-ret);\n> > - return ret;\n> > - }\n> > \n> > return 0;\n> > }\n> > \n> > -/**\n> > - * \\brief SIGCHLD handler\n> > - * \\param[in] wstatus The status as output by waitpid()\n> > - *\n> > - * This function is called when the process associated with Process terminates.\n> > - * It emits the Process::finished signal.\n> > - */\n> > -void Process::died(int wstatus)\n> > +void Process::onPidfdNotify()\n> > {\n> > - pid_ = -1;\n> > - exitStatus_ = WIFEXITED(wstatus) ? NormalExit : SignalExit;\n> > - exitCode_ = exitStatus_ == NormalExit ? WEXITSTATUS(wstatus) : -1;\n> > + auto pidfdNotify = std::exchange(pidfdNotify_, {});\n> > + auto pidfd = std::exchange(pidfd_, {});\n> > + auto pid = std::exchange(pid_, -1);\n> > +\n> > + ASSERT(pidfdNotify);\n> > + ASSERT(pidfd.isValid());\n> > + ASSERT(pid > 0);\n> > +\n> > + siginfo_t info;\n> > +\n> > + /*\n> > + * `static_cast` is needed because `P_PIDFD` is not defined in `sys/wait.h` if the C standard library\n> > + * is old enough. So `P_PIDFD` is taken from `linux/wait.h`, where it is just an integer #define.\n> > + */\n> > + if (waitid(static_cast(P_PIDFD), pidfd.get(), &info, WNOHANG | WEXITED) >= 0) {\n> > + ASSERT(info.si_pid == pid);\n> > \n> > - finished.emit(exitStatus_, exitCode_);\n> > + LOG(Process, Debug)\n> > + << this << \"[\" << pid << ':' << pidfd.get() << \"]\"\n> > + << \" code: \" << info.si_code\n> > + << \" status: \" << info.si_status;\n> > +\n> > + exitStatus_ = info.si_code == CLD_EXITED ? Process::NormalExit : Process::SignalExit;\n> > + exitCode_ = info.si_code == CLD_EXITED ? info.si_status : -1;\n> > +\n> > + finished.emit(exitStatus_, exitCode_);\n> > + } else {\n> > + int err = errno;\n> > + LOG(Process, Warning)\n> > + << this << \"[\" << pid << \":\" << pidfd.get() << \"]\"\n> > + << \" waitid() failed: \" << strerror(err);\n> \n> I wonder if there's any extra way we can get custom messages from the\n> IPA's over to the main process to report /why/ they shut down in event\n> of failures.\n> \n> So frequently when IPA's are isolated - do we get difficult to support\n> issues from users who's IPA isn't running - and we don't report in\n> libcamera any thing about /why/\n> \n> Anyway - I don't see anything wrong with this patch so :\n> \n> Reviewed-by: Kieran Bingham \n> \n> > + }\n> > }\n> > \n> > /**\n> > @@ -399,8 +266,8 @@ void Process::died(int wstatus)\n> > */\n> > void Process::kill()\n> > {\n> > - if (pid_ > 0)\n> > - ::kill(pid_, SIGKILL);\n> > + if (pidfd_.isValid())\n> > + syscall(SYS_pidfd_send_signal, pidfd_.get(), SIGKILL, nullptr, 0);\n> > }\n> > \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> > }\n> > \n> > private:\n> > - ProcessManager processManager_;\n> > -\n> > unique_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> > exitCode_ = exitCode;\n> > }\n> > \n> > - ProcessManager processManager_;\n> > -\n> > Process proc_;\n> > Process::ExitStatus exitStatus_ = Process::NotExited;\n> > string logPath_;\n> > diff --git a/test/process/process_test.cpp b/test/process/process_test.cpp\n> > index a88d8fef1..fe76666e6 100644\n> > --- a/test/process/process_test.cpp\n> > +++ b/test/process/process_test.cpp\n> > @@ -86,8 +86,6 @@ private:\n> > exitCode_ = exitCode;\n> > }\n> > \n> > - ProcessManager processManager_;\n> > -\n> > Process proc_;\n> > enum Process::ExitStatus exitStatus_;\n> > int 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 CB88DC327D\n\tfor ;\n\tSat, 26 Apr 2025 16:42:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D9C2C68ACD;\n\tSat, 26 Apr 2025 18:42:37 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 28BF5617E3\n\tfor ;\n\tSat, 26 Apr 2025 18:42:36 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3623D735;\n\tSat, 26 Apr 2025 18:42:32 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"jV/Z29R6\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1745685752;\n\tbh=Pc+eGlShNVJTQV1EfG6hHneg4G5jPIERra4pyiFhmVI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=jV/Z29R6JAqtSdK/sbtXBJ2ALw/zmYibxgr1DmK2qLW9VrMtTeWmbhdw+fk64MpfQ\n\tgiaCn3fnvdHYMx/5zd3vtbnwFkRzro1hrq/A/6RhATRyqL4CA/pXggvUnLiumAkvBZ\n\t8bfPaTofJcyR5y7ClC6EJvpkUuPLAk6qVL9CkOCM=","Date":"Sat, 26 Apr 2025 19:42:31 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v5 9/9] libcamera: process: Remove `ProcessManager`\n\tsingleton","Message-ID":"<20250426164231.GC21390@pendragon.ideasonboard.com>","References":"<20250424114103.451395-1-barnabas.pocze@ideasonboard.com>\n\t<20250424114103.451395-10-barnabas.pocze@ideasonboard.com>\n\t<174567407593.1742020.12024741917742167629@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<174567407593.1742020.12024741917742167629@ping.linuxembedded.co.uk>","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\" "}}]