From patchwork Thu Apr 24 11:22:02 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= X-Patchwork-Id: 23251 Return-Path: X-Original-To: parsemail@patchwork.libcamera.org Delivered-To: parsemail@patchwork.libcamera.org Received: from lancelot.ideasonboard.com (lancelot.ideasonboard.com [92.243.16.209]) by patchwork.libcamera.org (Postfix) with ESMTPS id C1933C3324 for ; Thu, 24 Apr 2025 11:22:29 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 7601168AD7; Thu, 24 Apr 2025 13:22:24 +0200 (CEST) Authentication-Results: lancelot.ideasonboard.com; dkim=pass (1024-bit key; unprotected) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="eC/KueJZ"; dkim-atps=neutral Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 70F1668AD8 for ; Thu, 24 Apr 2025 13:22:09 +0200 (CEST) Received: from pb-laptop.local (185.221.143.16.nat.pool.zt.hu [185.221.143.16]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 685C316A for ; Thu, 24 Apr 2025 13:22:07 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1745493727; bh=dNQ7Uo2BUTUD18/Av4Yqlj30bUMQ3mo9KsMR0e/491g=; h=From:To:Subject:Date:In-Reply-To:References:From; b=eC/KueJZrECT1QUtqY7ui33KJUIuzv5iSbjzRkgt1/ZzIp/VkfU2uM1q3W6eh4Av5 zCUCitMyOOTCRl/uu3fQbxS83aXINclQa5xCJVygnT3ekL3ziJVXGC3xQl7EC0/12T /FzayGJihu6+BfVljvpjDXx9c4kYiELeD5fJ7yDk= From: =?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= To: libcamera-devel@lists.libcamera.org Subject: [RFC PATCH v4 9/9] libcamera: process: Remove `ProcessManager` singleton Date: Thu, 24 Apr 2025 13:22:02 +0200 Message-ID: <20250424112203.445351-10-barnabas.pocze@ideasonboard.com> X-Mailer: git-send-email 2.49.0 In-Reply-To: <20250424112203.445351-1-barnabas.pocze@ideasonboard.com> References: <20250424112203.445351-1-barnabas.pocze@ideasonboard.com> MIME-Version: 1.0 X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" 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 --- include/libcamera/internal/camera_manager.h | 1 - include/libcamera/internal/process.h | 34 +-- meson.build | 2 +- src/libcamera/process.cpp | 246 +++++--------------- test/ipc/unixsocket_ipc.cpp | 2 - test/log/log_process.cpp | 2 - test/process/process_test.cpp | 2 - 7 files changed, 61 insertions(+), 228 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 enumerator_; std::unique_ptr 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 #include #include @@ -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 processes_; - - struct sigaction oldsa_; - - EventNotifier *sigEvent_; - UniqueFD pipe_[2]; + UniqueFD pidfd_; + std::unique_ptr pidfdNotify_; }; } /* namespace libcamera */ diff --git a/meson.build b/meson.build index 1525bdf4a..83c4b87de 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..124677038 100644 --- a/src/libcamera/process.cpp +++ b/src/libcamera/process.cpp @@ -10,15 +10,19 @@ #include #include #include -#include +#include #include #include #include +#include #include #include #include #include +#include +#include /* glibc only provides `P_PIDFD` in `sys/wait.h` since 2.36 */ + #include #include #include @@ -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 fds) { std::vector v(fds.begin(), fds.end()); @@ -118,129 +93,6 @@ void closeAllFdsExcept(Span 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 +143,6 @@ int Process::start(const std::string &path, Span args, Span fds) { - int ret; - if (pid_ > 0) return -EBUSY; @@ -301,20 +151,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(&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(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 +192,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(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 +267,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 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_;