[RFC PATCH v2] libcamera: process: Remove `ProcessManager` singleton

[Barnabás Pőcze]

Hi


2025. 03. 24. 16:41 keltezéssel, Laurent Pinchart írta:
> Hi Barnabás,
> 
> On Mon, Mar 24, 2025 at 12:46:05PM +0100, Barnabás Pőcze wrote:
>> 2025. 01. 22. 0:51 keltezéssel, Laurent Pinchart írta:
>>> On Tue, Jan 21, 2025 at 06:57:44PM +0000, Barnabás Pőcze wrote:
>>>> The `ProcessManager` is a singleton class to handle `SIGCHLD` signals
>>>> and report the exit status to the particular `Process` instances.
>>>>
>>>> However, having a singleton in a library is not favourable and it is
>>>> even less favourable if it installs a signal handler.
>>>>
>>>> Using pidfd it is possible to avoid the need for the signal handler;
>>>> and the `Process` objects can watch their pidfd themselves, eliminating
>>>> the need for the `ProcessManager` class altogether.
>>>
>>> This patch does a few other things: it replaces vector arguments to
>>> start() with spans, and make the closeAllFdsExcept() member function
>>> global. Splitting it in separate patches will make it easier to review.
>>>
>>> Additionally, usage of clone3() allows dropping the unshare() call, and
>>> that would be useful to mention in the commit message to facilitate
>>> reviews.
>>>
>>>> `clone3()` was introduced in Linux 5.3, so this change raises the
>>>> minimum supported kernel version.
>>>
>>> You should update the kernel_version_req variable in the main
>>> meson.build file.
>>>
>>>>
>>>> Signed-off-by: Barnabás Pőcze <pobrn at protonmail.com>
>>>> ---
>>>>    include/libcamera/internal/camera_manager.h |   1 -
>>>>    include/libcamera/internal/process.h        |  39 +--
>>>>    src/libcamera/process.cpp                   | 315 ++++++--------------
>>>>    test/ipc/unixsocket_ipc.cpp                 |   2 -
>>>>    test/log/log_process.cpp                    |   2 -
>>>>    test/process/process_test.cpp               |   2 -
>>>>    6 files changed, 91 insertions(+), 270 deletions(-)
> [...]
>>>> +	if (childPid) {
>>>>    		running_ = true;
>>>> +		pid_ = childPid;
>>>> +		pidfd_ = UniqueFD(pidfd);
>>>> +		pidfdNotify_ = std::make_unique<EventNotifier>(pidfd_.get(), EventNotifier::Type::Read);
>>>>    
>>>> -		return 0;
>>>> -	} else {
>>>> -		if (isolate())
>>>> -			_exit(EXIT_FAILURE);
>>>> +		pidfdNotify_->activated.connect(this, [this] {
>>>> +			::siginfo_t info;
>>>> +
>>>> +			running_ = false;
>>>> +
>>>> +			if (::waitid(P_PIDFD, pidfd_.get(), &info, WNOHANG | WEXITED) >= 0) {
>>>> +				ASSERT(info.si_signo == SIGCHLD);
>>>
>>> Is it worth checking this ? The man page tells si_signo is always set to
>>> SIGCHLD.
>>>
>>>> +				ASSERT(info.si_pid == pid_);
>>>
>>> Something would be seriously wrong if this assertion fails, I'm not sure
>>> it's needed.
>>
>> I suppose that's fair, but I'd like to keep at least the second one.
> 
> Out of curiosity, do you expect it could ever happen ?

I generally do not expect an assertion to ever trigger during normal operations.
Same applies here, unless there is some kind of memory error, file descriptor
mishap, kernel bug, etc. I wouldn't expect `info.si_pid == pid_` to ever fail.


> [...]

Regards,
Barnabás Pőcze