[{"id":5019,"web_url":"https://patchwork.libcamera.org/comment/5019/","msgid":"<1310083d-59e0-b5f8-10e4-a534f97049c1@ideasonboard.com>","date":"2020-06-04T09:53:16","subject":"Re: [libcamera-devel] [PATCH v2 1/2] test: ipc: unixsocket: Close\n\topen fds on error paths","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Umang,\n\nOn 15/04/2020 16:04, Umang Jain wrote:\n> Pointed out by Coverity DefectId=279052\n> \n> Signed-off-by: Umang Jain \n> ---\n> test/ipc/unixsocket.cpp | 19 ++++++++++++-------\n> 1 file changed, 12 insertions(+), 7 deletions(-)\n> \n> diff --git a/test/ipc/unixsocket.cpp b/test/ipc/unixsocket.cpp\n> index 5348f35..7bc6a89 100644\n> --- a/test/ipc/unixsocket.cpp\n> +++ b/test/ipc/unixsocket.cpp\n> @@ -16,6 +16,7 @@\n> #include \n> \n> #include \n> +#include \n> #include \n> \n> #include \"ipc_unixsocket.h\"\n> @@ -467,18 +468,22 @@ private:\n> \t\tif (fd < 0)\n> \t\t\treturn fd;\n> \n> +\t\tFileDescriptor file_desc = FileDescriptor(fd);\n> +\t\tclose(fd);\n\nSince you created this patch, the FileDescriptor has been updated to\ntake move semantics to avoid having to close an FD directly after\ninstantiating.\n\nI think updating this patch to use that new API needs to happen in this\npatch, could you update please?\n\nI'm not 100% sure of the syntax, but I think it's something like as\nsimple as:\n\nfile_desc = FileDescriptor(std::move(fd));\n\n\n> +\t\tif (file_desc.fd() < 0)\n> +\t\t\treturn file_desc.fd();\n> +\n> \t\tint size = 0;\n> \t\tfor (unsigned int i = 0; i < num; i++) {\n> -\t\t\tint clone = dup(fd);\n> -\t\t\tif (clone < 0)\n> -\t\t\t\treturn clone;\n> +\t\t\tFileDescriptor *clone = new FileDescriptor(file_desc.fd());\n> +\t\t\tint clone_int = clone->fd();\n\nI wouldn't use a local var here,, just reference clone->fd() below instead.\n\n> +\t\t\tif (clone_int < 0)\n> +\t\t\t\treturn clone_int;\n> \n> -\t\t\tsize += calculateLength(clone);\n> -\t\t\tmessage->fds.push_back(clone);\n> +\t\t\tsize += calculateLength(clone_int);\n> +\t\t\tmessage->fds.push_back(clone_int);\n> \t\t}\n> \n> -\t\tclose(fd);\n> -\n> \t\treturn size;\n> \t}\n> \n>","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7425361012\n\tfor ;\n\tThu, 4 Jun 2020 11:53:19 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BD70129B;\n\tThu, 4 Jun 2020 11:53:18 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Rxpnb55W\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1591264399;\n\tbh=/J7cpX7r9NASIgBSGNOLA5JASYnKOJwRyvhQ/fHsdnE=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=Rxpnb55Win93MtXSn7RKm1+zKhH4LrcKhf3idcr0KHi2Oz9OcMgcBXH2mo3eRpkzF\n\tG8G50Rv6dJErtah3FYIJsW4Ol1AynmitqbTD25KFFS3EkzzcspSW433vhLvlxRx0aF\n\tVYlV4ijbbM5WoGK/KRIHIkrE5mTCYKPXybsg0ZLU=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Umang Jain , libcamera-devel@lists.libcamera.org","References":"<20200415150409.27938-1-email@uajain.com>\n\t<20200415150409.27938-2-email@uajain.com>","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<1310083d-59e0-b5f8-10e4-a534f97049c1@ideasonboard.com>","Date":"Thu, 4 Jun 2020 10:53:16 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.7.0","MIME-Version":"1.0","In-Reply-To":"<20200415150409.27938-2-email@uajain.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v2 1/2] test: ipc: unixsocket: Close\n\topen fds on error paths","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","X-List-Received-Date":"Thu, 04 Jun 2020 09:53:19 -0000"}},{"id":5037,"web_url":"https://patchwork.libcamera.org/comment/5037/","msgid":"<20200605084228.GC5852@pendragon.ideasonboard.com>","date":"2020-06-05T08:42:28","subject":"Re: [libcamera-devel] [PATCH v2 1/2] test: ipc: unixsocket: Close\n\topen fds on error paths","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nOn Thu, Jun 04, 2020 at 10:53:16AM +0100, Kieran Bingham wrote:\n> On 15/04/2020 16:04, Umang Jain wrote:\n> > Pointed out by Coverity DefectId=279052\n> > \n> > Signed-off-by: Umang Jain \n> > ---\n> > test/ipc/unixsocket.cpp | 19 ++++++++++++-------\n> > 1 file changed, 12 insertions(+), 7 deletions(-)\n> > \n> > diff --git a/test/ipc/unixsocket.cpp b/test/ipc/unixsocket.cpp\n> > index 5348f35..7bc6a89 100644\n> > --- a/test/ipc/unixsocket.cpp\n> > +++ b/test/ipc/unixsocket.cpp\n> > @@ -16,6 +16,7 @@\n> > #include \n> > \n> > #include \n> > +#include \n> > #include \n> > \n> > #include \"ipc_unixsocket.h\"\n> > @@ -467,18 +468,22 @@ private:\n> > \t\tif (fd < 0)\n> > \t\t\treturn fd;\n> > \n> > +\t\tFileDescriptor file_desc = FileDescriptor(fd);\n> > +\t\tclose(fd);\n> \n> Since you created this patch, the FileDescriptor has been updated to\n> take move semantics to avoid having to close an FD directly after\n> instantiating.\n> \n> I think updating this patch to use that new API needs to happen in this\n> patch, could you update please?\n> \n> I'm not 100% sure of the syntax, but I think it's something like as\n> simple as:\n> \n> file_desc = FileDescriptor(std::move(fd));\n\nThis creates temporary file descriptor object and then uses copy\nassignment. The compiler will optimize it out, but it's best to write\n\n\t\tFileDescriptor file_desc{ std::move(fd) };\n\n> > +\t\tif (file_desc.fd() < 0)\n> > +\t\t\treturn file_desc.fd();\n> > +\n> > \t\tint size = 0;\n> > \t\tfor (unsigned int i = 0; i < num; i++) {\n> > -\t\t\tint clone = dup(fd);\n> > -\t\t\tif (clone < 0)\n> > -\t\t\t\treturn clone;\n> > +\t\t\tFileDescriptor *clone = new FileDescriptor(file_desc.fd());\n> > +\t\t\tint clone_int = clone->fd();\n> \n> I wouldn't use a local var here,, just reference clone->fd() below instead.\n\nI wouldn't use FileDescriptor here actually. The reason why new is used\nabove is (I assume) to avoid closing the clone when the FileDescriptor\ninstance is destroyed, but that ends up leaking that new FileDescriptor\ninstance.\n\nFileDescriptor is fine for file_desc (which should be named fileDesc as\nwe use camelCase), but here you can just keep the int clone variable.\n\n> > +\t\t\tif (clone_int < 0)\n> > +\t\t\t\treturn clone_int;\n> > \n> > -\t\t\tsize += calculateLength(clone);\n> > -\t\t\tmessage->fds.push_back(clone);\n> > +\t\t\tsize += calculateLength(clone_int);\n> > +\t\t\tmessage->fds.push_back(clone_int);\n> > \t\t}\n> > \n> > -\t\tclose(fd);\n> > -\n> > \t\treturn size;\n> > \t}\n> >","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 488AB61027\n\tfor ;\n\tFri, 5 Jun 2020 10:42:46 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C0A5227C;\n\tFri, 5 Jun 2020 10:42:45 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"mAEdnYVv\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1591346565;\n\tbh=o3JNGpmP+6o7WTOEkGMEySLqVxIv43H4TJ/rVQJcEL0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=mAEdnYVv6zCSrBKaS3e5emrQ0VecWjBCOrLt9TIDS7LpvwMPQgMVBshEAC0v1Or2x\n\tNKutj6C1gSkYYiX+PECqMpCU+HVrCXbXUOqAcwUtyui1GdoxOf92eIJOoVwL0+Fo/E\n\t7Gm6DcG94Zluala9UVnUnI5g6GSZh1PdBXLYYGWU=","Date":"Fri, 5 Jun 2020 11:42:28 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Umang Jain , libcamera-devel@lists.libcamera.org","Message-ID":"<20200605084228.GC5852@pendragon.ideasonboard.com>","References":"<20200415150409.27938-1-email@uajain.com>\n\t<20200415150409.27938-2-email@uajain.com>\n\t<1310083d-59e0-b5f8-10e4-a534f97049c1@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<1310083d-59e0-b5f8-10e4-a534f97049c1@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 1/2] test: ipc: unixsocket: Close\n\topen fds on error paths","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","X-List-Received-Date":"Fri, 05 Jun 2020 08:42:46 -0000"}}]