| Message ID | 20250325180821.1456154-1-barnabas.pocze@ideasonboard.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Quoting Barnabás Pőcze (2025-03-25 18:08:19) > Remove `closeAllFdsExcept()` from the `Process` class and have > it as a local function since it is no needed "outside" and does > not depend on any part of the `Process` class. Any impact on https://patchwork.libcamera.org/patch/22403/ ? Could you review that please? > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > --- > include/libcamera/internal/process.h | 1 - > src/libcamera/process.cpp | 56 ++++++++++++++-------------- > 2 files changed, 28 insertions(+), 29 deletions(-) > > diff --git a/include/libcamera/internal/process.h b/include/libcamera/internal/process.h > index e4f5bb979..14391d60a 100644 > --- a/include/libcamera/internal/process.h > +++ b/include/libcamera/internal/process.h > @@ -44,7 +44,6 @@ public: > private: > LIBCAMERA_DISABLE_COPY_AND_MOVE(Process) > > - void closeAllFdsExcept(Span<const int> fds); It's already private though isn't it ? Is it just better to keep functions isolated if they aren't directly using the class ? > int isolate(); > void died(int wstatus); > > diff --git a/src/libcamera/process.cpp b/src/libcamera/process.cpp > index 5603dc903..01503e485 100644 > --- a/src/libcamera/process.cpp > +++ b/src/libcamera/process.cpp > @@ -63,6 +63,34 @@ void sigact(int signal, siginfo_t *info, void *ucontext) > } > } > > +void closeAllFdsExcept(Span<const int> fds) static? or is this already in an annoymous namespace or such ? > +{ > + std::vector<int> v(fds.begin(), fds.end()); > + std::sort(v.begin(), v.end()); > + > + ASSERT(v.empty() || v.front() >= 0); > + > + 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 */ > > void ProcessManager::sighandler() > @@ -282,34 +310,6 @@ int Process::start(const std::string &path, > } > } > > -void Process::closeAllFdsExcept(Span<const int> fds) > -{ > - std::vector<int> v(fds.begin(), fds.end()); > - sort(v.begin(), v.end()); > - > - ASSERT(v.empty() || v.front() >= 0); > - > - 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); > -- > 2.49.0 >
Quoting Barnabás Pőcze (2025-03-25 18:08:14) > Firstly, get the number of arguments first, and use that to determine the > size of the allocation instead of retrieving it twice. > > Secondly, use `const_cast` instead of a C-style cast when calling `execv()`. > > Third, use `size_t` to match the type of `args.size()`. > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > --- > src/libcamera/process.cpp | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/src/libcamera/process.cpp b/src/libcamera/process.cpp > index 68fad3270..567b878d4 100644 > --- a/src/libcamera/process.cpp > +++ b/src/libcamera/process.cpp > @@ -265,14 +265,15 @@ int Process::start(const std::string &path, > if (file && strcmp(file, "syslog")) > unsetenv("LIBCAMERA_LOG_FILE"); > > - const char **argv = new const char *[args.size() + 2]; > - unsigned int len = args.size(); > + size_t len = args.size(); > + auto argv = std::make_unique<const char *[]>(len + 2); There's also a unique_ptr instaed of a 'new'. Will that have any side effect at the execv ? I assume not ... Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > + > 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 **>(argv.get())); > > exit(EXIT_FAILURE); > } > -- > 2.49.0 >
Quoting Barnabás Pőcze (2025-03-25 18:08:15) > Instead of using a separate member variable, use `pid_ > 0` to determine > if the process is still running. Previously the value of `pid_` was not > reset to -1 when the process terminated, but since it is only meaningful > while the process is running, reset it to -1 in `Process::died()`. > > Neither `pid_` nor `running_` are exposed, so this change has no effect > on the public interface or observable behaviour. > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > include/libcamera/internal/process.h | 1 - > src/libcamera/process.cpp | 8 +++----- > 2 files changed, 3 insertions(+), 6 deletions(-) > > diff --git a/include/libcamera/internal/process.h b/include/libcamera/internal/process.h > index 58e3e41ce..030a1e66e 100644 > --- a/include/libcamera/internal/process.h > +++ b/include/libcamera/internal/process.h > @@ -49,7 +49,6 @@ private: > void died(int wstatus); > > pid_t pid_; > - bool running_; > enum ExitStatus exitStatus_; > int exitCode_; > > diff --git a/src/libcamera/process.cpp b/src/libcamera/process.cpp > index 567b878d4..aa9e8f519 100644 > --- a/src/libcamera/process.cpp > +++ b/src/libcamera/process.cpp > @@ -208,7 +208,7 @@ const struct sigaction &ProcessManager::oldsa() const > */ > > Process::Process() > - : pid_(-1), running_(false), exitStatus_(NotExited), exitCode_(0) > + : pid_(-1), exitStatus_(NotExited), exitCode_(0) > { > } > > @@ -240,7 +240,7 @@ int Process::start(const std::string &path, > { > int ret; > > - if (running_) > + if (pid_ > 0) > return 0; > > int childPid = fork(); > @@ -252,8 +252,6 @@ int Process::start(const std::string &path, > pid_ = childPid; > ProcessManager::instance()->registerProcess(this); > > - running_ = true; > - > return 0; > } else { > if (isolate()) > @@ -327,7 +325,7 @@ int Process::isolate() > */ > void Process::died(int wstatus) > { > - running_ = false; > + pid_ = -1; > exitStatus_ = WIFEXITED(wstatus) ? NormalExit : SignalExit; > exitCode_ = exitStatus_ == NormalExit ? WEXITSTATUS(wstatus) : -1; > > -- > 2.49.0 >
On Tue, Mar 25, 2025 at 07:09:37PM +0000, Kieran Bingham wrote: > Quoting Barnabás Pőcze (2025-03-25 18:08:19) > > Remove `closeAllFdsExcept()` from the `Process` class and have > > it as a local function since it is no needed "outside" and does > > not depend on any part of the `Process` class. > > Any impact on https://patchwork.libcamera.org/patch/22403/ ? Could you > review that please? > > > > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > --- > > include/libcamera/internal/process.h | 1 - > > src/libcamera/process.cpp | 56 ++++++++++++++-------------- > > 2 files changed, 28 insertions(+), 29 deletions(-) > > > > diff --git a/include/libcamera/internal/process.h b/include/libcamera/internal/process.h > > index e4f5bb979..14391d60a 100644 > > --- a/include/libcamera/internal/process.h > > +++ b/include/libcamera/internal/process.h > > @@ -44,7 +44,6 @@ public: > > private: > > LIBCAMERA_DISABLE_COPY_AND_MOVE(Process) > > > > - void closeAllFdsExcept(Span<const int> fds); > > It's already private though isn't it ? > > Is it just better to keep functions isolated if they aren't directly > using the class ? The rationale for not including members in the class definition, even as private members, is two-fold: - It avoids ABI breakages - It avoids recompilation of users of the class The former is only applicable to member data, and member virtual functions. Adding, removing, or changing the signature of a non-virtual member function doesn't affect the ABI. The latter is true in all cases, but the practical impact is limited, unless you routinely make modifications to a class whose header file is widely included. The downsides of avoiding private member functions is a decrease (in my opinion) of readability. Making them non-member static functions possibly requires passing member data to the function. It also decouples the function from the class, which can make the code harder to follow. In this specific case the function didn't use any member data, and the readability isn't very negatively impacted. I am therefore not opposed to this patch, even if I probably wouldn't have authored it. I think it's partly a matter of personal preference. > > int isolate(); > > void died(int wstatus); > > > > diff --git a/src/libcamera/process.cpp b/src/libcamera/process.cpp > > index 5603dc903..01503e485 100644 > > --- a/src/libcamera/process.cpp > > +++ b/src/libcamera/process.cpp > > @@ -63,6 +63,34 @@ void sigact(int signal, siginfo_t *info, void *ucontext) > > } > > } > > > > +void closeAllFdsExcept(Span<const int> fds) > > static? or is this already in an annoymous namespace or such ? It's already in an anonymous namespace. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > +{ > > + std::vector<int> v(fds.begin(), fds.end()); > > + std::sort(v.begin(), v.end()); > > + > > + ASSERT(v.empty() || v.front() >= 0); > > + > > + 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 */ > > > > void ProcessManager::sighandler() > > @@ -282,34 +310,6 @@ int Process::start(const std::string &path, > > } > > } > > > > -void Process::closeAllFdsExcept(Span<const int> fds) > > -{ > > - std::vector<int> v(fds.begin(), fds.end()); > > - sort(v.begin(), v.end()); > > - > > - ASSERT(v.empty() || v.front() >= 0); > > - > > - 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);
Hi Barnabás, Thank you for the patch. On Tue, Mar 25, 2025 at 07:08:21PM +0100, 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. > > `P_PIDFD` for `waitid()` was introduced in Linux 5.4, so this change > raises the minimum supported kernel version. `clone3()`, `CLONE_PIDFD`, > `pidfd_send_signal()` were all introduced earlier. > > Furthermore, the call to the `unshare()` system call can be removed > as those options can be passed to `clone3()` directly. > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > --- > include/libcamera/internal/camera_manager.h | 1 - > include/libcamera/internal/process.h | 34 +-- > meson.build | 2 +- > src/libcamera/process.cpp | 241 +++++--------------- > test/ipc/unixsocket_ipc.cpp | 2 - > test/log/log_process.cpp | 2 - > test/process/process_test.cpp | 2 - > 7 files changed, 55 insertions(+), 229 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 14391d60a..e600a49f8 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/signal.h> > @@ -44,41 +43,14 @@ public: > private: > LIBCAMERA_DISABLE_COPY_AND_MOVE(Process) > > - int isolate(); > - void died(int wstatus); > - > 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_; > + UniqueFD pidfd_; > + std::unique_ptr<EventNotifier> pidfdNotify_; > > - EventNotifier *sigEvent_; > - UniqueFD pipe_[2]; > + void onPidfdNotify(); We declare member functions before member variables. > }; > > } /* namespace libcamera */ > diff --git a/meson.build b/meson.build > index 00291d628..bd2c88dfa 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 5fa813300..cadf5c43e 100644 > --- a/src/libcamera/process.cpp > +++ b/src/libcamera/process.cpp > @@ -10,15 +10,19 @@ > #include <algorithm> > #include <dirent.h> > #include <fcntl.h> > -#include <list> > +#include <sched.h> > #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 +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<const int> fds) > { > std::vector<int> v(fds.begin(), fds.end()); > @@ -113,129 +88,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 > @@ -286,8 +138,6 @@ int Process::start(const std::string &path, > Span<const std::string> args, > Span<const int> fds) > { > - int ret; > - > if (pid_ > 0) > return -EBUSY; > > @@ -296,20 +146,26 @@ int Process::start(const std::string &path, > return -EINVAL; > } > > - 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); > + } > > - return 0; > + if (childPid) { > + pid_ = childPid; > + pidfd_ = UniqueFD(pidfd); > + pidfdNotify_ = std::make_unique<EventNotifier>(pidfd_.get(), EventNotifier::Type::Read); > + pidfdNotify_->activated.connect(this, &Process::onPidfdNotify); > } else { > - if (isolate()) > - _exit(EXIT_FAILURE); > - > closeAllFdsExcept(fds); > > const char *file = utils::secure_getenv("LIBCAMERA_LOG_FILE"); > @@ -328,35 +184,40 @@ 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); Do we have to guard against this, or could we just write pidfdNotify_.reset(); ? > + ASSERT(pidfd.isValid()); > + ASSERT(pid > 0); > + > + siginfo_t info; > + > + 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() << "]" Sounds like a candidate to inherit from Loggable and avoid duplication of this in log message. You would need to reset pid_ to -1 at the end of the function though. Except it should be done before emitting the finished signal, as there's no running_ variable anymore. Annoying... I wonder if we should keep running_, or if there's a cleaner way. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + << " 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); > + } > } > > /** > @@ -394,8 +255,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_;
Hi 2025. 03. 26. 16:03 keltezéssel, Laurent Pinchart írta: > Hi Barnabás, > > Thank you for the patch. > > On Tue, Mar 25, 2025 at 07:08:21PM +0100, 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. >> >> `P_PIDFD` for `waitid()` was introduced in Linux 5.4, so this change >> raises the minimum supported kernel version. `clone3()`, `CLONE_PIDFD`, >> `pidfd_send_signal()` were all introduced earlier. >> >> Furthermore, the call to the `unshare()` system call can be removed >> as those options can be passed to `clone3()` directly. >> >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >> --- >> include/libcamera/internal/camera_manager.h | 1 - >> include/libcamera/internal/process.h | 34 +-- >> meson.build | 2 +- >> src/libcamera/process.cpp | 241 +++++--------------- >> test/ipc/unixsocket_ipc.cpp | 2 - >> test/log/log_process.cpp | 2 - >> test/process/process_test.cpp | 2 - >> 7 files changed, 55 insertions(+), 229 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 14391d60a..e600a49f8 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/signal.h> >> @@ -44,41 +43,14 @@ public: >> private: >> LIBCAMERA_DISABLE_COPY_AND_MOVE(Process) >> >> - int isolate(); >> - void died(int wstatus); >> - >> 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_; >> + UniqueFD pidfd_; >> + std::unique_ptr<EventNotifier> pidfdNotify_; >> >> - EventNotifier *sigEvent_; >> - UniqueFD pipe_[2]; >> + void onPidfdNotify(); > > We declare member functions before member variables. > >> }; >> >> } /* namespace libcamera */ >> diff --git a/meson.build b/meson.build >> index 00291d628..bd2c88dfa 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 5fa813300..cadf5c43e 100644 >> --- a/src/libcamera/process.cpp >> +++ b/src/libcamera/process.cpp >> @@ -10,15 +10,19 @@ >> #include <algorithm> >> #include <dirent.h> >> #include <fcntl.h> >> -#include <list> >> +#include <sched.h> >> #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 +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<const int> fds) >> { >> std::vector<int> v(fds.begin(), fds.end()); >> @@ -113,129 +88,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 >> @@ -286,8 +138,6 @@ int Process::start(const std::string &path, >> Span<const std::string> args, >> Span<const int> fds) >> { >> - int ret; >> - >> if (pid_ > 0) >> return -EBUSY; >> >> @@ -296,20 +146,26 @@ int Process::start(const std::string &path, >> return -EINVAL; >> } >> >> - 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); >> + } >> >> - return 0; >> + if (childPid) { >> + pid_ = childPid; >> + pidfd_ = UniqueFD(pidfd); >> + pidfdNotify_ = std::make_unique<EventNotifier>(pidfd_.get(), EventNotifier::Type::Read); >> + pidfdNotify_->activated.connect(this, &Process::onPidfdNotify); >> } else { >> - if (isolate()) >> - _exit(EXIT_FAILURE); >> - >> closeAllFdsExcept(fds); >> >> const char *file = utils::secure_getenv("LIBCAMERA_LOG_FILE"); >> @@ -328,35 +184,40 @@ 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); > > Do we have to guard against this, or could we just write > > pidfdNotify_.reset(); > > ? > >> + ASSERT(pidfd.isValid()); >> + ASSERT(pid > 0); >> + >> + siginfo_t info; >> + >> + 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() << "]" > > Sounds like a candidate to inherit from Loggable and avoid duplication > of this in log message. You would need to reset pid_ to -1 at the end of > the function though. Except it should be done before emitting the > finished signal, as there's no running_ variable anymore. Annoying... I > wonder if we should keep running_, or if there's a cleaner way. I'll give it some thought because this indeed does not look ideal. Also, regrettably it seems chromeos does not have new enough linux headers. I'm wondering if it would be possible to update the CI image to something newer? I have tested release-R135-16209.B locally, and that appears to have the necessary definitions. Regards, Barnabás Pőcze > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> + << " 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); >> + } >> } >> >> /** >> @@ -394,8 +255,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_; >
On Thu, Mar 27, 2025 at 05:13:05PM +0100, Barnabás Pőcze wrote: > 2025. 03. 26. 16:03 keltezéssel, Laurent Pinchart írta: > > On Tue, Mar 25, 2025 at 07:08:21PM +0100, 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. > >> > >> `P_PIDFD` for `waitid()` was introduced in Linux 5.4, so this change > >> raises the minimum supported kernel version. `clone3()`, `CLONE_PIDFD`, > >> `pidfd_send_signal()` were all introduced earlier. > >> > >> Furthermore, the call to the `unshare()` system call can be removed > >> as those options can be passed to `clone3()` directly. > >> > >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > >> --- > >> include/libcamera/internal/camera_manager.h | 1 - > >> include/libcamera/internal/process.h | 34 +-- > >> meson.build | 2 +- > >> src/libcamera/process.cpp | 241 +++++--------------- > >> test/ipc/unixsocket_ipc.cpp | 2 - > >> test/log/log_process.cpp | 2 - > >> test/process/process_test.cpp | 2 - > >> 7 files changed, 55 insertions(+), 229 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 14391d60a..e600a49f8 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/signal.h> > >> @@ -44,41 +43,14 @@ public: > >> private: > >> LIBCAMERA_DISABLE_COPY_AND_MOVE(Process) > >> > >> - int isolate(); > >> - void died(int wstatus); > >> - > >> 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_; > >> + UniqueFD pidfd_; > >> + std::unique_ptr<EventNotifier> pidfdNotify_; > >> > >> - EventNotifier *sigEvent_; > >> - UniqueFD pipe_[2]; > >> + void onPidfdNotify(); > > > > We declare member functions before member variables. > > > >> }; > >> > >> } /* namespace libcamera */ > >> diff --git a/meson.build b/meson.build > >> index 00291d628..bd2c88dfa 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 5fa813300..cadf5c43e 100644 > >> --- a/src/libcamera/process.cpp > >> +++ b/src/libcamera/process.cpp > >> @@ -10,15 +10,19 @@ > >> #include <algorithm> > >> #include <dirent.h> > >> #include <fcntl.h> > >> -#include <list> > >> +#include <sched.h> > >> #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 +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<const int> fds) > >> { > >> std::vector<int> v(fds.begin(), fds.end()); > >> @@ -113,129 +88,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 > >> @@ -286,8 +138,6 @@ int Process::start(const std::string &path, > >> Span<const std::string> args, > >> Span<const int> fds) > >> { > >> - int ret; > >> - > >> if (pid_ > 0) > >> return -EBUSY; > >> > >> @@ -296,20 +146,26 @@ int Process::start(const std::string &path, > >> return -EINVAL; > >> } > >> > >> - 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); > >> + } > >> > >> - return 0; > >> + if (childPid) { > >> + pid_ = childPid; > >> + pidfd_ = UniqueFD(pidfd); > >> + pidfdNotify_ = std::make_unique<EventNotifier>(pidfd_.get(), EventNotifier::Type::Read); > >> + pidfdNotify_->activated.connect(this, &Process::onPidfdNotify); > >> } else { > >> - if (isolate()) > >> - _exit(EXIT_FAILURE); > >> - > >> closeAllFdsExcept(fds); > >> > >> const char *file = utils::secure_getenv("LIBCAMERA_LOG_FILE"); > >> @@ -328,35 +184,40 @@ 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); > > > > Do we have to guard against this, or could we just write > > > > pidfdNotify_.reset(); > > > > ? > > > >> + ASSERT(pidfd.isValid()); > >> + ASSERT(pid > 0); > >> + > >> + siginfo_t info; > >> + > >> + 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() << "]" > > > > Sounds like a candidate to inherit from Loggable and avoid duplication > > of this in log message. You would need to reset pid_ to -1 at the end of > > the function though. Except it should be done before emitting the > > finished signal, as there's no running_ variable anymore. Annoying... I > > wonder if we should keep running_, or if there's a cleaner way. > > I'll give it some thought because this indeed does not look ideal. > > Also, regrettably it seems chromeos does not have new enough linux headers. > I'm wondering if it would be possible to update the CI image to something newer? > I have tested release-R135-16209.B locally, and that appears to have the > necessary definitions. That would require a recent image for Soraka, and I don't know if we can obtain one. Without that, we won't be able to run test on Chrome OS anymore. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > >> + << " 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); > >> + } > >> } > >> > >> /** > >> @@ -394,8 +255,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_;