[libcamera-devel] [PATCH] utils: ipc: Assign a new gid to proxy worker

[Umang Jain]

Hi Paul,

Thanks for the review.

On 7/27/21 9:56 AM, paul.elder at ideasonboard.com wrote:
> Hi,
>
> On Mon, Jul 26, 2021 at 07:21:43PM +0300, Laurent Pinchart wrote:
>> Hello,
>>
>> On Mon, Jul 26, 2021 at 05:01:14PM +0100, Kieran Bingham wrote:
>>> On 26/07/2021 11:31, Umang Jain wrote:
>>>> Doing so, handles the situation where the proxy worker
>>> We wouldn't normally open the commit message with a statement which
>>> follows on from the subject. They're somewhat distinct.
>>>
>>> The subject/title should be a just that, which in this case is fine, but
>>> then the commit message should be something along the lines of:
>>>
>>>   - Introduce the issue
>>>   - Describe what the patch does to solve it.
>>>
>>> Imagine reading the commit message without the title:
>>>
>>> "Doing so," ... leaves me thinking ... doing what?
>>>
>>> So we should write this as something like:
>>>
>>> """
>>> Isolated IPAs are forked to a new process by the proxy worker, which
>>> shares the same process group. This allows the undesired effect that the
>>> The proxy worker will receive signals such as SIGINT and will be closed
>>> by a Ctrl-C event before the pipeline handlers have been able to fully
>>> clean up.
>>>
>>> Prevent this signal from being delivered to the proxy worker by moving
>>> the process to a new process group, matching the pid of the isolated proxy.
>>>
>>> """
>>>
>>>> has been shutdown by SIGINT/SIGTERM preemptively, before
>>>> the pipeline handler got a chance to cleanup nicely.
>>>>
>>>> Bug: https://bugs.libcamera.org/show_bug.cgi?id=60
>>>> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> \o/
>
>>> Ah, excellent:
>>>
>>> Tested-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>>>
>>>> ---
>>>>   .../module_ipa_proxy_worker.cpp.tmpl                   | 10 ++++++++++
>>>>   1 file changed, 10 insertions(+)
>>>>
>>>> diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl
>>>> index d993e39e..32ac3869 100644
>>>> --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl
>>>> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl
>>>> @@ -207,6 +207,16 @@ int main(int argc, char **argv)
>>>>   		return EXIT_FAILURE;
>>>>   	}
>>>>   
>>>> +	/*
>>>> +	 * Shutdown of proxy worker can be preemptive by events like
>>> s/preemptive/pre-empted/ ?
>> Or "preempt" ? Interestingly, there's also "preëmpt", but it's rare :-)
> "pre-empted" I think is the right choice here, since it's "<subject> can
> be pre-empted by <object>".

Yes, "pre-empted" seems correct now (I just put the v2 on ML). Will 
address this while pushing.

Thanks!

>
>>>> +	 * SIGINT/SIGTERM, even before the pipeline handler can request
>>>> +	 * shutdown. Hence, assign a new gid to handle these situations.
>>> "assign a new gid to prevent signals on the application being delivered
>>> to the proxy." ... ?
>>>
>>> Otherwise,
>>>
>>> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>>>
>>>> +	 */
>>>> +	if (setpgid(0, 0) < -1) {
>>>> +		LOG({{proxy_worker_name}}, Warning)
>>>> +			<< "Failed to set new gid: " << strerror(errno);
>> The LOG() call may end up changing errno. You should write
>>
>> 		int err = errno;
>> 		LOG({{proxy_worker_name}}, Warning)
>> 			<< "Failed to set new gid: " << strerror(err);
>>
>> With this fixed, as well ass the commit message and comments,
>>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> Laurent and Kieran have covered all my comments :p
>
> With those fixed,
>
> Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
>
>>>> +	}
>>>> +
>>>>   	{{proxy_worker_name}} proxyWorker;
>>>>   	int ret = proxyWorker.init(ipam, fd);
>>>>   	if (ret < 0) {
>>>>