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

[Laurent Pinchart]

On Tue, Jul 27, 2021 at 10:02:16AM +0530, Umang Jain wrote:
> On 7/27/21 9:56 AM, paul.elder at ideasonboard.com wrote:
> > On Mon, Jul 26, 2021 at 07:21:43PM +0300, Laurent Pinchart wrote:
> >> 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.

Sorry, I meant "preempted", not "preempt". Doesn't matter much :-)

> >>>> +	 * 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) {

-- 
Regards,

Laurent Pinchart