| Message ID | 20250424114103.451395-10-barnabas.pocze@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Barnabás Pőcze (2025-04-24 12:41:03) > 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. Do we still have the segfault signal handler? I thought we had some custom handler to print a backtrace somewhere... I guess that's separate. > 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. I know we've had issues in CI tests before with unshare() so I think that being removed sounds good, except I expect the same permission requirements of unshare() on the clone3() so I guess it won't change much. Is there anything to explore here that the process we launch can have custom restrictions/jails added? (or is that where we would have to handle things in a separate daemon context?) Anyway, that's a lot of code removed, which if it doesn't mean a loss of functionality is a good thing. > 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(-) > > 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); I wonder if there's any extra way we can get custom messages from the IPA's over to the main process to report /why/ they shut down in event of failures. So frequently when IPA's are isolated - do we get difficult to support issues from users who's IPA isn't running - and we don't report in libcamera any thing about /why/ Anyway - I don't see anything wrong with this patch so : Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > + } > } > > /** > @@ -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_; > -- > 2.49.0 >
On Sat, Apr 26, 2025 at 02:27:55PM +0100, Kieran Bingham wrote: > Quoting Barnabás Pőcze (2025-04-24 12:41:03) > > 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. > > Do we still have the segfault signal handler? I thought we had some > custom handler to print a backtrace somewhere... That's for ASSERT() and the fatal log level. We don't install a signal handler. > I guess that's separate. > > > 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. > > I know we've had issues in CI tests before with unshare() so I think > that being removed sounds good, except I expect the same permission > requirements of unshare() on the clone3() so I guess it won't change > much. > > Is there anything to explore here that the process we launch can have > custom restrictions/jails added? (or is that where we would have to > handle things in a separate daemon context?) It could be done here, but I think we should move it to a daemon. > Anyway, that's a lot of code removed, which if it doesn't mean a loss of > functionality is a good thing. > > > 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(-) > > > > 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); > > I wonder if there's any extra way we can get custom messages from the > IPA's over to the main process to report /why/ they shut down in event > of failures. > > So frequently when IPA's are isolated - do we get difficult to support > issues from users who's IPA isn't running - and we don't report in > libcamera any thing about /why/ > > Anyway - I don't see anything wrong with this patch so : > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > + } > > } > > > > /** > > @@ -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_;
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_;