[RFC PATCH v2] libcamera: process: Remove `ProcessManager` singleton
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Mar 24 16:41:23 CET 2025
Hi Barnabás,
On Mon, Mar 24, 2025 at 12:46:05PM +0100, Barnabás Pőcze wrote:
> 2025. 01. 22. 0:51 keltezéssel, Laurent Pinchart írta:
> > On Tue, Jan 21, 2025 at 06:57:44PM +0000, 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.
> >
> > This patch does a few other things: it replaces vector arguments to
> > start() with spans, and make the closeAllFdsExcept() member function
> > global. Splitting it in separate patches will make it easier to review.
> >
> > Additionally, usage of clone3() allows dropping the unshare() call, and
> > that would be useful to mention in the commit message to facilitate
> > reviews.
> >
> >> `clone3()` was introduced in Linux 5.3, so this change raises the
> >> minimum supported kernel version.
> >
> > You should update the kernel_version_req variable in the main
> > meson.build file.
> >
> >>
> >> Signed-off-by: Barnabás Pőcze <pobrn at protonmail.com>
> >> ---
> >> include/libcamera/internal/camera_manager.h | 1 -
> >> include/libcamera/internal/process.h | 39 +--
> >> src/libcamera/process.cpp | 315 ++++++--------------
> >> test/ipc/unixsocket_ipc.cpp | 2 -
> >> test/log/log_process.cpp | 2 -
> >> test/process/process_test.cpp | 2 -
> >> 6 files changed, 91 insertions(+), 270 deletions(-)
> >
> > Very nice simplification overall :-)
> >
> >>
> >> 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 b1d07a5a5..9d476f3bf 100644
> >> --- a/include/libcamera/internal/process.h
> >> +++ b/include/libcamera/internal/process.h
> >> @@ -12,6 +12,7 @@
> >> #include <vector>
> >>
> >> #include <libcamera/base/signal.h>
> >> +#include <libcamera/base/span.h>
> >> #include <libcamera/base/unique_fd.h>
> >>
> >> namespace libcamera {
> >> @@ -31,8 +32,8 @@ public:
> >> ~Process();
> >>
> >> int start(const std::string &path,
> >> - const std::vector<std::string> &args = std::vector<std::string>(),
> >> - const std::vector<int> &fds = std::vector<int>());
> >> + Span<const std::string> args = {},
> >> + Span<const int> fds = {});
> >>
> >> ExitStatus exitStatus() const { return exitStatus_; }
> >> int exitCode() const { return exitCode_; }
> >> @@ -42,43 +43,13 @@ public:
> >> Signal<enum ExitStatus, int> finished;
> >>
> >> private:
> >> - void closeAllFdsExcept(const std::vector<int> &fds);
> >> - int isolate();
> >> - void died(int wstatus);
> >> -
> >> pid_t pid_;
> >> bool running_;
> >> 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/src/libcamera/process.cpp b/src/libcamera/process.cpp
> >> index bc9833f41..2b059ed92 100644
> >> --- a/src/libcamera/process.cpp
> >> +++ b/src/libcamera/process.cpp
> >> @@ -19,6 +19,11 @@
> >> #include <unistd.h>
> >> #include <vector>
> >>
> >> +#include <linux/sched.h>
> >> +#include <sched.h>
> >> +#include <sys/syscall.h>
> >> +#include <unistd.h>
> >
> > Those headers should be added to the previous group of headers. You're
> > duplicating unistd.h here.
> >
> >> +
> >> #include <libcamera/base/event_notifier.h>
> >> #include <libcamera/base/log.h>
> >> #include <libcamera/base/utils.h>
> >> @@ -32,162 +37,6 @@ 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);
> >> - }
> >> -}
> >> -
> >> -} /* 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
> >> @@ -218,6 +67,36 @@ Process::~Process()
> >> /* \todo wait for child process to exit */
> >> }
> >>
> >> +namespace {
> >> +
> >> +void closeAllFdsExcept(Span<const int> fds)
> >
> > Any particular reason not to keep this as a member function ?
> >
> >> +{
> >> + std::vector<int> v(fds.begin(), fds.end());
> >> + sort(v.begin(), v.end());
> >> +
> >> + DIR *dir = opendir("/proc/self/fd");
> >> + if (!dir)
> >> + return;
> >> +
> >> + int dfd = dirfd(dir);
> >> +
> >> + struct dirent *ent;
> >> + while ((ent = readdir(dir)) != nullptr) {
> >> + char *endp;
> >> + int fd = strtoul(ent->d_name, &endp, 10);
> >> + if (*endp)
> >> + continue;
> >> +
> >> + if (fd >= 0 && fd != dfd &&
> >> + !std::binary_search(v.begin(), v.end(), fd))
> >> + close(fd);
> >> + }
> >> +
> >> + closedir(dir);
> >> +}
> >> +
> >> +} /* namespace */
> >> +
> >> /**
> >> * \brief Fork and exec a process, and close fds
> >> * \param[in] path Path to executable
> >> @@ -235,104 +114,82 @@ Process::~Process()
> >> * or a negative error code otherwise
> >> */
> >> int Process::start(const std::string &path,
> >> - const std::vector<std::string> &args,
> >> - const std::vector<int> &fds)
> >> + Span<const std::string> args,
> >> + Span<const int> fds)
> >> {
> >> - int ret;
> >> -
> >> if (running_)
> >> return 0;
> >>
> >> - 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;
> >
> > Do we need to deliver SIGCHLD to the parent, now that we use the pidfd
> > to be notified of termination of the child ?
>
> I found that setting that is seemingly needed for gdb to work as expected,
> but I did not look further into it. glibc's `pidfd_spawn()` also sets it.
I was wondering if we could omit it to avoid applications receiving the
signal unexpectedly for the processes we spawn, but if it breaks
something, we can keep it.
> >> +
> >> + long childPid = ::syscall(SYS_clone3, &cargs, sizeof(cargs));
> >> + if (childPid < 0) {
> >> + int ret = -errno;
> >> LOG(Process, Error) << "Failed to fork: " << strerror(-ret);
> >
> > Maybe s/fork/clone/
>
> I left it as "fork" because my assumption was that more people are familiar
> with the concept of "fork", so I thought the potential error message will be
> easier to understand.
>
> >> return ret;
> >> - } else if (childPid) {
> >> - pid_ = childPid;
> >> - ProcessManager::instance()->registerProcess(this);
> >> + }
> >>
> >> + if (childPid) {
> >> running_ = true;
> >> + pid_ = childPid;
> >> + pidfd_ = UniqueFD(pidfd);
> >> + pidfdNotify_ = std::make_unique<EventNotifier>(pidfd_.get(), EventNotifier::Type::Read);
> >>
> >> - return 0;
> >> - } else {
> >> - if (isolate())
> >> - _exit(EXIT_FAILURE);
> >> + pidfdNotify_->activated.connect(this, [this] {
> >> + ::siginfo_t info;
> >> +
> >> + running_ = false;
> >> +
> >> + if (::waitid(P_PIDFD, pidfd_.get(), &info, WNOHANG | WEXITED) >= 0) {
> >> + ASSERT(info.si_signo == SIGCHLD);
> >
> > Is it worth checking this ? The man page tells si_signo is always set to
> > SIGCHLD.
> >
> >> + ASSERT(info.si_pid == pid_);
> >
> > Something would be seriously wrong if this assertion fails, I'm not sure
> > it's needed.
>
> I suppose that's fair, but I'd like to keep at least the second one.
Out of curiosity, do you expect it could ever happen ?
> >> +
> >> + LOG(Process, Debug)
> >> + << this << "[" << pid_ << ":" << pidfd_.get() << "] "
> >> + "code:" << info.si_code << " status:" << info.si_status;
> >
> > Let's not split strings in the middle, and add missing spaces:
> >
> > << 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);
> >> + }
> >>
> >> + pid_ = -1;
> >> + pidfd_.reset();
> >> + pidfdNotify_.reset();
> >
> > Could all this be a member function instead of a lambda please ? Using a
> > lambda function make the code more difficult to read.
>
> I like lambdas a lot. :( My "issue" is that private member functions bleed
> into the public interface, e.g. triggering recompilations on changes that are
> completely private and irrelevant to the outside. This was also the motivation
> for moving `closeAllFdsExcept()`.
I understand that, especially for public APIs, but in this case the
impact on readability far outweights the gains we would get from
avoiding recompilation in my opinion.
> >> + });
> >> + } else {
> >> closeAllFdsExcept(fds);
> >>
> >> const char *file = utils::secure_getenv("LIBCAMERA_LOG_FILE");
> >> if (file && strcmp(file, "syslog"))
> >> unsetenv("LIBCAMERA_LOG_FILE");
> >>
> >> - const char **argv = new const char *[args.size() + 2];
> >> - unsigned int len = args.size();
> >> + const size_t len = args.size();
> >> + const char **argv = new const char *[len + 2];
> >> argv[0] = path.c_str();
> >> - for (unsigned int i = 0; i < len; i++)
> >> + for (size_t i = 0; i < len; i++)
> >> argv[i + 1] = args[i].c_str();
> >> argv[len + 1] = nullptr;
> >>
> >> - execv(path.c_str(), (char **)argv);
> >> + execv(path.c_str(), const_cast<char * const *>(argv));
> >
> > All of this could also be done in a separate patch for misc cleanups.
> >
> >>
> >> exit(EXIT_FAILURE);
> >> }
> >> -}
> >> -
> >> -void Process::closeAllFdsExcept(const std::vector<int> &fds)
> >> -{
> >> - std::vector<int> v(fds);
> >> - sort(v.begin(), v.end());
> >> -
> >> - DIR *dir = opendir("/proc/self/fd");
> >> - if (!dir)
> >> - return;
> >> -
> >> - int dfd = dirfd(dir);
> >> -
> >> - struct dirent *ent;
> >> - while ((ent = readdir(dir)) != nullptr) {
> >> - char *endp;
> >> - int fd = strtoul(ent->d_name, &endp, 10);
> >> - if (*endp)
> >> - continue;
> >> -
> >> - if (fd >= 0 && fd != dfd &&
> >> - !std::binary_search(v.begin(), v.end(), fd))
> >> - close(fd);
> >> - }
> >> -
> >> - closedir(dir);
> >> -}
> >> -
> >> -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)
> >> -{
> >> - running_ = false;
> >> - exitStatus_ = WIFEXITED(wstatus) ? NormalExit : SignalExit;
> >> - exitCode_ = exitStatus_ == NormalExit ? WEXITSTATUS(wstatus) : -1;
> >> -
> >> - finished.emit(exitStatus_, exitCode_);
> >> -}
> >> -
> >> /**
> >> * \fn Process::exitStatus()
> >> * \brief Retrieve the exit status of the process
> >> @@ -368,8 +225,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 e9f5e7e9b..75de9563c 100644
> >> --- a/test/process/process_test.cpp
> >> +++ b/test/process/process_test.cpp
> >> @@ -87,8 +87,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