| Message ID | 20210726103126.43238-1-umang.jain@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@ideasonboard.com> Ah, excellent: Tested-by: Kieran Bingham <kieran.bingham@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@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) { >
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@ideasonboard.com> > > Ah, excellent: > > Tested-by: Kieran Bingham <kieran.bingham@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@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@ideasonboard.com> > > + } > > + > > {{proxy_worker_name}} proxyWorker; > > int ret = proxyWorker.init(ipam, fd); > > if (ret < 0) { > >
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@ideasonboard.com> \o/ > > > > Ah, excellent: > > > > Tested-by: Kieran Bingham <kieran.bingham@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@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@ideasonboard.com> Laurent and Kieran have covered all my comments :p With those fixed, Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > > > > + } > > > + > > > {{proxy_worker_name}} proxyWorker; > > > int ret = proxyWorker.init(ipam, fd); > > > if (ret < 0) { > > >
Hi Paul, Thanks for the review. On 7/27/21 9:56 AM, paul.elder@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@ideasonboard.com> > \o/ > >>> Ah, excellent: >>> >>> Tested-by: Kieran Bingham <kieran.bingham@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@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@ideasonboard.com> > Laurent and Kieran have covered all my comments :p > > With those fixed, > > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > >>>> + } >>>> + >>>> {{proxy_worker_name}} proxyWorker; >>>> int ret = proxyWorker.init(ipam, fd); >>>> if (ret < 0) { >>>>
On Tue, Jul 27, 2021 at 10:02:16AM +0530, Umang Jain wrote: > On 7/27/21 9:56 AM, paul.elder@ideasonboard.com wrote: > > On Mon, Jul 26, 2021 at 07:21:43PM +0300, Laurent Pinchart wrote: > >> 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@ideasonboard.com> > > \o/ > > > >>> Ah, excellent: > >>> > >>> Tested-by: Kieran Bingham <kieran.bingham@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. Sorry, I meant "preempted", not "preempt". Doesn't matter much :-) > >>>> + * 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@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@ideasonboard.com> > > Laurent and Kieran have covered all my comments :p > > > > With those fixed, > > > > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > > > >>>> + } > >>>> + > >>>> {{proxy_worker_name}} proxyWorker; > >>>> int ret = proxyWorker.init(ipam, fd); > >>>> if (ret < 0) {
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 + * SIGINT/SIGTERM, even before the pipeline handler can request + * shutdown. Hence, assign a new gid to handle these situations. + */ + 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) {
Doing so, handles the situation where the proxy worker 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@ideasonboard.com> --- .../module_ipa_proxy_worker.cpp.tmpl | 10 ++++++++++ 1 file changed, 10 insertions(+)