[RFC PATCH v2] libcamera: process: Remove `ProcessManager` singleton
Barnabás Pőcze
pobrn at protonmail.com
Wed Jan 22 09:53:26 CET 2025
2025. január 22., szerda 0:51 keltezéssel, Laurent Pinchart <laurent.pinchart at ideasonboard.com> írta:
> Hi Barnabás,
>
> Thank you for the patch.
>
> 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.
Regards,
Barnabás Pőcze
>
> >
> > 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