[RFC PATCH v5 9/9] libcamera: process: Remove `ProcessManager` singleton
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sat Apr 26 18:42:31 CEST 2025
On Sat, Apr 26, 2025 at 02:27:55PM +0100, Kieran Bingham wrote:
> 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...
That's for ASSERT() and the fatal log level. We don't install a signal
handler.
> 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?)
It could be done here, but I think we should move it to a daemon.
> 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_;
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list