[RFC PATCH v2] libcamera: process: Remove `ProcessManager` singleton
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Jan 22 11:17:31 CET 2025
On Wed, Jan 22, 2025 at 08:53:26AM +0000, Barnabás Pőcze wrote:
> 2025. január 22., szerda 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.
>
> Sorry, yes, I know it is far from perfect, I just wanted to see if this approach
> is acceptable (with the kernel version requirement increase, etc.) before I
> spend more time on this.
I like this patch very much :-) I would be very happy to see the
ProcessManager singleton before removed.
> > > 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 ?
> >
> > > +
> > > + 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/
> >
> > > 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.
> >
> > > +
> > > + 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.
> >
> > > + });
> > > + } 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