[{"id":18859,"web_url":"https://patchwork.libcamera.org/comment/18859/","msgid":"<20210817040248.GD1733965@pyrite.rasen.tech>","date":"2021-08-17T04:02:48","subject":"Re: [libcamera-devel] [PATCH] utils: ipc: ipa_proxy_worker: Log\n\tIPCUnixSocket::send() failures","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Laurent,\n\nOn Tue, Aug 17, 2021 at 06:16:19AM +0300, Laurent Pinchart wrote:\n> The IPCUnixSocket::send() function may fail, in which case it can be\n> useful for debugging to log an error message that tells which event was\n> affected. Do so.\n> \n> Reported-by: Coverity CID=354836\n> Reported-by: Coverity CID=354837\n> Reported-by: Coverity CID=354838\n> Reported-by: Coverity CID=354839\n> Reported-by: Coverity CID=354840\n> Reported-by: Coverity CID=354841\n> Signed-off-by: Laurent Pinchart \n\nMakes sense.\n\nReviewed-by: Paul Elder \n\n> ---\n> This stems from a Coverity report that complains about the lock of error\n> checking for the return value of send(). As the send() function itself\n> logs a message on failure, I'm not entirely sure if this is needed, but\n> we do so when replying to synchronous methods, and there's no reason to\n> have a different behaviour for events. We should either add a message\n> here, or drop it from method replies.\n> \n> Kieran, is the Reported-by tag processed by any script ? Can I write\n> \n> Reported-by: Coverity CID=354836-354841\n> \n> or something similar ?\n> \n> ---\n> .../libcamera_templates/module_ipa_proxy_worker.cpp.tmpl | 5 ++++-\n> 1 file changed, 4 insertions(+), 1 deletion(-)\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 164a4dd64882..8a88bd467da7 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> @@ -164,7 +164,10 @@ private:\n> \n> \t\t{{proxy_funcs.serialize_call(method|method_param_inputs, \"_message.data()\", \"_message.fds()\")}}\n> \n> -\t\tsocket_.send(_message.payload());\n> +\t\tint _ret = socket_.send(_message.payload());\n> +\t\tif (_ret < 0)\n> +\t\t\tLOG({{proxy_worker_name}}, Error)\n> +\t\t\t\t<< \"Sending event {{method.mojom_name}}() failed: \" << _ret;\n> \n> \t\tLOG({{proxy_worker_name}}, Debug) << \"{{method.mojom_name}} done\";\n> \t}\n> -- \n> Regards,\n> \n> Laurent Pinchart\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 7864EBD87D\n\tfor ;\n\tTue, 17 Aug 2021 04:02:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DDFE5688A2;\n\tTue, 17 Aug 2021 06:02:56 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 83E5A6025B\n\tfor ;\n\tTue, 17 Aug 2021 06:02:55 +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 EFF47F1;\n\tTue, 17 Aug 2021 06:02:53 +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=\"Ig7f0TDd\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629172975;\n\tbh=Fi04svZO+TwKWfYjPWbafEDQhLJM9bEOyvNMNppFGOk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Ig7f0TDdkZigY9UMVx+KF+PWwQLYKHkAwTxXHKmYZz3YLOc19lyMZVONSsd4n4FjO\n\t1vzuZk2qsi+q/X3KsSVlZE85llcZ3Ez81tQylELvh2y8kxenVQ3+SJmVxcCCOAA66S\n\t72LTrxFHR3w8IN5hMzJAaTrU7wnFQ73spDYIVq+8=","Date":"Tue, 17 Aug 2021 13:02:48 +0900","From":"paul.elder@ideasonboard.com","To":"Laurent Pinchart ","Message-ID":"<20210817040248.GD1733965@pyrite.rasen.tech>","References":"<20210817031619.11587-1-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20210817031619.11587-1-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH] utils: ipc: ipa_proxy_worker: Log\n\tIPCUnixSocket::send() failures","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":18860,"web_url":"https://patchwork.libcamera.org/comment/18860/","msgid":"","date":"2021-08-17T05:36:07","subject":"Re: [libcamera-devel] [PATCH] utils: ipc: ipa_proxy_worker: Log\n\tIPCUnixSocket::send() failures","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 8/17/21 8:46 AM, Laurent Pinchart wrote:\n> The IPCUnixSocket::send() function may fail, in which case it can be\n> useful for debugging to log an error message that tells which event was\n> affected. Do so.\n>\n> Reported-by: Coverity CID=354836\n> Reported-by: Coverity CID=354837\n> Reported-by: Coverity CID=354838\n> Reported-by: Coverity CID=354839\n> Reported-by: Coverity CID=354840\n> Reported-by: Coverity CID=354841\n> Signed-off-by: Laurent Pinchart \nReviewed-by: Umang Jain \n> ---\n> This stems from a Coverity report that complains about the lock of error\n> checking for the return value of send(). As the send() function itself\n> logs a message on failure, I'm not entirely sure if this is needed, but\n> we do so when replying to synchronous methods, and there's no reason to\n> have a different behaviour for events. We should either add a message\n> here, or drop it from method replies.\n>\n> Kieran, is the Reported-by tag processed by any script ? Can I write\n>\n> Reported-by: Coverity CID=354836-354841\n>\n> or something similar ?\n>\n> ---\n> .../libcamera_templates/module_ipa_proxy_worker.cpp.tmpl | 5 ++++-\n> 1 file changed, 4 insertions(+), 1 deletion(-)\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 164a4dd64882..8a88bd467da7 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> @@ -164,7 +164,10 @@ private:\n> \n> \t\t{{proxy_funcs.serialize_call(method|method_param_inputs, \"_message.data()\", \"_message.fds()\")}}\n> \n> -\t\tsocket_.send(_message.payload());\n> +\t\tint _ret = socket_.send(_message.payload());\n> +\t\tif (_ret < 0)\n> +\t\t\tLOG({{proxy_worker_name}}, Error)\n> +\t\t\t\t<< \"Sending event {{method.mojom_name}}() failed: \" << _ret;\n> \n> \t\tLOG({{proxy_worker_name}}, Debug) << \"{{method.mojom_name}} done\";\n> \t}","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 C7E4CBD87D\n\tfor ;\n\tTue, 17 Aug 2021 05:36:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2FD0968894;\n\tTue, 17 Aug 2021 07:36:13 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2E6B06025B\n\tfor ;\n\tTue, 17 Aug 2021 07:36:12 +0200 (CEST)","from [192.168.1.104] (unknown [103.238.109.15])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3E2C02C5;\n\tTue, 17 Aug 2021 07:36:11 +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=\"Z7Jd05u1\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629178571;\n\tbh=cPa3I2XgSKmHMmwKCVzH2bGUfnm5ZWRF6nAfGQho0DI=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=Z7Jd05u1giSNyFgRKop48VdZhfg0mCuMmHmw2DaiufdF7XyYCRhuGPAP5nEwKVsCP\n\tMdiH+q7GUe3Rr82g2vbZJUFDBWGKhejlg7XreiFQjqDe7p2o6yT0ieuf/NE9CeM6FG\n\tjXV1Ei6RmtKSQzJuUuXI0wpwEi+OFyQgyz6OoSaY=","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210817031619.11587-1-laurent.pinchart@ideasonboard.com>","From":"Umang Jain ","Message-ID":"","Date":"Tue, 17 Aug 2021 11:06:07 +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":"<20210817031619.11587-1-laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH] utils: ipc: ipa_proxy_worker: Log\n\tIPCUnixSocket::send() failures","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":18870,"web_url":"https://patchwork.libcamera.org/comment/18870/","msgid":"","date":"2021-08-17T12:54:08","subject":"Re: [libcamera-devel] [PATCH] utils: ipc: ipa_proxy_worker: Log\n\tIPCUnixSocket::send() failures","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On 17/08/2021 04:16, Laurent Pinchart wrote:\n> The IPCUnixSocket::send() function may fail, in which case it can be\n> useful for debugging to log an error message that tells which event was\n> affected. Do so.\n> \n> Reported-by: Coverity CID=354836\n> Reported-by: Coverity CID=354837\n> Reported-by: Coverity CID=354838\n> Reported-by: Coverity CID=354839\n> Reported-by: Coverity CID=354840\n> Reported-by: Coverity CID=354841\n> Signed-off-by: Laurent Pinchart \n> ---\n> This stems from a Coverity report that complains about the lock of error\n> checking for the return value of send(). As the send() function itself\n> logs a message on failure, I'm not entirely sure if this is needed, but\n> we do so when replying to synchronous methods, and there's no reason to\n> have a different behaviour for events. We should either add a message\n> here, or drop it from method replies.\n\nI think coverity considers how an API is used, so indeed - here it\nlikely noticed \"Hey you check this in other places, should you check it\nhere too\"?\n\nI think adding the print here is ok ... as it will be clearer that the\nfailure occurred from the specific proxy worker, so it adds value. The\n::send() will report the strerror() as well, so overall this helps.\n\n> Kieran, is the Reported-by tag processed by any script ? Can I write\n\nThere is no scripting no. It's just a reference.\n\n> Reported-by: Coverity CID=354836-354841\n\nIf you wish ;-)\n\nReviewed-by: Kieran Bingham \n\n> \n> or something similar ?\n> \n> ---\n> .../libcamera_templates/module_ipa_proxy_worker.cpp.tmpl | 5 ++++-\n> 1 file changed, 4 insertions(+), 1 deletion(-)\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 164a4dd64882..8a88bd467da7 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> @@ -164,7 +164,10 @@ private:\n> \n> \t\t{{proxy_funcs.serialize_call(method|method_param_inputs, \"_message.data()\", \"_message.fds()\")}}\n> \n> -\t\tsocket_.send(_message.payload());\n> +\t\tint _ret = socket_.send(_message.payload());\n> +\t\tif (_ret < 0)\n> +\t\t\tLOG({{proxy_worker_name}}, Error)\n> +\t\t\t\t<< \"Sending event {{method.mojom_name}}() failed: \" << _ret;\n> \n> \t\tLOG({{proxy_worker_name}}, Debug) << \"{{method.mojom_name}} done\";\n> \t}\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 30A5EBD87C\n\tfor ;\n\tTue, 17 Aug 2021 12:54:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D9A7F6888F;\n\tTue, 17 Aug 2021 14:54:12 +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 AF5306025C\n\tfor ;\n\tTue, 17 Aug 2021 14:54:10 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 560C42C5;\n\tTue, 17 Aug 2021 14:54:10 +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=\"ZKlZhS1L\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629204850;\n\tbh=goXwe1bV3f2pW5kfQDeXO2m6VcO3L1XQfLhN0RICKqI=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=ZKlZhS1L8QyETGsjlbEmUR8KDSto/pXCLogbJ5UJjDal+FOXTpd4VMesk34zcro3i\n\taTu0OUnb8UWM4TYJOrQCkpXVRdtbuWK5Swidl3eybNNhumBzFMotgyiW4dGwMOUaDE\n\tfk1KHsUJu56LKmfmpWcGCRs3ovhTiXkaKABjA5EU=","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210817031619.11587-1-laurent.pinchart@ideasonboard.com>","From":"Kieran Bingham ","Message-ID":"","Date":"Tue, 17 Aug 2021 13:54:08 +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":"<20210817031619.11587-1-laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH] utils: ipc: ipa_proxy_worker: Log\n\tIPCUnixSocket::send() failures","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\" "}}]