[RFC PATCH v3 9/9] libcamera: process: Remove `ProcessManager` singleton
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Mar 26 16:03:25 CET 2025
Hi Barnabás,
Thank you for the patch.
On Tue, Mar 25, 2025 at 07:08:21PM +0100, Barnabás Pőcze wrote:
> The `ProcessManager` is a singleton class to handle `SIGCHLD` signals
> and report the exit status to the particular `Process` instances.
>
> However, having a singleton in a library is not favourable and it is
> even less favourable if it installs a signal handler.
>
> 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.
>
> Signed-off-by: Barnabás Pőcze <barnabas.pocze at ideasonboard.com>
> ---
> include/libcamera/internal/camera_manager.h | 1 -
> include/libcamera/internal/process.h | 34 +--
> meson.build | 2 +-
> src/libcamera/process.cpp | 241 +++++---------------
> test/ipc/unixsocket_ipc.cpp | 2 -
> test/log/log_process.cpp | 2 -
> test/process/process_test.cpp | 2 -
> 7 files changed, 55 insertions(+), 229 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 14391d60a..e600a49f8 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/signal.h>
> @@ -44,41 +43,14 @@ public:
> private:
> LIBCAMERA_DISABLE_COPY_AND_MOVE(Process)
>
> - int isolate();
> - void died(int wstatus);
> -
> 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_;
> + UniqueFD pidfd_;
> + std::unique_ptr<EventNotifier> pidfdNotify_;
>
> - EventNotifier *sigEvent_;
> - UniqueFD pipe_[2];
> + void onPidfdNotify();
We declare member functions before member variables.
> };
>
> } /* namespace libcamera */
> diff --git a/meson.build b/meson.build
> index 00291d628..bd2c88dfa 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 5fa813300..cadf5c43e 100644
> --- a/src/libcamera/process.cpp
> +++ b/src/libcamera/process.cpp
> @@ -10,15 +10,19 @@
> #include <algorithm>
> #include <dirent.h>
> #include <fcntl.h>
> -#include <list>
> +#include <sched.h>
> #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 +36,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());
> @@ -113,129 +88,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
> @@ -286,8 +138,6 @@ int Process::start(const std::string &path,
> Span<const std::string> args,
> Span<const int> fds)
> {
> - int ret;
> -
> if (pid_ > 0)
> return -EBUSY;
>
> @@ -296,20 +146,26 @@ int Process::start(const std::string &path,
> return -EINVAL;
> }
>
> - int childPid = fork();
> - if (childPid == -1) {
> - ret = -errno;
> + int pidfd;
> + clone_args cargs = {};
> +
> + 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) {
> - pid_ = childPid;
> - ProcessManager::instance()->registerProcess(this);
> + }
>
> - return 0;
> + if (childPid) {
> + pid_ = childPid;
> + pidfd_ = UniqueFD(pidfd);
> + pidfdNotify_ = std::make_unique<EventNotifier>(pidfd_.get(), EventNotifier::Type::Read);
> + pidfdNotify_->activated.connect(this, &Process::onPidfdNotify);
> } else {
> - if (isolate())
> - _exit(EXIT_FAILURE);
> -
> closeAllFdsExcept(fds);
>
> const char *file = utils::secure_getenv("LIBCAMERA_LOG_FILE");
> @@ -328,35 +184,40 @@ 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);
Do we have to guard against this, or could we just write
pidfdNotify_.reset();
?
> + ASSERT(pidfd.isValid());
> + ASSERT(pid > 0);
> +
> + siginfo_t info;
> +
> + 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() << "]"
Sounds like a candidate to inherit from Loggable and avoid duplication
of this in log message. You would need to reset pid_ to -1 at the end of
the function though. Except it should be done before emitting the
finished signal, as there's no running_ variable anymore. Annoying... I
wonder if we should keep running_, or if there's a cleaner way.
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> + << " 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);
> + }
> }
>
> /**
> @@ -394,8 +255,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_;
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list