[RFC PATCH v5 9/9] libcamera: process: Remove `ProcessManager` singleton
Kieran Bingham
kieran.bingham at ideasonboard.com
Sat Apr 26 15:27:55 CEST 2025
Quoting Barnabás Pőcze (2025-04-24 12:41:03)
> The `ProcessManager` is a singleton class to handle `SIGCHLD` signals
> and report the exit status to the particular `Process` instance.
>
> However, having a singleton in a library is not favourable and it is
> even less favourable if it installs a signal handler.
Do we still have the segfault signal handler? I thought we had some
custom handler to print a backtrace somewhere...
I guess that's separate.
> Using pidfd it is possible to avoid the need for the signal handler;
> and the `Process` objects can watch their pidfd themselves, eliminating
> the need for the `ProcessManager` class altogether.
>
> `P_PIDFD` for `waitid()` was introduced in Linux 5.4, so this change
> raises the minimum supported kernel version. `clone3()`, `CLONE_PIDFD`,
> `pidfd_send_signal()` were all introduced earlier.
>
> Furthermore, the call to the `unshare()` system call can be removed
> as those options can be passed to `clone3()` directly.
I know we've had issues in CI tests before with unshare() so I think
that being removed sounds good, except I expect the same permission
requirements of unshare() on the clone3() so I guess it won't change
much.
Is there anything to explore here that the process we launch can have
custom restrictions/jails added? (or is that where we would have to
handle things in a separate daemon context?)
Anyway, that's a lot of code removed, which if it doesn't mean a loss of
functionality is a good thing.
> Signed-off-by: Barnabás Pőcze <barnabas.pocze at ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
> include/libcamera/internal/camera_manager.h | 1 -
> include/libcamera/internal/process.h | 34 +--
> meson.build | 2 +-
> src/libcamera/process.cpp | 245 +++++---------------
> test/ipc/unixsocket_ipc.cpp | 2 -
> test/log/log_process.cpp | 2 -
> test/process/process_test.cpp | 2 -
> 7 files changed, 60 insertions(+), 228 deletions(-)
>
> diff --git a/include/libcamera/internal/camera_manager.h b/include/libcamera/internal/camera_manager.h
> index 0150ca61f..5dfbe1f65 100644
> --- a/include/libcamera/internal/camera_manager.h
> +++ b/include/libcamera/internal/camera_manager.h
> @@ -65,7 +65,6 @@ private:
> std::unique_ptr<DeviceEnumerator> enumerator_;
>
> std::unique_ptr<IPAManager> ipaManager_;
> - ProcessManager processManager_;
> };
>
> } /* namespace libcamera */
> diff --git a/include/libcamera/internal/process.h b/include/libcamera/internal/process.h
> index e55d11fa3..27d4b9258 100644
> --- a/include/libcamera/internal/process.h
> +++ b/include/libcamera/internal/process.h
> @@ -7,7 +7,6 @@
>
> #pragma once
>
> -#include <signal.h>
> #include <string>
>
> #include <libcamera/base/class.h>
> @@ -45,41 +44,14 @@ public:
> private:
> LIBCAMERA_DISABLE_COPY_AND_MOVE(Process)
>
> - int isolate();
> - void died(int wstatus);
> + void onPidfdNotify();
>
> pid_t pid_;
> enum ExitStatus exitStatus_;
> int exitCode_;
>
> - friend class ProcessManager;
> -};
> -
> -class ProcessManager
> -{
> -public:
> - ProcessManager();
> - ~ProcessManager();
> -
> - void registerProcess(Process *proc);
> -
> - static ProcessManager *instance();
> -
> - int writePipe() const;
> -
> - const struct sigaction &oldsa() const;
> -
> -private:
> - static ProcessManager *self_;
> -
> - void sighandler();
> -
> - std::list<Process *> processes_;
> -
> - struct sigaction oldsa_;
> -
> - EventNotifier *sigEvent_;
> - UniqueFD pipe_[2];
> + UniqueFD pidfd_;
> + std::unique_ptr<EventNotifier> pidfdNotify_;
> };
>
> } /* namespace libcamera */
> diff --git a/meson.build b/meson.build
> index ae13727eb..3c895ae9e 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -265,7 +265,7 @@ subdir('Documentation')
> subdir('test')
>
> if not meson.is_cross_build()
> - kernel_version_req = '>= 5.0.0'
> + kernel_version_req = '>= 5.4.0'
> kernel_version = run_command('uname', '-r', check : true).stdout().strip()
> if not kernel_version.version_compare(kernel_version_req)
> warning('The current running kernel version @0@ is too old to run libcamera.'
> diff --git a/src/libcamera/process.cpp b/src/libcamera/process.cpp
> index 42b56374d..b01c78034 100644
> --- a/src/libcamera/process.cpp
> +++ b/src/libcamera/process.cpp
> @@ -10,15 +10,18 @@
> #include <algorithm>
> #include <dirent.h>
> #include <fcntl.h>
> -#include <list>
> #include <signal.h>
> #include <string.h>
> #include <sys/socket.h>
> +#include <sys/syscall.h>
> #include <sys/types.h>
> #include <sys/wait.h>
> #include <unistd.h>
> #include <vector>
>
> +#include <linux/sched.h>
> +#include <linux/wait.h> /* glibc only provides `P_PIDFD` in `sys/wait.h` since 2.36 */
> +
> #include <libcamera/base/event_notifier.h>
> #include <libcamera/base/log.h>
> #include <libcamera/base/utils.h>
> @@ -32,37 +35,8 @@ namespace libcamera {
>
> LOG_DEFINE_CATEGORY(Process)
>
> -/**
> - * \class ProcessManager
> - * \brief Manager of processes
> - *
> - * The ProcessManager singleton keeps track of all created Process instances,
> - * and manages the signal handling involved in terminating processes.
> - */
> -
> namespace {
>
> -void sigact(int signal, siginfo_t *info, void *ucontext)
> -{
> -#pragma GCC diagnostic push
> -#pragma GCC diagnostic ignored "-Wunused-result"
> - /*
> - * We're in a signal handler so we can't log any message, and we need
> - * to continue anyway.
> - */
> - char data = 0;
> - write(ProcessManager::instance()->writePipe(), &data, sizeof(data));
> -#pragma GCC diagnostic pop
> -
> - const struct sigaction &oldsa = ProcessManager::instance()->oldsa();
> - if (oldsa.sa_flags & SA_SIGINFO) {
> - oldsa.sa_sigaction(signal, info, ucontext);
> - } else {
> - if (oldsa.sa_handler != SIG_IGN && oldsa.sa_handler != SIG_DFL)
> - oldsa.sa_handler(signal);
> - }
> -}
> -
> void closeAllFdsExcept(Span<const int> fds)
> {
> std::vector<int> v(fds.begin(), fds.end());
> @@ -118,129 +92,6 @@ void closeAllFdsExcept(Span<const int> fds)
>
> } /* namespace */
>
> -void ProcessManager::sighandler()
> -{
> - char data;
> - ssize_t ret = read(pipe_[0].get(), &data, sizeof(data));
> - if (ret < 0) {
> - LOG(Process, Error)
> - << "Failed to read byte from signal handler pipe";
> - return;
> - }
> -
> - for (auto it = processes_.begin(); it != processes_.end();) {
> - Process *process = *it;
> -
> - int wstatus;
> - pid_t pid = waitpid(process->pid_, &wstatus, WNOHANG);
> - if (process->pid_ != pid) {
> - ++it;
> - continue;
> - }
> -
> - it = processes_.erase(it);
> - process->died(wstatus);
> - }
> -}
> -
> -/**
> - * \brief Register process with process manager
> - * \param[in] proc Process to register
> - *
> - * This function registers the \a proc with the process manager. It
> - * shall be called by the parent process after successfully forking, in
> - * order to let the parent signal process termination.
> - */
> -void ProcessManager::registerProcess(Process *proc)
> -{
> - processes_.push_back(proc);
> -}
> -
> -ProcessManager *ProcessManager::self_ = nullptr;
> -
> -/**
> - * \brief Construct a ProcessManager instance
> - *
> - * The ProcessManager class is meant to only be instantiated once, by the
> - * CameraManager.
> - */
> -ProcessManager::ProcessManager()
> -{
> - if (self_)
> - LOG(Process, Fatal)
> - << "Multiple ProcessManager objects are not allowed";
> -
> - sigaction(SIGCHLD, NULL, &oldsa_);
> -
> - struct sigaction sa;
> - memset(&sa, 0, sizeof(sa));
> - sa.sa_sigaction = &sigact;
> - memcpy(&sa.sa_mask, &oldsa_.sa_mask, sizeof(sa.sa_mask));
> - sigaddset(&sa.sa_mask, SIGCHLD);
> - sa.sa_flags = oldsa_.sa_flags | SA_SIGINFO;
> -
> - sigaction(SIGCHLD, &sa, NULL);
> -
> - int pipe[2];
> - if (pipe2(pipe, O_CLOEXEC | O_DIRECT | O_NONBLOCK))
> - LOG(Process, Fatal)
> - << "Failed to initialize pipe for signal handling";
> -
> - pipe_[0] = UniqueFD(pipe[0]);
> - pipe_[1] = UniqueFD(pipe[1]);
> -
> - sigEvent_ = new EventNotifier(pipe_[0].get(), EventNotifier::Read);
> - sigEvent_->activated.connect(this, &ProcessManager::sighandler);
> -
> - self_ = this;
> -}
> -
> -ProcessManager::~ProcessManager()
> -{
> - sigaction(SIGCHLD, &oldsa_, NULL);
> -
> - delete sigEvent_;
> -
> - self_ = nullptr;
> -}
> -
> -/**
> - * \brief Retrieve the Process manager instance
> - *
> - * The ProcessManager is constructed by the CameraManager. This function shall
> - * be used to retrieve the single instance of the manager.
> - *
> - * \return The Process manager instance
> - */
> -ProcessManager *ProcessManager::instance()
> -{
> - return self_;
> -}
> -
> -/**
> - * \brief Retrieve the Process manager's write pipe
> - *
> - * This function is meant only to be used by the static signal handler.
> - *
> - * \return Pipe for writing
> - */
> -int ProcessManager::writePipe() const
> -{
> - return pipe_[1].get();
> -}
> -
> -/**
> - * \brief Retrive the old signal action data
> - *
> - * This function is meant only to be used by the static signal handler.
> - *
> - * \return The old signal action data
> - */
> -const struct sigaction &ProcessManager::oldsa() const
> -{
> - return oldsa_;
> -}
> -
> /**
> * \class Process
> * \brief Process object
> @@ -291,8 +142,6 @@ int Process::start(const std::string &path,
> Span<const std::string> args,
> Span<const int> fds)
> {
> - int ret;
> -
> if (pid_ > 0)
> return -EBUSY;
>
> @@ -301,20 +150,29 @@ int Process::start(const std::string &path,
> return -EINVAL;
> }
>
> - int childPid = fork();
> - if (childPid == -1) {
> - ret = -errno;
> + clone_args cargs = {};
> + int pidfd;
> +
> + cargs.flags = CLONE_CLEAR_SIGHAND | CLONE_PIDFD | CLONE_NEWUSER | CLONE_NEWNET;
> + cargs.pidfd = reinterpret_cast<uintptr_t>(&pidfd);
> + cargs.exit_signal = SIGCHLD;
> +
> + long childPid = syscall(SYS_clone3, &cargs, sizeof(cargs));
> + if (childPid < 0) {
> + int ret = -errno;
> LOG(Process, Error) << "Failed to fork: " << strerror(-ret);
> return ret;
> - } else if (childPid) {
> + }
> +
> + if (childPid) {
> pid_ = childPid;
> - ProcessManager::instance()->registerProcess(this);
> + pidfd_ = UniqueFD(pidfd);
> + pidfdNotify_ = std::make_unique<EventNotifier>(pidfd_.get(), EventNotifier::Type::Read);
> + pidfdNotify_->activated.connect(this, &Process::onPidfdNotify);
>
> - return 0;
> + LOG(Process, Debug) << this << "[" << childPid << ':' << pidfd << "]"
> + << " forked";
> } else {
> - if (isolate())
> - _exit(EXIT_FAILURE);
> -
> closeAllFdsExcept(fds);
>
> const char *file = utils::secure_getenv("LIBCAMERA_LOG_FILE");
> @@ -333,35 +191,44 @@ int Process::start(const std::string &path,
>
> exit(EXIT_FAILURE);
> }
> -}
> -
> -int Process::isolate()
> -{
> - int ret = unshare(CLONE_NEWUSER | CLONE_NEWNET);
> - if (ret) {
> - ret = -errno;
> - LOG(Process, Error) << "Failed to unshare execution context: "
> - << strerror(-ret);
> - return ret;
> - }
>
> return 0;
> }
>
> -/**
> - * \brief SIGCHLD handler
> - * \param[in] wstatus The status as output by waitpid()
> - *
> - * This function is called when the process associated with Process terminates.
> - * It emits the Process::finished signal.
> - */
> -void Process::died(int wstatus)
> +void Process::onPidfdNotify()
> {
> - pid_ = -1;
> - exitStatus_ = WIFEXITED(wstatus) ? NormalExit : SignalExit;
> - exitCode_ = exitStatus_ == NormalExit ? WEXITSTATUS(wstatus) : -1;
> + auto pidfdNotify = std::exchange(pidfdNotify_, {});
> + auto pidfd = std::exchange(pidfd_, {});
> + auto pid = std::exchange(pid_, -1);
> +
> + ASSERT(pidfdNotify);
> + ASSERT(pidfd.isValid());
> + ASSERT(pid > 0);
> +
> + siginfo_t info;
> +
> + /*
> + * `static_cast` is needed because `P_PIDFD` is not defined in `sys/wait.h` if the C standard library
> + * is old enough. So `P_PIDFD` is taken from `linux/wait.h`, where it is just an integer #define.
> + */
> + if (waitid(static_cast<idtype_t>(P_PIDFD), pidfd.get(), &info, WNOHANG | WEXITED) >= 0) {
> + ASSERT(info.si_pid == pid);
>
> - finished.emit(exitStatus_, exitCode_);
> + LOG(Process, Debug)
> + << this << "[" << pid << ':' << pidfd.get() << "]"
> + << " code: " << info.si_code
> + << " status: " << info.si_status;
> +
> + exitStatus_ = info.si_code == CLD_EXITED ? Process::NormalExit : Process::SignalExit;
> + exitCode_ = info.si_code == CLD_EXITED ? info.si_status : -1;
> +
> + finished.emit(exitStatus_, exitCode_);
> + } else {
> + int err = errno;
> + LOG(Process, Warning)
> + << this << "[" << pid << ":" << pidfd.get() << "]"
> + << " waitid() failed: " << strerror(err);
I wonder if there's any extra way we can get custom messages from the
IPA's over to the main process to report /why/ they shut down in event
of failures.
So frequently when IPA's are isolated - do we get difficult to support
issues from users who's IPA isn't running - and we don't report in
libcamera any thing about /why/
Anyway - I don't see anything wrong with this patch so :
Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> + }
> }
>
> /**
> @@ -399,8 +266,8 @@ void Process::died(int wstatus)
> */
> void Process::kill()
> {
> - if (pid_ > 0)
> - ::kill(pid_, SIGKILL);
> + if (pidfd_.isValid())
> + syscall(SYS_pidfd_send_signal, pidfd_.get(), SIGKILL, nullptr, 0);
> }
>
> } /* namespace libcamera */
> diff --git a/test/ipc/unixsocket_ipc.cpp b/test/ipc/unixsocket_ipc.cpp
> index df7d9c2b4..f3e3c09ef 100644
> --- a/test/ipc/unixsocket_ipc.cpp
> +++ b/test/ipc/unixsocket_ipc.cpp
> @@ -209,8 +209,6 @@ protected:
> }
>
> private:
> - ProcessManager processManager_;
> -
> unique_ptr<IPCPipeUnixSocket> ipc_;
> };
>
> diff --git a/test/log/log_process.cpp b/test/log/log_process.cpp
> index 9609e23d5..367b78803 100644
> --- a/test/log/log_process.cpp
> +++ b/test/log/log_process.cpp
> @@ -137,8 +137,6 @@ private:
> exitCode_ = exitCode;
> }
>
> - ProcessManager processManager_;
> -
> Process proc_;
> Process::ExitStatus exitStatus_ = Process::NotExited;
> string logPath_;
> diff --git a/test/process/process_test.cpp b/test/process/process_test.cpp
> index a88d8fef1..fe76666e6 100644
> --- a/test/process/process_test.cpp
> +++ b/test/process/process_test.cpp
> @@ -86,8 +86,6 @@ private:
> exitCode_ = exitCode;
> }
>
> - ProcessManager processManager_;
> -
> Process proc_;
> enum Process::ExitStatus exitStatus_;
> int exitCode_;
> --
> 2.49.0
>
More information about the libcamera-devel
mailing list