From patchwork Tue Jan 21 18:57:44 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: 22609 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 0AA6DBD160 for ; Tue, 21 Jan 2025 18:57:53 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id B072568559; Tue, 21 Jan 2025 19:57:52 +0100 (CET) Authentication-Results: lancelot.ideasonboard.com; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=protonmail.com header.i=@protonmail.com header.b="G4FoLWpP"; dkim-atps=neutral Received: from mail-10629.protonmail.ch (mail-10629.protonmail.ch [79.135.106.29]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 2AB9A60380 for ; Tue, 21 Jan 2025 19:57:51 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com; s=protonmail3; t=1737485870; x=1737745070; bh=zOUqTXgPyEs3f0IAjySwj72fppOeQaAFIBsqCL1J2HM=; h=Date:To:From:Subject:Message-ID:Feedback-ID:From:To:Cc:Date: Subject:Reply-To:Feedback-ID:Message-ID:BIMI-Selector: List-Unsubscribe:List-Unsubscribe-Post; b=G4FoLWpPYiG0qFdj7h70M/2mrGvG2YPkqSwubYbdQBk3wRnaD7its11LBYFXfeaOy x7Sybg9DGzDF0mOIqoaVHtUbn3OL0iJO+vxuPx2204lJISv9qH/VCOP70n3dnGxYBO BJYZ1mEq+OmSp7cUAOi7zhoftdteu7DAS9T2GcgJlsC2wpm08nU1LBD0tRBXMoKfyT qdxgvawGcLpCVOPllnajFTdNjuCUWdt5CjzcPKPjQlqqXz4vg6uo9BylKCAEnYGiGg +ex4AG6DBMic4sv7Ge0PzCLL2NX9cEIr/hUB8RHDQBIvHqccedVXvszJgG11FTLIHJ mR7dwUAc6PEOQ== Date: Tue, 21 Jan 2025 18:57:44 +0000 To: libcamera-devel@lists.libcamera.org From: =?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= Subject: [RFC PATCH v2] libcamera: process: Remove `ProcessManager` singleton Message-ID: <20250121185741.302256-1-pobrn@protonmail.com> Feedback-ID: 20568564:user:proton X-Pm-Message-ID: f96bf18032157eb41363a6dfbe6cf0b5988d0d46 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` 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. `clone3()` was introduced in Linux 5.3, so this change raises the minimum supported kernel version. Signed-off-by: Barnabás Pőcze --- 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(-) 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 b1d07a5a5..9d476f3bf 100644 --- a/include/libcamera/internal/process.h +++ b/include/libcamera/internal/process.h @@ -12,6 +12,7 @@ #include #include +#include #include namespace libcamera { @@ -31,8 +32,8 @@ public: ~Process(); int start(const std::string &path, - const std::vector &args = std::vector(), - const std::vector &fds = std::vector()); + Span args = {}, + Span fds = {}); ExitStatus exitStatus() const { return exitStatus_; } int exitCode() const { return exitCode_; } @@ -42,43 +43,13 @@ public: Signal finished; private: - void closeAllFdsExcept(const std::vector &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 processes_; - - struct sigaction oldsa_; - - EventNotifier *sigEvent_; - UniqueFD pipe_[2]; + UniqueFD pidfd_; + std::unique_ptr 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 #include +#include +#include +#include +#include + #include #include #include @@ -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 fds) +{ + std::vector 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 &args, - const std::vector &fds) + Span args, + Span 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(&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); + } + if (childPid) { running_ = true; + pid_ = childPid; + pidfd_ = UniqueFD(pidfd); + pidfdNotify_ = std::make_unique(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); + ASSERT(info.si_pid == pid_); + + 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); + } + pid_ = -1; + pidfd_.reset(); + pidfdNotify_.reset(); + }); + } 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(argv)); exit(EXIT_FAILURE); } -} - -void Process::closeAllFdsExcept(const std::vector &fds) -{ - std::vector 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 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_;