[libcamera-devel] [PATCH] utils: ipc: Assign a new gid to proxy worker
Umang Jain
umang.jain at ideasonboard.com
Tue Jul 27 06:32:16 CEST 2021
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) {
>>>>
More information about the libcamera-devel
mailing list