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

[Laurent Pinchart]

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>
> 
> 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 :-)

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

> > +	}
> > +
> >  	{{proxy_worker_name}} proxyWorker;
> >  	int ret = proxyWorker.init(ipam, fd);
> >  	if (ret < 0) {
> > 

-- 
Regards,

Laurent Pinchart