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

[paul.elder at ideasonboard.com]

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>".

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