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

[Kieran Bingham]

Hi Umang,

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/ ?

> +	 * 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);
> +	}
> +
>  	{{proxy_worker_name}} proxyWorker;
>  	int ret = proxyWorker.init(ipam, fd);
>  	if (ret < 0) {
>