| Message ID | 20250121185741.302256-1-pobrn@protonmail.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Barnabás, Thank you for the patch. On Tue, Jan 21, 2025 at 06:57:44PM +0000, 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. This patch does a few other things: it replaces vector arguments to start() with spans, and make the closeAllFdsExcept() member function global. Splitting it in separate patches will make it easier to review. Additionally, usage of clone3() allows dropping the unshare() call, and that would be useful to mention in the commit message to facilitate reviews. > `clone3()` was introduced in Linux 5.3, so this change raises the > minimum supported kernel version. You should update the kernel_version_req variable in the main meson.build file. > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > --- > 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(-) Very nice simplification overall :-) > > 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 b1d07a5a5..9d476f3bf 100644 > --- a/include/libcamera/internal/process.h > +++ b/include/libcamera/internal/process.h > @@ -12,6 +12,7 @@ > #include <vector> > > #include <libcamera/base/signal.h> > +#include <libcamera/base/span.h> > #include <libcamera/base/unique_fd.h> > > namespace libcamera { > @@ -31,8 +32,8 @@ public: > ~Process(); > > int start(const std::string &path, > - const std::vector<std::string> &args = std::vector<std::string>(), > - const std::vector<int> &fds = std::vector<int>()); > + Span<const std::string> args = {}, > + Span<const int> fds = {}); > > ExitStatus exitStatus() const { return exitStatus_; } > int exitCode() const { return exitCode_; } > @@ -42,43 +43,13 @@ public: > Signal<enum ExitStatus, int> finished; > > private: > - void closeAllFdsExcept(const std::vector<int> &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<Process *> processes_; > - > - struct sigaction oldsa_; > - > - EventNotifier *sigEvent_; > - UniqueFD pipe_[2]; > + UniqueFD pidfd_; > + std::unique_ptr<EventNotifier> 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 <unistd.h> > #include <vector> > > +#include <linux/sched.h> > +#include <sched.h> > +#include <sys/syscall.h> > +#include <unistd.h> Those headers should be added to the previous group of headers. You're duplicating unistd.h here. > + > #include <libcamera/base/event_notifier.h> > #include <libcamera/base/log.h> > #include <libcamera/base/utils.h> > @@ -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<const int> fds) Any particular reason not to keep this as a member function ? > +{ > + std::vector<int> 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<std::string> &args, > - const std::vector<int> &fds) > + Span<const std::string> args, > + Span<const int> 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<uintptr_t>(&pidfd); > + cargs.exit_signal = SIGCHLD; Do we need to deliver SIGCHLD to the parent, now that we use the pidfd to be notified of termination of the child ? > + > + long childPid = ::syscall(SYS_clone3, &cargs, sizeof(cargs)); > + if (childPid < 0) { > + int ret = -errno; > LOG(Process, Error) << "Failed to fork: " << strerror(-ret); Maybe s/fork/clone/ > return ret; > - } else if (childPid) { > - pid_ = childPid; > - ProcessManager::instance()->registerProcess(this); > + } > > + if (childPid) { > running_ = true; > + pid_ = childPid; > + pidfd_ = UniqueFD(pidfd); > + pidfdNotify_ = std::make_unique<EventNotifier>(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); Is it worth checking this ? The man page tells si_signo is always set to SIGCHLD. > + ASSERT(info.si_pid == pid_); Something would be seriously wrong if this assertion fails, I'm not sure it's needed. > + > + LOG(Process, Debug) > + << this << "[" << pid_ << ":" << pidfd_.get() << "] " > + "code:" << info.si_code << " status:" << info.si_status; Let's not split strings in the middle, and add missing spaces: << 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(); Could all this be a member function instead of a lambda please ? Using a lambda function make the code more difficult to read. > + }); > + } 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<char * const *>(argv)); All of this could also be done in a separate patch for misc cleanups. > > exit(EXIT_FAILURE); > } > -} > - > -void Process::closeAllFdsExcept(const std::vector<int> &fds) > -{ > - std::vector<int> 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<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 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_;
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 b1d07a5a5..9d476f3bf 100644 --- a/include/libcamera/internal/process.h +++ b/include/libcamera/internal/process.h @@ -12,6 +12,7 @@ #include <vector> #include <libcamera/base/signal.h> +#include <libcamera/base/span.h> #include <libcamera/base/unique_fd.h> namespace libcamera { @@ -31,8 +32,8 @@ public: ~Process(); int start(const std::string &path, - const std::vector<std::string> &args = std::vector<std::string>(), - const std::vector<int> &fds = std::vector<int>()); + Span<const std::string> args = {}, + Span<const int> fds = {}); ExitStatus exitStatus() const { return exitStatus_; } int exitCode() const { return exitCode_; } @@ -42,43 +43,13 @@ public: Signal<enum ExitStatus, int> finished; private: - void closeAllFdsExcept(const std::vector<int> &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<Process *> processes_; - - struct sigaction oldsa_; - - EventNotifier *sigEvent_; - UniqueFD pipe_[2]; + UniqueFD pidfd_; + std::unique_ptr<EventNotifier> 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 <unistd.h> #include <vector> +#include <linux/sched.h> +#include <sched.h> +#include <sys/syscall.h> +#include <unistd.h> + #include <libcamera/base/event_notifier.h> #include <libcamera/base/log.h> #include <libcamera/base/utils.h> @@ -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<const int> fds) +{ + std::vector<int> 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<std::string> &args, - const std::vector<int> &fds) + Span<const std::string> args, + Span<const int> 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<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); + } + if (childPid) { running_ = true; + pid_ = childPid; + pidfd_ = UniqueFD(pidfd); + pidfdNotify_ = std::make_unique<EventNotifier>(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<char * const *>(argv)); exit(EXIT_FAILURE); } -} - -void Process::closeAllFdsExcept(const std::vector<int> &fds) -{ - std::vector<int> 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<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 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_;
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 <pobrn@protonmail.com> --- 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(-)