| Message ID | 20210817031619.11587-1-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Commit | d0d1733027c16aa3fe9b7427b4a00a126ebbc6ba |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, On Tue, Aug 17, 2021 at 06:16:19AM +0300, Laurent Pinchart wrote: > The IPCUnixSocket::send() function may fail, in which case it can be > useful for debugging to log an error message that tells which event was > affected. Do so. > > Reported-by: Coverity CID=354836 > Reported-by: Coverity CID=354837 > Reported-by: Coverity CID=354838 > Reported-by: Coverity CID=354839 > Reported-by: Coverity CID=354840 > Reported-by: Coverity CID=354841 > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Makes sense. Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > --- > This stems from a Coverity report that complains about the lock of error > checking for the return value of send(). As the send() function itself > logs a message on failure, I'm not entirely sure if this is needed, but > we do so when replying to synchronous methods, and there's no reason to > have a different behaviour for events. We should either add a message > here, or drop it from method replies. > > Kieran, is the Reported-by tag processed by any script ? Can I write > > Reported-by: Coverity CID=354836-354841 > > or something similar ? > > --- > .../libcamera_templates/module_ipa_proxy_worker.cpp.tmpl | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > 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 164a4dd64882..8a88bd467da7 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 > @@ -164,7 +164,10 @@ private: > > {{proxy_funcs.serialize_call(method|method_param_inputs, "_message.data()", "_message.fds()")}} > > - socket_.send(_message.payload()); > + int _ret = socket_.send(_message.payload()); > + if (_ret < 0) > + LOG({{proxy_worker_name}}, Error) > + << "Sending event {{method.mojom_name}}() failed: " << _ret; > > LOG({{proxy_worker_name}}, Debug) << "{{method.mojom_name}} done"; > } > -- > Regards, > > Laurent Pinchart >
Hi Laurent, On 8/17/21 8:46 AM, Laurent Pinchart wrote: > The IPCUnixSocket::send() function may fail, in which case it can be > useful for debugging to log an error message that tells which event was > affected. Do so. > > Reported-by: Coverity CID=354836 > Reported-by: Coverity CID=354837 > Reported-by: Coverity CID=354838 > Reported-by: Coverity CID=354839 > Reported-by: Coverity CID=354840 > Reported-by: Coverity CID=354841 > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > --- > This stems from a Coverity report that complains about the lock of error > checking for the return value of send(). As the send() function itself > logs a message on failure, I'm not entirely sure if this is needed, but > we do so when replying to synchronous methods, and there's no reason to > have a different behaviour for events. We should either add a message > here, or drop it from method replies. > > Kieran, is the Reported-by tag processed by any script ? Can I write > > Reported-by: Coverity CID=354836-354841 > > or something similar ? > > --- > .../libcamera_templates/module_ipa_proxy_worker.cpp.tmpl | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > 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 164a4dd64882..8a88bd467da7 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 > @@ -164,7 +164,10 @@ private: > > {{proxy_funcs.serialize_call(method|method_param_inputs, "_message.data()", "_message.fds()")}} > > - socket_.send(_message.payload()); > + int _ret = socket_.send(_message.payload()); > + if (_ret < 0) > + LOG({{proxy_worker_name}}, Error) > + << "Sending event {{method.mojom_name}}() failed: " << _ret; > > LOG({{proxy_worker_name}}, Debug) << "{{method.mojom_name}} done"; > }
On 17/08/2021 04:16, Laurent Pinchart wrote: > The IPCUnixSocket::send() function may fail, in which case it can be > useful for debugging to log an error message that tells which event was > affected. Do so. > > Reported-by: Coverity CID=354836 > Reported-by: Coverity CID=354837 > Reported-by: Coverity CID=354838 > Reported-by: Coverity CID=354839 > Reported-by: Coverity CID=354840 > Reported-by: Coverity CID=354841 > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > This stems from a Coverity report that complains about the lock of error > checking for the return value of send(). As the send() function itself > logs a message on failure, I'm not entirely sure if this is needed, but > we do so when replying to synchronous methods, and there's no reason to > have a different behaviour for events. We should either add a message > here, or drop it from method replies. I think coverity considers how an API is used, so indeed - here it likely noticed "Hey you check this in other places, should you check it here too"? I think adding the print here is ok ... as it will be clearer that the failure occurred from the specific proxy worker, so it adds value. The ::send() will report the strerror() as well, so overall this helps. > Kieran, is the Reported-by tag processed by any script ? Can I write There is no scripting no. It's just a reference. > Reported-by: Coverity CID=354836-354841 If you wish ;-) Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > or something similar ? > > --- > .../libcamera_templates/module_ipa_proxy_worker.cpp.tmpl | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > 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 164a4dd64882..8a88bd467da7 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 > @@ -164,7 +164,10 @@ private: > > {{proxy_funcs.serialize_call(method|method_param_inputs, "_message.data()", "_message.fds()")}} > > - socket_.send(_message.payload()); > + int _ret = socket_.send(_message.payload()); > + if (_ret < 0) > + LOG({{proxy_worker_name}}, Error) > + << "Sending event {{method.mojom_name}}() failed: " << _ret; > > LOG({{proxy_worker_name}}, Debug) << "{{method.mojom_name}} done"; > } >
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 164a4dd64882..8a88bd467da7 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 @@ -164,7 +164,10 @@ private: {{proxy_funcs.serialize_call(method|method_param_inputs, "_message.data()", "_message.fds()")}} - socket_.send(_message.payload()); + int _ret = socket_.send(_message.payload()); + if (_ret < 0) + LOG({{proxy_worker_name}}, Error) + << "Sending event {{method.mojom_name}}() failed: " << _ret; LOG({{proxy_worker_name}}, Debug) << "{{method.mojom_name}} done"; }
The IPCUnixSocket::send() function may fail, in which case it can be useful for debugging to log an error message that tells which event was affected. Do so. Reported-by: Coverity CID=354836 Reported-by: Coverity CID=354837 Reported-by: Coverity CID=354838 Reported-by: Coverity CID=354839 Reported-by: Coverity CID=354840 Reported-by: Coverity CID=354841 Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- This stems from a Coverity report that complains about the lock of error checking for the return value of send(). As the send() function itself logs a message on failure, I'm not entirely sure if this is needed, but we do so when replying to synchronous methods, and there's no reason to have a different behaviour for events. We should either add a message here, or drop it from method replies. Kieran, is the Reported-by tag processed by any script ? Can I write Reported-by: Coverity CID=354836-354841 or something similar ? --- .../libcamera_templates/module_ipa_proxy_worker.cpp.tmpl | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)