[{"id":1014,"web_url":"https://patchwork.libcamera.org/comment/1014/","msgid":"<20190305124733.GB4949@pendragon.ideasonboard.com>","date":"2019-03-05T12:47:33","subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: v4l2_device: Close\n\tPlane dmabuf fd","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nThank you for the patch.\n\nOn Mon, Mar 04, 2019 at 11:25:29PM +0000, Kieran Bingham wrote:\n> When constructing a Plane, the exported buffer provides a dmabuf handle which\n> is set to the Plane object.\n> \n> This action duplicates the handle for internal storage, and the original fd is\n> not used and needs to be closed.\n> \n> Close the handle, ensuring that the resources can be correctly managed.\n\nGood catch !\n\nThe fact that this problem went unnoticed makes me wonder if we have a\nproblem with the API. Would there be a way to make it more robust,\npreventing these errors from happening ? We could take ownership of the\nfd passed to the setDmabuf() function, requiring the caller to call\ndup() if needed, but that would create a different issue, equally bad in\nmy opinion. We could however change the setDmabuf() prototype in that case\n\nint Plane::setDmabuf(int &&fd, unsigned int length);\n\nwhich would require the caller to use std::move(). Pros, the caller\nwould be forced to think about it, cons, the caller may just add an\nstd::move() to fix the compilation error without actually thinking about\nit (the default move constructor for int doesn't modify the value of the\nother instance). I thus wonder if it's worth it, or if we'll have to\nlive with this. Or if there's another way I'm not thinking about right\nnow.\n\nIn any case,\n\nReviewed-by: Laurent Pinchart \n\n> Fixes: 771befc6dc0e (\"libcamera: v4l2_device: Request buffers from the device\")\n> Signed-off-by: Kieran Bingham \n> ---\n> src/libcamera/v4l2_device.cpp | 1 +\n> 1 file changed, 1 insertion(+)\n> \n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> index 0cd9f4b8e178..a88a5f5ff036 100644\n> --- a/src/libcamera/v4l2_device.cpp\n> +++ b/src/libcamera/v4l2_device.cpp\n> @@ -676,6 +676,7 @@ int V4L2Device::createPlane(Buffer *buffer, unsigned int planeIndex,\n> \tbuffer->planes().emplace_back();\n> \tPlane &plane = buffer->planes().back();\n> \tplane.setDmabuf(expbuf.fd, length);\n> +\t::close(expbuf.fd);\n> \n> \treturn 0;\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 71346600FC\n\tfor ;\n\tTue, 5 Mar 2019 13:47:39 +0100 (CET)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E86C824A;\n\tTue, 5 Mar 2019 13:47:38 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1551790059;\n\tbh=IkIOdmhz2LixRZvvnnNbY+oiDN3ubMy/GzNq+b6/FEw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=QQHVZ5LFwYcEX7mA3/czD3TfCwZe42QMX6d8u7HWeCyNt3lh1AlsGlGI++T+J76jd\n\tTsCHHzwWwPa51yNS+CvLl4ReTifgnSz0tWUtdBdJhc6frW/hXiOXyJp+/OI3BneaGH\n\to5vGTAChuaaWzT3B548/oQgwqHiTi94WFLPChteQ=","Date":"Tue, 5 Mar 2019 14:47:33 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"LibCamera Devel ","Message-ID":"<20190305124733.GB4949@pendragon.ideasonboard.com>","References":"<20190304232530.4427-1-kieran.bingham@ideasonboard.com>\n\t<20190304232530.4427-2-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190304232530.4427-2-kieran.bingham@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: v4l2_device: Close\n\tPlane dmabuf fd","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 05 Mar 2019 12:47:39 -0000"}},{"id":1016,"web_url":"https://patchwork.libcamera.org/comment/1016/","msgid":"<946923d6-bf47-30af-ce31-2ef598b2cd46@ideasonboard.com>","date":"2019-03-05T15:04:01","subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: v4l2_device: Close\n\tPlane dmabuf fd","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 05/03/2019 12:47, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> Thank you for the patch.\n> \n> On Mon, Mar 04, 2019 at 11:25:29PM +0000, Kieran Bingham wrote:\n>> When constructing a Plane, the exported buffer provides a dmabuf handle which\n>> is set to the Plane object.\n>>\n>> This action duplicates the handle for internal storage, and the original fd is\n>> not used and needs to be closed.\n>>\n>> Close the handle, ensuring that the resources can be correctly managed.\n> \n> Good catch !\n> \n> The fact that this problem went unnoticed makes me wonder if we have a\n> problem with the API.\n\nWell, it didn't exactly go unnoticed - It just hadn't been dug out.\n\nThis was part of the reason for the call to\nV4L2Device::requestBuffers(0) failing.\n\n\n> Would there be a way to make it more robust,\n> preventing these errors from happening ? We could take ownership of the\n> fd passed to the setDmabuf() function, requiring the caller to call\n> dup() if needed, but that would create a different issue, equally bad in\n> my opinion. We could however change the setDmabuf() prototype in that case\n\n\nI did think about that too, and considered removing the dup. Especially\nas it feels a bit weird to take the expbuf.fd and close it (almost)\nimmediately.\n\nBut, I think the duplication does keep the 'layering' and ownership in\nbetter form. And in this instance it's just a matter of making sure the\ninternal ownership is tracked.\n\nWhen we start using externally provided buffers - I believe the\nseparation of the internal FD will be beneficial.\n\n\n\n> int Plane::setDmabuf(int &&fd, unsigned int length);\n> \n> which would require the caller to use std::move(). Pros, the caller\n> would be forced to think about it, cons, the caller may just add an\n> std::move() to fix the compilation error without actually thinking about\n> it (the default move constructor for int doesn't modify the value of the\n> other instance). I thus wonder if it's worth it, or if we'll have to\n> live with this. Or if there's another way I'm not thinking about right\n> now.\n\nI think that might just add more complication without any real benefit\nright now. If it becomes an issue we can reconsider this when we add\nsupport for externally provided buffers later.\n\n> \n> In any case,\n> \n> Reviewed-by: Laurent Pinchart \n\nThanks, I'll merge this to master now as it fixes a bug.\n\n--\nKieran\n\n\n> \n>> Fixes: 771befc6dc0e (\"libcamera: v4l2_device: Request buffers from the device\")\n>> Signed-off-by: Kieran Bingham \n>> ---\n>> src/libcamera/v4l2_device.cpp | 1 +\n>> 1 file changed, 1 insertion(+)\n>>\n>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n>> index 0cd9f4b8e178..a88a5f5ff036 100644\n>> --- a/src/libcamera/v4l2_device.cpp\n>> +++ b/src/libcamera/v4l2_device.cpp\n>> @@ -676,6 +676,7 @@ int V4L2Device::createPlane(Buffer *buffer, unsigned int planeIndex,\n>> \tbuffer->planes().emplace_back();\n>> \tPlane &plane = buffer->planes().back();\n>> \tplane.setDmabuf(expbuf.fd, length);\n>> +\t::close(expbuf.fd);\n>> \n>> \treturn 0;\n>> }\n>","headers":{"Return-Path":"","Received":["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 97534600FC\n\tfor ;\n\tTue, 5 Mar 2019 16:04:04 +0100 (CET)","from [192.168.0.21]\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 EF9A024A;\n\tTue, 5 Mar 2019 16:04:03 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1551798244;\n\tbh=tOdJZ0JJu5msq9L0bCbmldi6LD13SshGjC9uTZsl+ew=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=A2lWb/aLzhSFhncRZq5o/zff0os9jTqpkBliRCduNWI5z21xiIlPYFhJ4siTiwxW0\n\tGcY2P9ihjyNSWimBEo2xnE3s2Cf14QgTV1mKsES8FYCFjNgGHQM1APO3dL2qJbKm9w\n\tTUlVT2u3zwxUQFRKkN+uRT4cJx5tUrrWvywSz8Xo=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Laurent Pinchart ","Cc":"LibCamera Devel ","References":"<20190304232530.4427-1-kieran.bingham@ideasonboard.com>\n\t<20190304232530.4427-2-kieran.bingham@ideasonboard.com>\n\t<20190305124733.GB4949@pendragon.ideasonboard.com>","From":"Kieran Bingham ","Openpgp":"preference=signencrypt","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\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAkAEEwEKACoCGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEFAlnDk/gFCQeA/YsACgkQoR5GchCkYf3X5w/9EaZ7\n\tcnUcT6dxjxrcmmMnfFPoQA1iQXr/MXQJBjFWfxRUWYzjvUJb2D/FpA8FY7y+vksoJP7pWDL7\n\tQTbksdwzagUEk7CU45iLWL/CZ/knYhj1I/+5LSLFmvZ/5Gf5xn2ZCsmg7C0MdW/GbJ8IjWA8\n\t/LKJSEYH8tefoiG6+9xSNp1p0Gesu3vhje/GdGX4wDsfAxx1rIYDYVoX4bDM+uBUQh7sQox/\n\tR1bS0AaVJzPNcjeC14MS226mQRUaUPc9250aj44WmDfcg44/kMsoLFEmQo2II9aOlxUDJ+x1\n\txohGbh9mgBoVawMO3RMBihcEjo/8ytW6v7xSF+xP4Oc+HOn7qebAkxhSWcRxQVaQYw3S9iZz\n\t2iA09AXAkbvPKuMSXi4uau5daXStfBnmOfalG0j+9Y6hOFjz5j0XzaoF6Pln0jisDtWltYhP\n\tX9LjFVhhLkTzPZB/xOeWGmsG4gv2V2ExbU3uAmb7t1VSD9+IO3Km4FtnYOKBWlxwEd8qOFpS\n\tjEqMXURKOiJvnw3OXe9MqG19XdeENA1KyhK5rqjpwdvPGfSn2V+SlsdJA0DFsobUScD9qXQw\n\tOvhapHe3XboK2+Rd7L+g/9Ud7ZKLQHAsMBXOVJbufA1AT+IaOt0ugMcFkAR5UbBg5+dZUYJj\n\t1QbPQcGmM3wfvuaWV5+SlJ+WeKIb8ta5Ag0EVgT9ZgEQAM4o5G/kmruIQJ3K9SYzmPishRHV\n\tDcUcvoakyXSX2mIoccmo9BHtD9MxIt+QmxOpYFNFM7YofX4lG0ld8H7FqoNVLd/+a0yru5Cx\n\tadeZBe3qr1eLns10Q90LuMo7/6zJhCW2w+HE7xgmCHejAwuNe3+7yt4QmwlSGUqdxl8cgtS1\n\tPlEK93xXDsgsJj/bw1EfSVdAUqhx8UQ3aVFxNug5OpoX9FdWJLKROUrfNeBE16RLrNrq2ROc\n\tiSFETpVjyC/oZtzRFnwD9Or7EFMi76/xrWzk+/b15RJ9WrpXGMrttHUUcYZEOoiC2lEXMSAF\n\tSSSj4vHbKDJ0vKQdEFtdgB1roqzxdIOg4rlHz5qwOTynueiBpaZI3PHDudZSMR5Fk6QjFooE\n\tXTw3sSl/km/lvUFiv9CYyHOLdygWohvDuMkV/Jpdkfq8XwFSjOle+vT/4VqERnYFDIGBxaRx\n\tkoBLfNDiiuR3lD8tnJ4A1F88K6ojOUs+jndKsOaQpDZV6iNFv8IaNIklTPvPkZsmNDhJMRHH\n\tIu60S7BpzNeQeT4yyY4dX9lC2JL/LOEpw8DGf5BNOP1KgjCvyp1/KcFxDAo89IeqljaRsCdP\n\t7WCIECWYem6pLwaw6IAL7oX+tEqIMPph/G/jwZcdS6Hkyt/esHPuHNwX4guqTbVEuRqbDzDI\n\t2DJO5FbxABEBAAGJAiUEGAEKAA8CGwwFAlnDlGsFCQeA/gIACgkQoR5GchCkYf1yYRAAq+Yo\n\tnbf9DGdK1kTAm2RTFg+w9oOp2Xjqfhds2PAhFFvrHQg1XfQR/UF/SjeUmaOmLSczM0s6XMeO\n\tVcE77UFtJ/+hLo4PRFKm5X1Pcar6g5m4xGqa+Xfzi9tRkwC29KMCoQOag1BhHChgqYaUH3yo\n\tUzaPwT/fY75iVI+yD0ih/e6j8qYvP8pvGwMQfrmN9YB0zB39YzCSdaUaNrWGD3iCBxg6lwSO\n\tLKeRhxxfiXCIYEf3vwOsP3YMx2JkD5doseXmWBGW1U0T/oJF+DVfKB6mv5UfsTzpVhJRgee7\n\t4jkjqFq4qsUGxcvF2xtRkfHFpZDbRgRlVmiWkqDkT4qMA+4q1y/dWwshSKi/uwVZNycuLsz+\n\t+OD8xPNCsMTqeUkAKfbD8xW4LCay3r/dD2ckoxRxtMD9eOAyu5wYzo/ydIPTh1QEj9SYyvp8\n\tO0g6CpxEwyHUQtF5oh15O018z3ZLztFJKR3RD42VKVsrnNDKnoY0f4U0z7eJv2NeF8xHMuiU\n\tRCIzqxX1GVYaNkKTnb/Qja8hnYnkUzY1Lc+OtwiGmXTwYsPZjjAaDX35J/RSKAoy5wGo/YFA\n\tJxB1gWThL4kOTbsqqXj9GLcyOImkW0lJGGR3o/fV91Zh63S5TKnf2YGGGzxki+ADdxVQAm+Q\n\tsbsRB8KNNvVXBOVNwko86rQqF9drZuw=","Organization":"Ideas on Board","Message-ID":"<946923d6-bf47-30af-ce31-2ef598b2cd46@ideasonboard.com>","Date":"Tue, 5 Mar 2019 15:04:01 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.4.0","MIME-Version":"1.0","In-Reply-To":"<20190305124733.GB4949@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: v4l2_device: Close\n\tPlane dmabuf fd","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 05 Mar 2019 15:04:04 -0000"}}]