[RFC PATCH v3 9/9] libcamera: process: Remove `ProcessManager` singleton
Barnabás Pőcze
barnabas.pocze at ideasonboard.com
Tue Mar 25 19:08:21 CET 2025
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();
};
} /* 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);
+ 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() << "]"
+ << " 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_;
--
2.49.0
More information about the libcamera-devel
mailing list