[RFC PATCH v3 9/9] libcamera: process: Remove `ProcessManager` singleton
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Mar 28 00:45:31 CET 2025
On Thu, Mar 27, 2025 at 05:13:05PM +0100, Barnabás Pőcze wrote:
> 2025. 03. 26. 16:03 keltezéssel, Laurent Pinchart írta:
> > 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.
>
> I'll give it some thought because this indeed does not look ideal.
>
> Also, regrettably it seems chromeos does not have new enough linux headers.
> I'm wondering if it would be possible to update the CI image to something newer?
> I have tested release-R135-16209.B locally, and that appears to have the
> necessary definitions.
That would require a recent image for Soraka, and I don't know if we can
obtain one. Without that, we won't be able to run test on Chrome OS
anymore.
> > 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