[{"id":18363,"web_url":"https://patchwork.libcamera.org/comment/18363/","msgid":"<824b4696-0c3f-e70d-d6c8-f1e53452d3e4@ideasonboard.com>","date":"2021-07-26T16:01:14","subject":"Re: [libcamera-devel] [PATCH] utils: ipc: Assign a new gid to proxy\n\tworker","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Umang,\n\nOn 26/07/2021 11:31, Umang Jain wrote:\n> Doing so, handles the situation where the proxy worker\n\nWe wouldn't normally open the commit message with a statement which\nfollows on from the subject. They're somewhat distinct.\n\nThe subject/title should be a just that, which in this case is fine, but\nthen the commit message should be something along the lines of:\n\n - Introduce the issue\n - Describe what the patch does to solve it.\n\nImagine reading the commit message without the title:\n\n\"Doing so,\" ... leaves me thinking ... doing what?\n\nSo we should write this as something like:\n\n\"\"\"\nIsolated IPAs are forked to a new process by the proxy worker, which\nshares the same process group. This allows the undesired effect that the\nThe proxy worker will receive signals such as SIGINT and will be closed\nby a Ctrl-C event before the pipeline handlers have been able to fully\nclean up.\n\nPrevent this signal from being delivered to the proxy worker by moving\nthe process to a new process group, matching the pid of the isolated proxy.\n\n\"\"\"\n\n> has been shutdown by SIGINT/SIGTERM preemptively, before\n> the pipeline handler got a chance to cleanup nicely.\n> \n> Bug: https://bugs.libcamera.org/show_bug.cgi?id=60\n> Signed-off-by: Umang Jain \n\nAh, excellent:\n\nTested-by: Kieran Bingham \n\n\n> ---\n> .../module_ipa_proxy_worker.cpp.tmpl | 10 ++++++++++\n> 1 file changed, 10 insertions(+)\n> \n> 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\n> index d993e39e..32ac3869 100644\n> --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl\n> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl\n> @@ -207,6 +207,16 @@ int main(int argc, char **argv)\n> \t\treturn EXIT_FAILURE;\n> \t}\n> \n> +\t/*\n> +\t * Shutdown of proxy worker can be preemptive by events like\n\ns/preemptive/pre-empted/ ?\n\n> +\t * SIGINT/SIGTERM, even before the pipeline handler can request\n> +\t * shutdown. Hence, assign a new gid to handle these situations.\n\n\"assign a new gid to prevent signals on the application being delivered\nto the proxy.\" ... ?\n\n\nOtherwise,\n\nReviewed-by: Kieran Bingham \n\n\n> +\t */\n> +\tif (setpgid(0, 0) < -1) {\n> +\t\tLOG({{proxy_worker_name}}, Warning)\n> +\t\t\t<< \"Failed to set new gid: \" << strerror(errno);\n> +\t}\n> +\n> \t{{proxy_worker_name}} proxyWorker;\n> \tint ret = proxyWorker.init(ipam, fd);\n> \tif (ret < 0) {\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 27A97C0109\n\tfor ;\n\tMon, 26 Jul 2021 16:01:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6A577687BE;\n\tMon, 26 Jul 2021 18:01:19 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6B070687AF\n\tfor ;\n\tMon, 26 Jul 2021 18:01:18 +0200 (CEST)","from [192.168.36.5] (unknown [80.2.20.111])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A9958332;\n\tMon, 26 Jul 2021 18:01:17 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"vafFwKL+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627315278;\n\tbh=nZaDvCwhqB6rpybwqSsBieE6IT3G51S16I8cDCFA/Ps=;\n\th=To:References:From:Subject:Date:In-Reply-To:From;\n\tb=vafFwKL+FR2Bv3BXbUBc8iMZB20/XUoySa/vX8V6wBA40SIF2vyDX/tvqwtNrbE9/\n\tCfgm0osyMCAvyCDEDPLOd/WMwLJTytEnR1oV9bt/l3PgM2UtIVsLVuVbiBetfup+ib\n\tpLvPfv44xYbWJjBr4hnQFa/Yuz2JW2e/iJcPet4Y=","To":"Umang Jain ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210726103126.43238-1-umang.jain@ideasonboard.com>","From":"Kieran Bingham ","Message-ID":"<824b4696-0c3f-e70d-d6c8-f1e53452d3e4@ideasonboard.com>","Date":"Mon, 26 Jul 2021 17:01:14 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"<20210726103126.43238-1-umang.jain@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH] utils: ipc: Assign a new gid to proxy\n\tworker","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18364,"web_url":"https://patchwork.libcamera.org/comment/18364/","msgid":"","date":"2021-07-26T16:21:43","subject":"Re: [libcamera-devel] [PATCH] utils: ipc: Assign a new gid to proxy\n\tworker","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Mon, Jul 26, 2021 at 05:01:14PM +0100, Kieran Bingham wrote:\n> On 26/07/2021 11:31, Umang Jain wrote:\n> > Doing so, handles the situation where the proxy worker\n> \n> We wouldn't normally open the commit message with a statement which\n> follows on from the subject. They're somewhat distinct.\n> \n> The subject/title should be a just that, which in this case is fine, but\n> then the commit message should be something along the lines of:\n> \n> - Introduce the issue\n> - Describe what the patch does to solve it.\n> \n> Imagine reading the commit message without the title:\n> \n> \"Doing so,\" ... leaves me thinking ... doing what?\n> \n> So we should write this as something like:\n> \n> \"\"\"\n> Isolated IPAs are forked to a new process by the proxy worker, which\n> shares the same process group. This allows the undesired effect that the\n> The proxy worker will receive signals such as SIGINT and will be closed\n> by a Ctrl-C event before the pipeline handlers have been able to fully\n> clean up.\n> \n> Prevent this signal from being delivered to the proxy worker by moving\n> the process to a new process group, matching the pid of the isolated proxy.\n> \n> \"\"\"\n> \n> > has been shutdown by SIGINT/SIGTERM preemptively, before\n> > the pipeline handler got a chance to cleanup nicely.\n> > \n> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=60\n> > Signed-off-by: Umang Jain \n> \n> Ah, excellent:\n> \n> Tested-by: Kieran Bingham \n> \n> > ---\n> > .../module_ipa_proxy_worker.cpp.tmpl | 10 ++++++++++\n> > 1 file changed, 10 insertions(+)\n> > \n> > 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\n> > index d993e39e..32ac3869 100644\n> > --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl\n> > +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl\n> > @@ -207,6 +207,16 @@ int main(int argc, char **argv)\n> > \t\treturn EXIT_FAILURE;\n> > \t}\n> > \n> > +\t/*\n> > +\t * Shutdown of proxy worker can be preemptive by events like\n> \n> s/preemptive/pre-empted/ ?\n\nOr \"preempt\" ? Interestingly, there's also \"preëmpt\", but it's rare :-)\n\n> > +\t * SIGINT/SIGTERM, even before the pipeline handler can request\n> > +\t * shutdown. Hence, assign a new gid to handle these situations.\n> \n> \"assign a new gid to prevent signals on the application being delivered\n> to the proxy.\" ... ?\n> \n> Otherwise,\n> \n> Reviewed-by: Kieran Bingham \n>\n> > +\t */\n> > +\tif (setpgid(0, 0) < -1) {\n> > +\t\tLOG({{proxy_worker_name}}, Warning)\n> > +\t\t\t<< \"Failed to set new gid: \" << strerror(errno);\n\nThe LOG() call may end up changing errno. You should write\n\n\t\tint err = errno;\n\t\tLOG({{proxy_worker_name}}, Warning)\n\t\t\t<< \"Failed to set new gid: \" << strerror(err);\n\nWith this fixed, as well ass the commit message and comments,\n\nReviewed-by: Laurent Pinchart \n\n> > +\t}\n> > +\n> > \t{{proxy_worker_name}} proxyWorker;\n> > \tint ret = proxyWorker.init(ipam, fd);\n> > \tif (ret < 0) {\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 16B12C322C\n\tfor ;\n\tMon, 26 Jul 2021 16:21:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8222B687BE;\n\tMon, 26 Jul 2021 18:21:50 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8B1E0687AF\n\tfor ;\n\tMon, 26 Jul 2021 18:21:48 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 01A41332;\n\tMon, 26 Jul 2021 18:21:47 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ng2lEE/t\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627316508;\n\tbh=39I/ApHgSBXysyqiO2JR1WHeKQN7mgSSRn3sXtycsOM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ng2lEE/t1VMkL6gbjGeRuqxjPgHeLyGKQAeqC2fL83KnRgNaDMwysIen6FUMlVwAi\n\tP4n26K8IOpAQC7ECNxvqu27iUhsLQpm/Fwu5XseZiAyHZYOC6W2D6x1EKIjKvG8zo+\n\tYi6LAHm1mAfgTplImXipMtmMoNynmbox9C+/7v3c=","Date":"Mon, 26 Jul 2021 19:21:43 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210726103126.43238-1-umang.jain@ideasonboard.com>\n\t<824b4696-0c3f-e70d-d6c8-f1e53452d3e4@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<824b4696-0c3f-e70d-d6c8-f1e53452d3e4@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH] utils: ipc: Assign a new gid to proxy\n\tworker","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18373,"web_url":"https://patchwork.libcamera.org/comment/18373/","msgid":"<20210727042643.GA2167@pyrite.rasen.tech>","date":"2021-07-27T04:26:43","subject":"Re: [libcamera-devel] [PATCH] utils: ipc: Assign a new gid to proxy\n\tworker","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi,\n\nOn Mon, Jul 26, 2021 at 07:21:43PM +0300, Laurent Pinchart wrote:\n> Hello,\n> \n> On Mon, Jul 26, 2021 at 05:01:14PM +0100, Kieran Bingham wrote:\n> > On 26/07/2021 11:31, Umang Jain wrote:\n> > > Doing so, handles the situation where the proxy worker\n> > \n> > We wouldn't normally open the commit message with a statement which\n> > follows on from the subject. They're somewhat distinct.\n> > \n> > The subject/title should be a just that, which in this case is fine, but\n> > then the commit message should be something along the lines of:\n> > \n> > - Introduce the issue\n> > - Describe what the patch does to solve it.\n> > \n> > Imagine reading the commit message without the title:\n> > \n> > \"Doing so,\" ... leaves me thinking ... doing what?\n> > \n> > So we should write this as something like:\n> > \n> > \"\"\"\n> > Isolated IPAs are forked to a new process by the proxy worker, which\n> > shares the same process group. This allows the undesired effect that the\n> > The proxy worker will receive signals such as SIGINT and will be closed\n> > by a Ctrl-C event before the pipeline handlers have been able to fully\n> > clean up.\n> > \n> > Prevent this signal from being delivered to the proxy worker by moving\n> > the process to a new process group, matching the pid of the isolated proxy.\n> > \n> > \"\"\"\n> > \n> > > has been shutdown by SIGINT/SIGTERM preemptively, before\n> > > the pipeline handler got a chance to cleanup nicely.\n> > > \n> > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=60\n> > > Signed-off-by: Umang Jain \n\n\\o/\n\n> > \n> > Ah, excellent:\n> > \n> > Tested-by: Kieran Bingham \n> > \n> > > ---\n> > > .../module_ipa_proxy_worker.cpp.tmpl | 10 ++++++++++\n> > > 1 file changed, 10 insertions(+)\n> > > \n> > > 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\n> > > index d993e39e..32ac3869 100644\n> > > --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl\n> > > +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl\n> > > @@ -207,6 +207,16 @@ int main(int argc, char **argv)\n> > > \t\treturn EXIT_FAILURE;\n> > > \t}\n> > > \n> > > +\t/*\n> > > +\t * Shutdown of proxy worker can be preemptive by events like\n> > \n> > s/preemptive/pre-empted/ ?\n> \n> Or \"preempt\" ? Interestingly, there's also \"preëmpt\", but it's rare :-)\n\n\"pre-empted\" I think is the right choice here, since it's \" can\nbe pre-empted by \".\n\n> \n> > > +\t * SIGINT/SIGTERM, even before the pipeline handler can request\n> > > +\t * shutdown. Hence, assign a new gid to handle these situations.\n> > \n> > \"assign a new gid to prevent signals on the application being delivered\n> > to the proxy.\" ... ?\n> > \n> > Otherwise,\n> > \n> > Reviewed-by: Kieran Bingham \n> >\n> > > +\t */\n> > > +\tif (setpgid(0, 0) < -1) {\n> > > +\t\tLOG({{proxy_worker_name}}, Warning)\n> > > +\t\t\t<< \"Failed to set new gid: \" << strerror(errno);\n> \n> The LOG() call may end up changing errno. You should write\n> \n> \t\tint err = errno;\n> \t\tLOG({{proxy_worker_name}}, Warning)\n> \t\t\t<< \"Failed to set new gid: \" << strerror(err);\n> \n> With this fixed, as well ass the commit message and comments,\n> \n> Reviewed-by: Laurent Pinchart \n\nLaurent and Kieran have covered all my comments :p\n\nWith those fixed,\n\nReviewed-by: Paul Elder \n\n> \n> > > +\t}\n> > > +\n> > > \t{{proxy_worker_name}} proxyWorker;\n> > > \tint ret = proxyWorker.init(ipam, fd);\n> > > \tif (ret < 0) {\n> > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id DFA0FC322C\n\tfor ;\n\tTue, 27 Jul 2021 04:26:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 465DA687C3;\n\tTue, 27 Jul 2021 06:26:54 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A253B60506\n\tfor ;\n\tTue, 27 Jul 2021 06:26:53 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BD13DEE;\n\tTue, 27 Jul 2021 06:26:51 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ABzELh26\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627360013;\n\tbh=KjDvKPBJqWfKe0G5y9fiFsBcJix3hL/Y+ZQH6GGkewM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ABzELh26TuIIVUjHqp16L/yL3jm7//x7trYh/7R3ddU9tyXjEaV1m/U6sH8gXxMrf\n\tG5oQqx9KKh8huYPBAc9QbXOLJ2l5Idkd7aflfhVlPwi5lYDoI/VsrIoWdwcSJ+emV0\n\tO4/URf1YWjF+T9I0mryMtVht/2dFVg+HukuiK6G4=","Date":"Tue, 27 Jul 2021 13:26:43 +0900","From":"paul.elder@ideasonboard.com","To":"Laurent Pinchart ","Message-ID":"<20210727042643.GA2167@pyrite.rasen.tech>","References":"<20210726103126.43238-1-umang.jain@ideasonboard.com>\n\t<824b4696-0c3f-e70d-d6c8-f1e53452d3e4@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] utils: ipc: Assign a new gid to proxy\n\tworker","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18374,"web_url":"https://patchwork.libcamera.org/comment/18374/","msgid":"<2a9969c5-8c21-14d3-ec62-c693d1968bea@ideasonboard.com>","date":"2021-07-27T04:32:16","subject":"Re: [libcamera-devel] [PATCH] utils: ipc: Assign a new gid to proxy\n\tworker","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Paul,\n\nThanks for the review.\n\nOn 7/27/21 9:56 AM, paul.elder@ideasonboard.com wrote:\n> Hi,\n>\n> On Mon, Jul 26, 2021 at 07:21:43PM +0300, Laurent Pinchart wrote:\n>> Hello,\n>>\n>> On Mon, Jul 26, 2021 at 05:01:14PM +0100, Kieran Bingham wrote:\n>>> On 26/07/2021 11:31, Umang Jain wrote:\n>>>> Doing so, handles the situation where the proxy worker\n>>> We wouldn't normally open the commit message with a statement which\n>>> follows on from the subject. They're somewhat distinct.\n>>>\n>>> The subject/title should be a just that, which in this case is fine, but\n>>> then the commit message should be something along the lines of:\n>>>\n>>> - Introduce the issue\n>>> - Describe what the patch does to solve it.\n>>>\n>>> Imagine reading the commit message without the title:\n>>>\n>>> \"Doing so,\" ... leaves me thinking ... doing what?\n>>>\n>>> So we should write this as something like:\n>>>\n>>> \"\"\"\n>>> Isolated IPAs are forked to a new process by the proxy worker, which\n>>> shares the same process group. This allows the undesired effect that the\n>>> The proxy worker will receive signals such as SIGINT and will be closed\n>>> by a Ctrl-C event before the pipeline handlers have been able to fully\n>>> clean up.\n>>>\n>>> Prevent this signal from being delivered to the proxy worker by moving\n>>> the process to a new process group, matching the pid of the isolated proxy.\n>>>\n>>> \"\"\"\n>>>\n>>>> has been shutdown by SIGINT/SIGTERM preemptively, before\n>>>> the pipeline handler got a chance to cleanup nicely.\n>>>>\n>>>> Bug: https://bugs.libcamera.org/show_bug.cgi?id=60\n>>>> Signed-off-by: Umang Jain \n> \\o/\n>\n>>> Ah, excellent:\n>>>\n>>> Tested-by: Kieran Bingham \n>>>\n>>>> ---\n>>>> .../module_ipa_proxy_worker.cpp.tmpl | 10 ++++++++++\n>>>> 1 file changed, 10 insertions(+)\n>>>>\n>>>> 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\n>>>> index d993e39e..32ac3869 100644\n>>>> --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl\n>>>> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl\n>>>> @@ -207,6 +207,16 @@ int main(int argc, char **argv)\n>>>> \t\treturn EXIT_FAILURE;\n>>>> \t}\n>>>> \n>>>> +\t/*\n>>>> +\t * Shutdown of proxy worker can be preemptive by events like\n>>> s/preemptive/pre-empted/ ?\n>> Or \"preempt\" ? Interestingly, there's also \"preëmpt\", but it's rare :-)\n> \"pre-empted\" I think is the right choice here, since it's \" can\n> be pre-empted by \".\n\nYes, \"pre-empted\" seems correct now (I just put the v2 on ML). Will \naddress this while pushing.\n\nThanks!\n\n>\n>>>> +\t * SIGINT/SIGTERM, even before the pipeline handler can request\n>>>> +\t * shutdown. Hence, assign a new gid to handle these situations.\n>>> \"assign a new gid to prevent signals on the application being delivered\n>>> to the proxy.\" ... ?\n>>>\n>>> Otherwise,\n>>>\n>>> Reviewed-by: Kieran Bingham \n>>>\n>>>> +\t */\n>>>> +\tif (setpgid(0, 0) < -1) {\n>>>> +\t\tLOG({{proxy_worker_name}}, Warning)\n>>>> +\t\t\t<< \"Failed to set new gid: \" << strerror(errno);\n>> The LOG() call may end up changing errno. You should write\n>>\n>> \t\tint err = errno;\n>> \t\tLOG({{proxy_worker_name}}, Warning)\n>> \t\t\t<< \"Failed to set new gid: \" << strerror(err);\n>>\n>> With this fixed, as well ass the commit message and comments,\n>>\n>> Reviewed-by: Laurent Pinchart \n> Laurent and Kieran have covered all my comments :p\n>\n> With those fixed,\n>\n> Reviewed-by: Paul Elder \n>\n>>>> +\t}\n>>>> +\n>>>> \t{{proxy_worker_name}} proxyWorker;\n>>>> \tint ret = proxyWorker.init(ipam, fd);\n>>>> \tif (ret < 0) {\n>>>>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 6146CC0109\n\tfor ;\n\tTue, 27 Jul 2021 04:32:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A85E9687B7;\n\tTue, 27 Jul 2021 06:32:23 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 93CDF60506\n\tfor ;\n\tTue, 27 Jul 2021 06:32:21 +0200 (CEST)","from [192.168.0.107] (unknown [103.251.226.156])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 55F38EE;\n\tTue, 27 Jul 2021 06:32:20 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"i0fCd6gw\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627360341;\n\tbh=xfLNg+dpKj+zu1I3mwHKTGDMu3PdxjTi3GJW4yjnsxY=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=i0fCd6gw0iU3KelxkMoqkJUnZqHzFmbhCzdTF52jHh57AyjdnAncUmUT4lhFKQ3Wv\n\tGLUNs0/1gBX6hkcc4KMm0WJ3L2Az0e9zC0PZO8AM2WWAKH/haPEWiGtALNEbnRkvHt\n\tRhfaOqaDLOl2IHGyfkYQL/aXHNxODi5rGtbtw5wk=","To":"paul.elder@ideasonboard.com,\n\tLaurent Pinchart ","References":"<20210726103126.43238-1-umang.jain@ideasonboard.com>\n\t<824b4696-0c3f-e70d-d6c8-f1e53452d3e4@ideasonboard.com>\n\t\n\t<20210727042643.GA2167@pyrite.rasen.tech>","From":"Umang Jain ","Message-ID":"<2a9969c5-8c21-14d3-ec62-c693d1968bea@ideasonboard.com>","Date":"Tue, 27 Jul 2021 10:02:16 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<20210727042643.GA2167@pyrite.rasen.tech>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH] utils: ipc: Assign a new gid to proxy\n\tworker","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18492,"web_url":"https://patchwork.libcamera.org/comment/18492/","msgid":"","date":"2021-08-02T16:55:20","subject":"Re: [libcamera-devel] [PATCH] utils: ipc: Assign a new gid to proxy\n\tworker","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Tue, Jul 27, 2021 at 10:02:16AM +0530, Umang Jain wrote:\n> On 7/27/21 9:56 AM, paul.elder@ideasonboard.com wrote:\n> > On Mon, Jul 26, 2021 at 07:21:43PM +0300, Laurent Pinchart wrote:\n> >> On Mon, Jul 26, 2021 at 05:01:14PM +0100, Kieran Bingham wrote:\n> >>> On 26/07/2021 11:31, Umang Jain wrote:\n> >>>> Doing so, handles the situation where the proxy worker\n> >>> We wouldn't normally open the commit message with a statement which\n> >>> follows on from the subject. They're somewhat distinct.\n> >>>\n> >>> The subject/title should be a just that, which in this case is fine, but\n> >>> then the commit message should be something along the lines of:\n> >>>\n> >>> - Introduce the issue\n> >>> - Describe what the patch does to solve it.\n> >>>\n> >>> Imagine reading the commit message without the title:\n> >>>\n> >>> \"Doing so,\" ... leaves me thinking ... doing what?\n> >>>\n> >>> So we should write this as something like:\n> >>>\n> >>> \"\"\"\n> >>> Isolated IPAs are forked to a new process by the proxy worker, which\n> >>> shares the same process group. This allows the undesired effect that the\n> >>> The proxy worker will receive signals such as SIGINT and will be closed\n> >>> by a Ctrl-C event before the pipeline handlers have been able to fully\n> >>> clean up.\n> >>>\n> >>> Prevent this signal from being delivered to the proxy worker by moving\n> >>> the process to a new process group, matching the pid of the isolated proxy.\n> >>>\n> >>> \"\"\"\n> >>>\n> >>>> has been shutdown by SIGINT/SIGTERM preemptively, before\n> >>>> the pipeline handler got a chance to cleanup nicely.\n> >>>>\n> >>>> Bug: https://bugs.libcamera.org/show_bug.cgi?id=60\n> >>>> Signed-off-by: Umang Jain \n> > \\o/\n> >\n> >>> Ah, excellent:\n> >>>\n> >>> Tested-by: Kieran Bingham \n> >>>\n> >>>> ---\n> >>>> .../module_ipa_proxy_worker.cpp.tmpl | 10 ++++++++++\n> >>>> 1 file changed, 10 insertions(+)\n> >>>>\n> >>>> 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\n> >>>> index d993e39e..32ac3869 100644\n> >>>> --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl\n> >>>> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl\n> >>>> @@ -207,6 +207,16 @@ int main(int argc, char **argv)\n> >>>> \t\treturn EXIT_FAILURE;\n> >>>> \t}\n> >>>> \n> >>>> +\t/*\n> >>>> +\t * Shutdown of proxy worker can be preemptive by events like\n> >>> s/preemptive/pre-empted/ ?\n> >> Or \"preempt\" ? Interestingly, there's also \"preëmpt\", but it's rare :-)\n> > \"pre-empted\" I think is the right choice here, since it's \" can\n> > be pre-empted by \".\n> \n> Yes, \"pre-empted\" seems correct now (I just put the v2 on ML). Will \n> address this while pushing.\n\nSorry, I meant \"preempted\", not \"preempt\". Doesn't matter much :-)\n\n> >>>> +\t * SIGINT/SIGTERM, even before the pipeline handler can request\n> >>>> +\t * shutdown. Hence, assign a new gid to handle these situations.\n> >>> \"assign a new gid to prevent signals on the application being delivered\n> >>> to the proxy.\" ... ?\n> >>>\n> >>> Otherwise,\n> >>>\n> >>> Reviewed-by: Kieran Bingham \n> >>>\n> >>>> +\t */\n> >>>> +\tif (setpgid(0, 0) < -1) {\n> >>>> +\t\tLOG({{proxy_worker_name}}, Warning)\n> >>>> +\t\t\t<< \"Failed to set new gid: \" << strerror(errno);\n> >> The LOG() call may end up changing errno. You should write\n> >>\n> >> \t\tint err = errno;\n> >> \t\tLOG({{proxy_worker_name}}, Warning)\n> >> \t\t\t<< \"Failed to set new gid: \" << strerror(err);\n> >>\n> >> With this fixed, as well ass the commit message and comments,\n> >>\n> >> Reviewed-by: Laurent Pinchart \n> > Laurent and Kieran have covered all my comments :p\n> >\n> > With those fixed,\n> >\n> > Reviewed-by: Paul Elder \n> >\n> >>>> +\t}\n> >>>> +\n> >>>> \t{{proxy_worker_name}} proxyWorker;\n> >>>> \tint ret = proxyWorker.init(ipam, fd);\n> >>>> \tif (ret < 0) {","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id BF8BFC3235\n\tfor ;\n\tMon, 2 Aug 2021 16:55:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7A337687D0;\n\tMon, 2 Aug 2021 18:55:33 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2DC056026F\n\tfor ;\n\tMon, 2 Aug 2021 18:55:32 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id ABF7C9FB;\n\tMon, 2 Aug 2021 18:55:31 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"dtSZGpwP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627923331;\n\tbh=i/0Yc9w/oB6H1YzN5ibK+EG62CI10rL2aZhoEPZ1vgI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=dtSZGpwP60KWKWREc5YrIsHzxr/2fLnYXaHOk+IzcjahULl5KsdoHLiv2TYCaOSXg\n\tjo2m1dVDJxkhiWcwupj6pTOTf1kkhuvpRaZ6UwrnpcE3SdzwqM8/PU0AE5lONPL77Q\n\t8fXCmvb57y6ztoTY9/Bvma1oIWM+sotUBzENYR3w=","Date":"Mon, 2 Aug 2021 19:55:20 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20210726103126.43238-1-umang.jain@ideasonboard.com>\n\t<824b4696-0c3f-e70d-d6c8-f1e53452d3e4@ideasonboard.com>\n\t\n\t<20210727042643.GA2167@pyrite.rasen.tech>\n\t<2a9969c5-8c21-14d3-ec62-c693d1968bea@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<2a9969c5-8c21-14d3-ec62-c693d1968bea@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH] utils: ipc: Assign a new gid to proxy\n\tworker","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]