[RFC,v5,9/9] libcamera: process: Remove `ProcessManager` singleton
diff mbox series

Message ID 20250424114103.451395-10-barnabas.pocze@ideasonboard.com
State New
Headers show
Series
  • libcamera: process: Remove `ProcessManager` singleton
Related show

Commit Message

Barnabás Pőcze April 24, 2025, 11:41 a.m. UTC
The `ProcessManager` is a singleton class to handle `SIGCHLD` signals
and report the exit status to the particular `Process` instance.

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@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 include/libcamera/internal/camera_manager.h |   1 -
 include/libcamera/internal/process.h        |  34 +--
 meson.build                                 |   2 +-
 src/libcamera/process.cpp                   | 245 +++++---------------
 test/ipc/unixsocket_ipc.cpp                 |   2 -
 test/log/log_process.cpp                    |   2 -
 test/process/process_test.cpp               |   2 -
 7 files changed, 60 insertions(+), 228 deletions(-)

Patch
diff mbox series

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 e55d11fa3..27d4b9258 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/class.h>
@@ -45,41 +44,14 @@  public:
 private:
 	LIBCAMERA_DISABLE_COPY_AND_MOVE(Process)
 
-	int isolate();
-	void died(int wstatus);
+	void onPidfdNotify();
 
 	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_;
-
-	EventNotifier *sigEvent_;
-	UniqueFD pipe_[2];
+	UniqueFD pidfd_;
+	std::unique_ptr<EventNotifier> pidfdNotify_;
 };
 
 } /* namespace libcamera */
diff --git a/meson.build b/meson.build
index ae13727eb..3c895ae9e 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 42b56374d..b01c78034 100644
--- a/src/libcamera/process.cpp
+++ b/src/libcamera/process.cpp
@@ -10,15 +10,18 @@ 
 #include <algorithm>
 #include <dirent.h>
 #include <fcntl.h>
-#include <list>
 #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 +35,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());
@@ -118,129 +92,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
@@ -291,8 +142,6 @@  int Process::start(const std::string &path,
 		   Span<const std::string> args,
 		   Span<const int> fds)
 {
-	int ret;
-
 	if (pid_ > 0)
 		return -EBUSY;
 
@@ -301,20 +150,29 @@  int Process::start(const std::string &path,
 			return -EINVAL;
 	}
 
-	int childPid = fork();
-	if (childPid == -1) {
-		ret = -errno;
+	clone_args cargs = {};
+	int pidfd;
+
+	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) {
+	}
+
+	if (childPid) {
 		pid_ = childPid;
-		ProcessManager::instance()->registerProcess(this);
+		pidfd_ = UniqueFD(pidfd);
+		pidfdNotify_ = std::make_unique<EventNotifier>(pidfd_.get(), EventNotifier::Type::Read);
+		pidfdNotify_->activated.connect(this, &Process::onPidfdNotify);
 
-		return 0;
+		LOG(Process, Debug) << this << "[" << childPid << ':' << pidfd << "]"
+				    << " forked";
 	} else {
-		if (isolate())
-			_exit(EXIT_FAILURE);
-
 		closeAllFdsExcept(fds);
 
 		const char *file = utils::secure_getenv("LIBCAMERA_LOG_FILE");
@@ -333,35 +191,44 @@  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;
+
+	/*
+	 * `static_cast` is needed because `P_PIDFD` is not defined in `sys/wait.h` if the C standard library
+	 * is old enough. So `P_PIDFD` is taken from `linux/wait.h`, where it is just an integer #define.
+	 */
+	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);
+	}
 }
 
 /**
@@ -399,8 +266,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_;