[RFC PATCH v3 9/9] libcamera: process: Remove `ProcessManager` singleton
Barnabás Pőcze
barnabas.pocze at ideasonboard.com
Thu Apr 24 12:56:03 CEST 2025
Hi
2025. 03. 26. 16:03 keltezéssel, Laurent Pinchart írta:
> Hi Barnabás,
>
> Thank you for the patch.
>
> 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();
>
> ?
It would be sufficient, but I like to check that the state of the object
is what is expected (necessary) when this function is called.
>
>> + 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.
Even with `running_`, I think the same problem applies to `pidFd_` because
it has to be closed, but if the `finished` signal handler starts the process
again, then it must not be closed. So one would have to duplicate `pidFd_.reset()`
in both branches.
I hope the duplication in the log message is acceptable because it keeps the
actual logic simpler (i.e. reset object state at the beginning unconditionally).
Regards,
Barnabás Pőcze
>
> 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_;
>
More information about the libcamera-devel
mailing list