[RFC PATCH v2] libcamera: process: Remove `ProcessManager` singleton
Barnabás Pőcze
barnabas.pocze at ideasonboard.com
Mon Mar 24 12:46:05 CET 2025
Hi
2025. 01. 22. 0:51 keltezéssel, Laurent Pinchart í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.
>
>>
>> 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.
>
>> +
>> + 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.
>
>> +
>> + 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()`.
>
>> + });
>> + } 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_;
>
More information about the libcamera-devel
mailing list