[{"id":2292,"web_url":"https://patchwork.libcamera.org/comment/2292/","msgid":"<20190718135640.GB8641@pendragon.ideasonboard.com>","date":"2019-07-18T13:56:40","subject":"Re: [libcamera-devel] [PATCH] libcamera: camera: Fail to configure\n\ton mis-configured streams","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 Thu, Jul 18, 2019 at 05:27:54AM +0100, Kieran Bingham wrote:\n> Uses of LOG(x, Fatal) and ASSERT() should be capable of continuing\n> without crashing.\n> \n> Camera::configure() validates that each StreamConfiguration has a\n> Stream* and reports a Fatal error otherwise. The code then goes on to\n> dereference the Stream pointer which will be a nullptr in the event of\n> the Fatal error being triggered.\n> \n> In release builds, ASSERTS are compiled out, and LOG(x, Fatal) should\n> attempt to continue or report the error up the call-stack.\n> \n> Make Camera::configure() return an error in the event that the Stream*\n> is not valid. This will cause the configure operation to fail and the\n> application will be notified just as other errors in the\n> Camera::configure() operation do.\n> \n> Signed-off-by: Kieran Bingham \n> ---\n> src/libcamera/camera.cpp | 4 +++-\n> 1 file changed, 3 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 76c737cb9381..7ad9e0e48686 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -672,9 +672,11 @@ int Camera::configure(CameraConfiguration *config)\n> \tactiveStreams_.clear();\n> \tfor (const StreamConfiguration &cfg : *config) {\n> \t\tStream *stream = cfg.stream();\n> -\t\tif (!stream)\n> +\t\tif (!stream) {\n> \t\t\tLOG(Camera, Fatal)\n> \t\t\t\t<< \"Pipeline handler failed to update stream configuration\";\n> +\t\t\treturn -EINVAL;\n\nOverall this makes sense, but I wonder if -EINVAL is the appropriate\nerror code. The issue at hand is that the pipeline handler is buggy. A\nspecific error code to denote errors that are not supposed to happen\never would be best I think (and bonus points if we could use that error\ncode consistently through libcamera).\n\n> +\t\t}\n> \n> \t\tstream->configuration_ = cfg;\n> \t\tactiveStreams_.insert(stream);","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 6FBAD60BE1\n\tfor ;\n\tThu, 18 Jul 2019 15:56:46 +0200 (CEST)","from pendragon.ideasonboard.com (softbank126159220198.bbtec.net\n\t[126.159.220.198])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1D4B431C;\n\tThu, 18 Jul 2019 15:56:44 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1563458206;\n\tbh=vmIjXu/9NWNoFmSS7klJpIfP0PMuiQUTfjhw7XLfkY0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=POBeVt5/7+bbltt+WQzd8QCQRTYX9c+Gf01gZQBJ3tr/5O2LzsFi0yjCsvoWm0Kcc\n\tmt5Tb0OH9A3F46q1bTtjYcbVONIP/OY4nP4jClGhNoCUrUus2AsJh3eFYtVozyagXm\n\t22/Z29KEz7OeGjPoT8G862pchLVQSBZ3Dek+4GMM=","Date":"Thu, 18 Jul 2019 16:56:40 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"LibCamera Devel ","Message-ID":"<20190718135640.GB8641@pendragon.ideasonboard.com>","References":"<20190718042754.26535-1-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190718042754.26535-1-kieran.bingham@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH] libcamera: camera: Fail to configure\n\ton mis-configured streams","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":"Thu, 18 Jul 2019 13:56:46 -0000"}},{"id":2295,"web_url":"https://patchwork.libcamera.org/comment/2295/","msgid":"<4680f40e-d94b-b310-d897-336d4578ce01@ideasonboard.com>","date":"2019-07-19T07:33:46","subject":"Re: [libcamera-devel] [PATCH] libcamera: camera: Fail to configure\n\ton mis-configured streams","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 18/07/2019 14:56, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> Thank you for the patch.\n> \n> On Thu, Jul 18, 2019 at 05:27:54AM +0100, Kieran Bingham wrote:\n>> Uses of LOG(x, Fatal) and ASSERT() should be capable of continuing\n>> without crashing.\n>>\n>> Camera::configure() validates that each StreamConfiguration has a\n>> Stream* and reports a Fatal error otherwise. The code then goes on to\n>> dereference the Stream pointer which will be a nullptr in the event of\n>> the Fatal error being triggered.\n>>\n>> In release builds, ASSERTS are compiled out, and LOG(x, Fatal) should\n>> attempt to continue or report the error up the call-stack.\n>>\n>> Make Camera::configure() return an error in the event that the Stream*\n>> is not valid. This will cause the configure operation to fail and the\n>> application will be notified just as other errors in the\n>> Camera::configure() operation do.\n>>\n>> Signed-off-by: Kieran Bingham \n>> ---\n>> src/libcamera/camera.cpp | 4 +++-\n>> 1 file changed, 3 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n>> index 76c737cb9381..7ad9e0e48686 100644\n>> --- a/src/libcamera/camera.cpp\n>> +++ b/src/libcamera/camera.cpp\n>> @@ -672,9 +672,11 @@ int Camera::configure(CameraConfiguration *config)\n>> \tactiveStreams_.clear();\n>> \tfor (const StreamConfiguration &cfg : *config) {\n>> \t\tStream *stream = cfg.stream();\n>> -\t\tif (!stream)\n>> +\t\tif (!stream) {\n>> \t\t\tLOG(Camera, Fatal)\n>> \t\t\t\t<< \"Pipeline handler failed to update stream configuration\";\n>> +\t\t\treturn -EINVAL;\n> \n> Overall this makes sense, but I wonder if -EINVAL is the appropriate\n> error code. The issue at hand is that the pipeline handler is buggy. A\n> specific error code to denote errors that are not supposed to happen\n> ever would be best I think (and bonus points if we could use that error\n> code consistently through libcamera).\n\n-EPIPE 'sounds' appropriate, because we have a broken pipe if we don't\nhave a stream pointer - as long as that doesn't get too confusing\nagainst other types of pipe :)\n\nOr we could use: \"ESTRPIPE 86 Streams pipe error\"?\n\nBut then again for a generic 'Fatal' error code we could choose one of:\n\n EOWNERDEAD 130 Owner died\n ENOTRECOVERABLE 131 State not recoverable\n\nI quite like both of those for a 'fatal' error.\n\n\n>> +\t\t}\n>> \n>> \t\tstream->configuration_ = cfg;\n>> \t\tactiveStreams_.insert(stream);\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 6341460C00\n\tfor ;\n\tFri, 19 Jul 2019 09:33:56 +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 7DCCD31C;\n\tFri, 19 Jul 2019 09:33:48 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1563521628;\n\tbh=wLXEvnp95ZfBRdzj7sNCvbEDenrN2WDTY64/k0RtlAU=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=MTGUdcdarOltPTWJwRm119+bJN3hIOht6IlbJZKNdRAJdkObUKta9Xm/Y7zH9Zoso\n\tSbDOs4MDlzqowi7y6E08jg7Dbq8bBaFDADBPKTI6PDaoqw0jNNez5lGmUUlA1pf3kv\n\twGYCSiB5FgfvLM6l/sxJLi0aGqq42fe8jJkz5Zrk=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Laurent Pinchart ","Cc":"LibCamera Devel ","References":"<20190718042754.26535-1-kieran.bingham@ideasonboard.com>\n\t<20190718135640.GB8641@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":"<4680f40e-d94b-b310-d897-336d4578ce01@ideasonboard.com>","Date":"Fri, 19 Jul 2019 08:33:46 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.7.2","MIME-Version":"1.0","In-Reply-To":"<20190718135640.GB8641@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH] libcamera: camera: Fail to configure\n\ton mis-configured streams","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":"Fri, 19 Jul 2019 07:33:56 -0000"}},{"id":2301,"web_url":"https://patchwork.libcamera.org/comment/2301/","msgid":"<20190720132237.GJ1679@wyvern>","date":"2019-07-20T13:22:37","subject":"Re: [libcamera-devel] [PATCH] libcamera: camera: Fail to configure\n\ton mis-configured streams","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Kieran,\n\nThanks for your work.\n\nOn 2019-07-19 08:33:46 +0100, Kieran Bingham wrote:\n> Hi Laurent,\n> \n> On 18/07/2019 14:56, Laurent Pinchart wrote:\n> > Hi Kieran,\n> > \n> > Thank you for the patch.\n> > \n> > On Thu, Jul 18, 2019 at 05:27:54AM +0100, Kieran Bingham wrote:\n> >> Uses of LOG(x, Fatal) and ASSERT() should be capable of continuing\n> >> without crashing.\n> >>\n> >> Camera::configure() validates that each StreamConfiguration has a\n> >> Stream* and reports a Fatal error otherwise. The code then goes on to\n> >> dereference the Stream pointer which will be a nullptr in the event of\n> >> the Fatal error being triggered.\n> >>\n> >> In release builds, ASSERTS are compiled out, and LOG(x, Fatal) should\n> >> attempt to continue or report the error up the call-stack.\n> >>\n> >> Make Camera::configure() return an error in the event that the Stream*\n> >> is not valid. This will cause the configure operation to fail and the\n> >> application will be notified just as other errors in the\n> >> Camera::configure() operation do.\n> >>\n> >> Signed-off-by: Kieran Bingham \n> >> ---\n> >> src/libcamera/camera.cpp | 4 +++-\n> >> 1 file changed, 3 insertions(+), 1 deletion(-)\n> >>\n> >> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> >> index 76c737cb9381..7ad9e0e48686 100644\n> >> --- a/src/libcamera/camera.cpp\n> >> +++ b/src/libcamera/camera.cpp\n> >> @@ -672,9 +672,11 @@ int Camera::configure(CameraConfiguration *config)\n> >> \tactiveStreams_.clear();\n> >> \tfor (const StreamConfiguration &cfg : *config) {\n> >> \t\tStream *stream = cfg.stream();\n> >> -\t\tif (!stream)\n> >> +\t\tif (!stream) {\n> >> \t\t\tLOG(Camera, Fatal)\n> >> \t\t\t\t<< \"Pipeline handler failed to update stream configuration\";\n> >> +\t\t\treturn -EINVAL;\n> > \n> > Overall this makes sense, but I wonder if -EINVAL is the appropriate\n> > error code. The issue at hand is that the pipeline handler is buggy. A\n> > specific error code to denote errors that are not supposed to happen\n> > ever would be best I think (and bonus points if we could use that error\n> > code consistently through libcamera).\n> \n> -EPIPE 'sounds' appropriate, because we have a broken pipe if we don't\n> have a stream pointer - as long as that doesn't get too confusing\n> against other types of pipe :)\n> \n> Or we could use: \"ESTRPIPE 86 Streams pipe error\"?\n> \n> But then again for a generic 'Fatal' error code we could choose one of:\n> \n> EOWNERDEAD 130 Owner died\n> ENOTRECOVERABLE 131 State not recoverable\n> \n> I quite like both of those for a 'fatal' error.\n\nIs this not an academic question? As LOG(..., Fatal) will never return \nand terminate the application?\n\nIf we need a return statement to keep analysers happy add one in the LOG \nmacro. The return code could be EXIT_FAILURE.\n\n> \n> \n> >> +\t\t}\n> >> \n> >> \t\tstream->configuration_ = cfg;\n> >> \t\tactiveStreams_.insert(stream);\n> > \n> \n> -- \n> Regards\n> --\n> Kieran\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from mail-pl1-x642.google.com (mail-pl1-x642.google.com\n\t[IPv6:2607:f8b0:4864:20::642])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0ACC660C00\n\tfor ;\n\tSat, 20 Jul 2019 15:22:42 +0200 (CEST)","by mail-pl1-x642.google.com with SMTP id 4so9987714pld.10\n\tfor ;\n\tSat, 20 Jul 2019 06:22:41 -0700 (PDT)","from localhost (M106185144154.v4.enabler.ne.jp. [106.185.144.154])\n\tby smtp.gmail.com with ESMTPSA id\n\tt9sm34544554pji.18.2019.07.20.06.22.39\n\t(version=TLS1_3 cipher=AEAD-AES256-GCM-SHA384 bits=256/256);\n\tSat, 20 Jul 2019 06:22:39 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=cvdY3SqkOgI+TOCLqSFq//ijbQOVGEIrI8ZDQroC/v4=;\n\tb=aOveKPmfYkVCyMRX9JflhF4yPIfih/Q01b2AQ8V4yQsMrqEi9iFvLWY2EZy5MRchHq\n\tOZNksYEAdxGfhYX8Yj33jzK+kwTybsxutFccbH/yiesKOSWo3HQwymQynWQ9x8xw34uR\n\tqC1UqLLGIveEWox+lzqS/kT8gCTQXg8ifqfcJ8gJJR42hyJyrVdfNISyLm1cTsDNDUeu\n\tuZhC6e9g0zBSofdAy4f0iqxJD1GGOGSNez4rghfati7NQfDghHgfzqD3MzqanRt6dikY\n\tfiHcS2EqljZdDOXd3BFtpiO0ie4UQxwVbJioLP8+6diauhFUsE18/Kim5np2TCLWp/If\n\tWiEQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=cvdY3SqkOgI+TOCLqSFq//ijbQOVGEIrI8ZDQroC/v4=;\n\tb=DXewON1hDPufhVRrlNphkGjEil0MpikS3e0Aa/9adYMLKfO2BDhpSv91p9Gsrj5mX1\n\tkbbLGGwlLlv2DMiQ5iIqUbM4NP6TDKOYpFLLBLUVL6sJh8x1Fwnf0GYa+VOBGhm6YB0r\n\t6cLmWS2acYb9XoY4S05z8jcMFzwBwKy7WErlWB4R81U8JANS1oGAzDENl0h2ZexA47U9\n\tod9zsgkYiPCzUTke9mElV+LOTJz4MC3QVfb9Ip3bnhyCPKiv2KjfcHDT+wjBZu+fOYDr\n\t+XhT0P7tSXlkM7UnLxJa4RK4Jpt9LsAzivTiOu8CqRtWgXDb4ZCl2N2d3ZurHW0QDCqC\n\tIEng==","X-Gm-Message-State":"APjAAAXAKE89k3pG86A07MeXyM8pCbQfr7jCRvnIZprHXzE+jYkhPpZt\n\tQLkRv6rp1hMMtdzDuvHWUrU=","X-Google-Smtp-Source":"APXvYqz0+7/r+okB1+yidxi+xsSabS3WRx+OOSw1T8x8KSp8bvsx3da9h6v5BPEJTnLzv9cJdrQuQw==","X-Received":"by 2002:a17:902:6ac6:: with SMTP id\n\ti6mr63441218plt.233.1563628960509; \n\tSat, 20 Jul 2019 06:22:40 -0700 (PDT)","Date":"Sat, 20 Jul 2019 22:22:37 +0900","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Kieran Bingham ","Cc":"Laurent Pinchart ,\n\tLibCamera Devel ","Message-ID":"<20190720132237.GJ1679@wyvern>","References":"<20190718042754.26535-1-kieran.bingham@ideasonboard.com>\n\t<20190718135640.GB8641@pendragon.ideasonboard.com>\n\t<4680f40e-d94b-b310-d897-336d4578ce01@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<4680f40e-d94b-b310-d897-336d4578ce01@ideasonboard.com>","User-Agent":"Mutt/1.12.1 (2019-06-15)","Subject":"Re: [libcamera-devel] [PATCH] libcamera: camera: Fail to configure\n\ton mis-configured streams","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":"Sat, 20 Jul 2019 13:22:42 -0000"}},{"id":2303,"web_url":"https://patchwork.libcamera.org/comment/2303/","msgid":"","date":"2019-07-22T09:54:26","subject":"Re: [libcamera-devel] [PATCH] libcamera: camera: Fail to configure\n\ton mis-configured streams","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Niklas,\n\nOn 20/07/2019 14:22, Niklas Söderlund wrote:\n> Hi Kieran,\n> \n> Thanks for your work.\n> \n> On 2019-07-19 08:33:46 +0100, Kieran Bingham wrote:\n>> Hi Laurent,\n>>\n>> On 18/07/2019 14:56, Laurent Pinchart wrote:\n>>> Hi Kieran,\n>>>\n>>> Thank you for the patch.\n>>>\n>>> On Thu, Jul 18, 2019 at 05:27:54AM +0100, Kieran Bingham wrote:\n>>>> Uses of LOG(x, Fatal) and ASSERT() should be capable of continuing\n>>>> without crashing.\n>>>>\n>>>> Camera::configure() validates that each StreamConfiguration has a\n>>>> Stream* and reports a Fatal error otherwise. The code then goes on to\n>>>> dereference the Stream pointer which will be a nullptr in the event of\n>>>> the Fatal error being triggered.\n>>>>\n>>>> In release builds, ASSERTS are compiled out, and LOG(x, Fatal) should\n>>>> attempt to continue or report the error up the call-stack.\n>>>>\n>>>> Make Camera::configure() return an error in the event that the Stream*\n>>>> is not valid. This will cause the configure operation to fail and the\n>>>> application will be notified just as other errors in the\n>>>> Camera::configure() operation do.\n>>>>\n>>>> Signed-off-by: Kieran Bingham \n>>>> ---\n>>>> src/libcamera/camera.cpp | 4 +++-\n>>>> 1 file changed, 3 insertions(+), 1 deletion(-)\n>>>>\n>>>> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n>>>> index 76c737cb9381..7ad9e0e48686 100644\n>>>> --- a/src/libcamera/camera.cpp\n>>>> +++ b/src/libcamera/camera.cpp\n>>>> @@ -672,9 +672,11 @@ int Camera::configure(CameraConfiguration *config)\n>>>> \tactiveStreams_.clear();\n>>>> \tfor (const StreamConfiguration &cfg : *config) {\n>>>> \t\tStream *stream = cfg.stream();\n>>>> -\t\tif (!stream)\n>>>> +\t\tif (!stream) {\n>>>> \t\t\tLOG(Camera, Fatal)\n>>>> \t\t\t\t<< \"Pipeline handler failed to update stream configuration\";\n>>>> +\t\t\treturn -EINVAL;\n>>>\n>>> Overall this makes sense, but I wonder if -EINVAL is the appropriate\n>>> error code. The issue at hand is that the pipeline handler is buggy. A\n>>> specific error code to denote errors that are not supposed to happen\n>>> ever would be best I think (and bonus points if we could use that error\n>>> code consistently through libcamera).\n>>\n>> -EPIPE 'sounds' appropriate, because we have a broken pipe if we don't\n>> have a stream pointer - as long as that doesn't get too confusing\n>> against other types of pipe :)\n>>\n>> Or we could use: \"ESTRPIPE 86 Streams pipe error\"?\n>>\n>> But then again for a generic 'Fatal' error code we could choose one of:\n>>\n>> EOWNERDEAD 130 Owner died\n>> ENOTRECOVERABLE 131 State not recoverable\n>>\n>> I quite like both of those for a 'fatal' error.\n> \n> Is this not an academic question? As LOG(..., Fatal) will never return \n> and terminate the application?\n\nCurrently, you are correct - but while discussing with Laurent on\nanother patch somewhere, or perhaps it was IRC - I can't recall, we\nrealised that we might have to make LOG(x, Fatal) *non-fatal* on release\nbuilds.\n\n(Just as assertions are compiled out in release builds)\n\nIt might be that we leave this to a packaging policy as well.\n\nAnyway, this is the only part of the code that in the event of the\nfailure being detected would then go on to do a null-dereference - so I\nthought the small fix up was worth it.\n\nPerhaps I should add a comment before the return explaining that this\ncode is unreachable in debug builds, but could be enabled in future\nrelease builds?\n\n> If we need a return statement to keep analysers happy add one in the LOG \n> macro. The return code could be EXIT_FAILURE.\n\n\nEeeep - no - I really don't want to have a macro which returns implicitly.\n\nI would really prefer to keep return paths explicit.\n\n\n\n> \n>>\n>>\n>>>> +\t\t}\n>>>> \n>>>> \t\tstream->configuration_ = cfg;\n>>>> \t\tactiveStreams_.insert(stream);\n>>>\n>>\n>> -- \n>> Regards\n>> --\n>> Kieran\n>> _______________________________________________\n>> libcamera-devel mailing list\n>> libcamera-devel@lists.libcamera.org\n>> https://lists.libcamera.org/listinfo/libcamera-devel\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 7A32160BDD\n\tfor ;\n\tMon, 22 Jul 2019 11:54:29 +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 B4467807;\n\tMon, 22 Jul 2019 11:54:28 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1563789269;\n\tbh=p/jDT5jwWSr4rDgT9q0r5CLROZaEQUtTxdNiQdN4KRo=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=XqtQ2f/ptm8kUzeWfvk6xXKR+zp0nqe3eGj0W19GTL/kCenECVR7Xr3jjr3WBC0Qe\n\t5miUpSyqNZgJeNNDaS8YCLjFXgAB8rwlnLptAnth9IGJMCYDskQO5qO7gZlI7By9Up\n\tiaU98civtghZo6vGsumf5KCZyMRO708f4V6tVM9Q=","Reply-To":"kieran.bingham@ideasonboard.com","To":"=?utf-8?q?Niklas_S=C3=B6derlund?= ","Cc":"Laurent Pinchart ,\n\tLibCamera Devel ","References":"<20190718042754.26535-1-kieran.bingham@ideasonboard.com>\n\t<20190718135640.GB8641@pendragon.ideasonboard.com>\n\t<4680f40e-d94b-b310-d897-336d4578ce01@ideasonboard.com>\n\t<20190720132237.GJ1679@wyvern>","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":"","Date":"Mon, 22 Jul 2019 10:54:26 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.8.0","MIME-Version":"1.0","In-Reply-To":"<20190720132237.GJ1679@wyvern>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH] libcamera: camera: Fail to configure\n\ton mis-configured streams","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":"Mon, 22 Jul 2019 09:54:29 -0000"}},{"id":2305,"web_url":"https://patchwork.libcamera.org/comment/2305/","msgid":"<20190730040905.GC4852@pendragon.ideasonboard.com>","date":"2019-07-30T04:09:05","subject":"Re: [libcamera-devel] [PATCH] libcamera: camera: Fail to configure\n\ton mis-configured streams","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Mon, Jul 22, 2019 at 10:54:26AM +0100, Kieran Bingham wrote:\n> On 20/07/2019 14:22, Niklas Söderlund wrote:\n> > On 2019-07-19 08:33:46 +0100, Kieran Bingham wrote:\n> >> On 18/07/2019 14:56, Laurent Pinchart wrote:\n> >>> On Thu, Jul 18, 2019 at 05:27:54AM +0100, Kieran Bingham wrote:\n> >>>> Uses of LOG(x, Fatal) and ASSERT() should be capable of continuing\n> >>>> without crashing.\n> >>>>\n> >>>> Camera::configure() validates that each StreamConfiguration has a\n> >>>> Stream* and reports a Fatal error otherwise. The code then goes on to\n> >>>> dereference the Stream pointer which will be a nullptr in the event of\n> >>>> the Fatal error being triggered.\n> >>>>\n> >>>> In release builds, ASSERTS are compiled out, and LOG(x, Fatal) should\n> >>>> attempt to continue or report the error up the call-stack.\n> >>>>\n> >>>> Make Camera::configure() return an error in the event that the Stream*\n> >>>> is not valid. This will cause the configure operation to fail and the\n> >>>> application will be notified just as other errors in the\n> >>>> Camera::configure() operation do.\n> >>>>\n> >>>> Signed-off-by: Kieran Bingham \n> >>>> ---\n> >>>> src/libcamera/camera.cpp | 4 +++-\n> >>>> 1 file changed, 3 insertions(+), 1 deletion(-)\n> >>>>\n> >>>> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> >>>> index 76c737cb9381..7ad9e0e48686 100644\n> >>>> --- a/src/libcamera/camera.cpp\n> >>>> +++ b/src/libcamera/camera.cpp\n> >>>> @@ -672,9 +672,11 @@ int Camera::configure(CameraConfiguration *config)\n> >>>> \tactiveStreams_.clear();\n> >>>> \tfor (const StreamConfiguration &cfg : *config) {\n> >>>> \t\tStream *stream = cfg.stream();\n> >>>> -\t\tif (!stream)\n> >>>> +\t\tif (!stream) {\n> >>>> \t\t\tLOG(Camera, Fatal)\n> >>>> \t\t\t\t<< \"Pipeline handler failed to update stream configuration\";\n> >>>> +\t\t\treturn -EINVAL;\n> >>>\n> >>> Overall this makes sense, but I wonder if -EINVAL is the appropriate\n> >>> error code. The issue at hand is that the pipeline handler is buggy. A\n> >>> specific error code to denote errors that are not supposed to happen\n> >>> ever would be best I think (and bonus points if we could use that error\n> >>> code consistently through libcamera).\n> >>\n> >> -EPIPE 'sounds' appropriate, because we have a broken pipe if we don't\n> >> have a stream pointer - as long as that doesn't get too confusing\n> >> against other types of pipe :)\n> >>\n> >> Or we could use: \"ESTRPIPE 86 Streams pipe error\"?\n> >>\n> >> But then again for a generic 'Fatal' error code we could choose one of:\n> >>\n> >> EOWNERDEAD 130 Owner died\n> >> ENOTRECOVERABLE 131 State not recoverable\n> >>\n> >> I quite like both of those for a 'fatal' error.\n\nI quite like ENOTRECOVERABLE too.\n\n> > Is this not an academic question? As LOG(..., Fatal) will never return \n> > and terminate the application?\n> \n> Currently, you are correct - but while discussing with Laurent on\n> another patch somewhere, or perhaps it was IRC - I can't recall, we\n> realised that we might have to make LOG(x, Fatal) *non-fatal* on release\n> builds.\n> \n> (Just as assertions are compiled out in release builds)\n> \n> It might be that we leave this to a packaging policy as well.\n> \n> Anyway, this is the only part of the code that in the event of the\n> failure being detected would then go on to do a null-dereference - so I\n> thought the small fix up was worth it.\n> \n> Perhaps I should add a comment before the return explaining that this\n> code is unreachable in debug builds, but could be enabled in future\n> release builds?\n\nI think mentioning it in the commit log is enough, there's no need to\nsprinkle copies of the information through the code.\n\n> > If we need a return statement to keep analysers happy add one in the LOG \n> > macro. The return code could be EXIT_FAILURE.\n> \n> Eeeep - no - I really don't want to have a macro which returns implicitly.\n> \n> I would really prefer to keep return paths explicit.\n\nI also dislike hiding return statements in macros, even if it could help\nstandardising the return value. Let's also note that LOG(..., Fatal) can\nbe used in functions returning any type, so we can't simply return\n-EWHATEVER there.\n\n> >>>> +\t\t}\n> >>>> \n> >>>> \t\tstream->configuration_ = cfg;\n> >>>> \t\tactiveStreams_.insert(stream);","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 8A9A0615A7\n\tfor ;\n\tTue, 30 Jul 2019 06:09:13 +0200 (CEST)","from pendragon.ideasonboard.com (om126208166005.22.openmobile.ne.jp\n\t[126.208.166.5])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0E16BCC;\n\tTue, 30 Jul 2019 06:09:10 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1564459753;\n\tbh=GbZo/x8rTwIzzbwxKUKjOE1+mdUjFoKWaYVNQGqKCdU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=GmhOC81rXj4Xs/O8AgGXfjrm4/PcGQzkHqJtsYBhwBmtOyOcKdJ1rc1a1xEDN/LqG\n\tgGrIBi/Yn5kJ3ULbaa3EKybMCoNHptpNbk5Ro2k+kirxy5Chm2ONJ+t605QzW6jTk8\n\tVSskwGb+MKpmUnLc1qMi9C2LPf/5QCQQ4nyYt7Gw=","Date":"Tue, 30 Jul 2019 07:09:05 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Niklas =?utf-8?q?S=C3=B6derlund?= ,\n\tLibCamera Devel ","Message-ID":"<20190730040905.GC4852@pendragon.ideasonboard.com>","References":"<20190718042754.26535-1-kieran.bingham@ideasonboard.com>\n\t<20190718135640.GB8641@pendragon.ideasonboard.com>\n\t<4680f40e-d94b-b310-d897-336d4578ce01@ideasonboard.com>\n\t<20190720132237.GJ1679@wyvern>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH] libcamera: camera: Fail to configure\n\ton mis-configured streams","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, 30 Jul 2019 04:09:13 -0000"}},{"id":2307,"web_url":"https://patchwork.libcamera.org/comment/2307/","msgid":"<20190730053715.GI3186@bigcity.dyn.berto.se>","date":"2019-07-30T05:37:15","subject":"Re: [libcamera-devel] [PATCH] libcamera: camera: Fail to configure\n\ton mis-configured streams","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi,\n\nOn 2019-07-30 07:09:05 +0300, Laurent Pinchart wrote:\n> Hello,\n> \n> On Mon, Jul 22, 2019 at 10:54:26AM +0100, Kieran Bingham wrote:\n> > On 20/07/2019 14:22, Niklas Söderlund wrote:\n> > > On 2019-07-19 08:33:46 +0100, Kieran Bingham wrote:\n> > >> On 18/07/2019 14:56, Laurent Pinchart wrote:\n> > >>> On Thu, Jul 18, 2019 at 05:27:54AM +0100, Kieran Bingham wrote:\n> > >>>> Uses of LOG(x, Fatal) and ASSERT() should be capable of continuing\n> > >>>> without crashing.\n> > >>>>\n> > >>>> Camera::configure() validates that each StreamConfiguration has a\n> > >>>> Stream* and reports a Fatal error otherwise. The code then goes on to\n> > >>>> dereference the Stream pointer which will be a nullptr in the event of\n> > >>>> the Fatal error being triggered.\n> > >>>>\n> > >>>> In release builds, ASSERTS are compiled out, and LOG(x, Fatal) should\n> > >>>> attempt to continue or report the error up the call-stack.\n> > >>>>\n> > >>>> Make Camera::configure() return an error in the event that the Stream*\n> > >>>> is not valid. This will cause the configure operation to fail and the\n> > >>>> application will be notified just as other errors in the\n> > >>>> Camera::configure() operation do.\n> > >>>>\n> > >>>> Signed-off-by: Kieran Bingham \n> > >>>> ---\n> > >>>> src/libcamera/camera.cpp | 4 +++-\n> > >>>> 1 file changed, 3 insertions(+), 1 deletion(-)\n> > >>>>\n> > >>>> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > >>>> index 76c737cb9381..7ad9e0e48686 100644\n> > >>>> --- a/src/libcamera/camera.cpp\n> > >>>> +++ b/src/libcamera/camera.cpp\n> > >>>> @@ -672,9 +672,11 @@ int Camera::configure(CameraConfiguration *config)\n> > >>>> \tactiveStreams_.clear();\n> > >>>> \tfor (const StreamConfiguration &cfg : *config) {\n> > >>>> \t\tStream *stream = cfg.stream();\n> > >>>> -\t\tif (!stream)\n> > >>>> +\t\tif (!stream) {\n> > >>>> \t\t\tLOG(Camera, Fatal)\n> > >>>> \t\t\t\t<< \"Pipeline handler failed to update stream configuration\";\n> > >>>> +\t\t\treturn -EINVAL;\n> > >>>\n> > >>> Overall this makes sense, but I wonder if -EINVAL is the appropriate\n> > >>> error code. The issue at hand is that the pipeline handler is buggy. A\n> > >>> specific error code to denote errors that are not supposed to happen\n> > >>> ever would be best I think (and bonus points if we could use that error\n> > >>> code consistently through libcamera).\n> > >>\n> > >> -EPIPE 'sounds' appropriate, because we have a broken pipe if we don't\n> > >> have a stream pointer - as long as that doesn't get too confusing\n> > >> against other types of pipe :)\n> > >>\n> > >> Or we could use: \"ESTRPIPE 86 Streams pipe error\"?\n> > >>\n> > >> But then again for a generic 'Fatal' error code we could choose one of:\n> > >>\n> > >> EOWNERDEAD 130 Owner died\n> > >> ENOTRECOVERABLE 131 State not recoverable\n> > >>\n> > >> I quite like both of those for a 'fatal' error.\n> \n> I quite like ENOTRECOVERABLE too.\n> \n> > > Is this not an academic question? As LOG(..., Fatal) will never return \n> > > and terminate the application?\n> > \n> > Currently, you are correct - but while discussing with Laurent on\n> > another patch somewhere, or perhaps it was IRC - I can't recall, we\n> > realised that we might have to make LOG(x, Fatal) *non-fatal* on release\n> > builds.\n> > \n> > (Just as assertions are compiled out in release builds)\n> > \n> > It might be that we leave this to a packaging policy as well.\n> > \n> > Anyway, this is the only part of the code that in the event of the\n> > failure being detected would then go on to do a null-dereference - so I\n> > thought the small fix up was worth it.\n> > \n> > Perhaps I should add a comment before the return explaining that this\n> > code is unreachable in debug builds, but could be enabled in future\n> > release builds?\n> \n> I think mentioning it in the commit log is enough, there's no need to\n> sprinkle copies of the information through the code.\n\nI'm not sure I like the idea of allowing continued execution after a \nLOG(..., Fatal) in release builds. It feels a bit like Java, whenever \none looks at a process logs it's sprinkled with unhandled expectation \nlog messages and trying to narrow down a problem is impossible so one \njust gives up and once prejudice against the language is reinforced ;-)\n\nI'm not against changing/removing LOG(..., Fatal) so that it always \nallow continued execution after it's been called. But I think it should \nbe the same behavior always, we will have enough trouble as it is to \ntest all different error paths :-)\n\nASSERT() for me is different, it's usually at the start of functions and \ncould be in hot paths and do not alter the execution order of functions \nso it make sens to disable them in release builds as it decreases the \nexecution time. While I think of LOG(..., Fatal) as BUG() if it happens \nwe are in deep shit and there is no point in trying to recover from it.\n\n> \n> > > If we need a return statement to keep analysers happy add one in the LOG \n> > > macro. The return code could be EXIT_FAILURE.\n> > \n> > Eeeep - no - I really don't want to have a macro which returns implicitly.\n> > \n> > I would really prefer to keep return paths explicit.\n> \n> I also dislike hiding return statements in macros, even if it could help\n> standardising the return value. Let's also note that LOG(..., Fatal) can\n> be used in functions returning any type, so we can't simply return\n> -EWHATEVER there.\n> \n> > >>>> +\t\t}\n> > >>>> \n> > >>>> \t\tstream->configuration_ = cfg;\n> > >>>> \t\tactiveStreams_.insert(stream);\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from mail-lj1-x241.google.com (mail-lj1-x241.google.com\n\t[IPv6:2a00:1450:4864:20::241])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5B218615A7\n\tfor ;\n\tTue, 30 Jul 2019 07:37:17 +0200 (CEST)","by mail-lj1-x241.google.com with SMTP id m8so27232413lji.7\n\tfor ;\n\tMon, 29 Jul 2019 22:37:17 -0700 (PDT)","from localhost (customer-145-14-112-32.stosn.net. [145.14.112.32])\n\tby smtp.gmail.com with ESMTPSA id\n\t11sm13157725ljc.66.2019.07.29.22.37.15\n\t(version=TLS1_3 cipher=AEAD-AES256-GCM-SHA384 bits=256/256);\n\tMon, 29 Jul 2019 22:37:15 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=iJydj95yXjIg9P2oRry3cP4yXp4DkUACIPosyVA1RIM=;\n\tb=ZYy0v9bCwVVUDHLEVbaXCFD0b1H8lhe6oJjKdzg4m4HYt5RvsxsS4+BfsOf2+u+p5P\n\tuARkGsNSplXoKH+isABRxija0/1MHH3T7aqTNQwJ33+UiWUYtZn2/fjSDMTFY28rDbi2\n\tIsGWmuzSO8R+vQYnaqPGfyU7Y23s+rCVRSgF7CMbur9glkOBn6E5WIctGcCrY4Q1mkyD\n\tyHjby3LQWjRg6Wc7yBMdyq4EbMH2yUfeF0KVdGYx+LEt9qbKls/fDQwmRdv8evhXnS9n\n\tUMuNqqNs2iENiMGeUxsLBg6da7w7lE8cXcKi4h0T/vflToQ0IRMYzNtjzTMSqzz6VA4M\n\tdNsg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=iJydj95yXjIg9P2oRry3cP4yXp4DkUACIPosyVA1RIM=;\n\tb=hGzmPyyi2bjHorFmpPi3Q1k1wYT81jm9FQwgyADnY29kqkiK/aNmQfaJGC3VzucX/Z\n\t03yehmog7oVlX7NtCn5UjOhufvQuhieTlTXf5py8A/rmBPup3TnrF8m4Vkjj5vilcRcT\n\tqeQ6gCZ3fogF99RlIxeAGwDWakAsK/pDbKf271WTlLheAQvvaFfzCjcFdy6FxjZ1053q\n\tSHx4WK8t4uqTLBDdStwWx2Yzq9oUSEPIg8iVHD5AJwUTNM2D68HjzduCBEwTAmFoIRmD\n\tMXnAJBqPOth+L4XsiRpSHsWfnUCYN8zAtNTo7niGKsU1npTDumaTjXJ7CBgAjEiVppJm\n\t/DfQ==","X-Gm-Message-State":"APjAAAUmzZhDpOAokgSL3ZKeFtKXg8CxKEJG0N4hhJMO2oYg4XzB+mgi\n\tK5ZzG/3vxud51TGY9+YGtAO/XY+v","X-Google-Smtp-Source":"APXvYqyVdmNLIHbtq4raQ7XrighcgxzovBmkC0JJYdq9L/4nDBy4Bo0hbRvv47vYpKMzqB9NC6nwbw==","X-Received":"by 2002:a2e:8515:: with SMTP id\n\tj21mr45603560lji.233.1564465036450; \n\tMon, 29 Jul 2019 22:37:16 -0700 (PDT)","Date":"Tue, 30 Jul 2019 07:37:15 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Cc":"Kieran Bingham ,\n\tLibCamera Devel ","Message-ID":"<20190730053715.GI3186@bigcity.dyn.berto.se>","References":"<20190718042754.26535-1-kieran.bingham@ideasonboard.com>\n\t<20190718135640.GB8641@pendragon.ideasonboard.com>\n\t<4680f40e-d94b-b310-d897-336d4578ce01@ideasonboard.com>\n\t<20190720132237.GJ1679@wyvern>\n\t\n\t<20190730040905.GC4852@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190730040905.GC4852@pendragon.ideasonboard.com>","User-Agent":"Mutt/1.12.1 (2019-06-15)","Subject":"Re: [libcamera-devel] [PATCH] libcamera: camera: Fail to configure\n\ton mis-configured streams","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, 30 Jul 2019 05:37:17 -0000"}},{"id":2308,"web_url":"https://patchwork.libcamera.org/comment/2308/","msgid":"<20190730054913.GA4810@pendragon.ideasonboard.com>","date":"2019-07-30T05:49:13","subject":"Re: [libcamera-devel] [PATCH] libcamera: camera: Fail to configure\n\ton mis-configured streams","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nOn Tue, Jul 30, 2019 at 07:37:15AM +0200, Niklas Söderlund wrote:\n> On 2019-07-30 07:09:05 +0300, Laurent Pinchart wrote:\n> > On Mon, Jul 22, 2019 at 10:54:26AM +0100, Kieran Bingham wrote:\n> >> On 20/07/2019 14:22, Niklas Söderlund wrote:\n> >>> On 2019-07-19 08:33:46 +0100, Kieran Bingham wrote:\n> >>>> On 18/07/2019 14:56, Laurent Pinchart wrote:\n> >>>>> On Thu, Jul 18, 2019 at 05:27:54AM +0100, Kieran Bingham wrote:\n> >>>>>> Uses of LOG(x, Fatal) and ASSERT() should be capable of continuing\n> >>>>>> without crashing.\n> >>>>>>\n> >>>>>> Camera::configure() validates that each StreamConfiguration has a\n> >>>>>> Stream* and reports a Fatal error otherwise. The code then goes on to\n> >>>>>> dereference the Stream pointer which will be a nullptr in the event of\n> >>>>>> the Fatal error being triggered.\n> >>>>>>\n> >>>>>> In release builds, ASSERTS are compiled out, and LOG(x, Fatal) should\n> >>>>>> attempt to continue or report the error up the call-stack.\n> >>>>>>\n> >>>>>> Make Camera::configure() return an error in the event that the Stream*\n> >>>>>> is not valid. This will cause the configure operation to fail and the\n> >>>>>> application will be notified just as other errors in the\n> >>>>>> Camera::configure() operation do.\n> >>>>>>\n> >>>>>> Signed-off-by: Kieran Bingham \n> >>>>>> ---\n> >>>>>> src/libcamera/camera.cpp | 4 +++-\n> >>>>>> 1 file changed, 3 insertions(+), 1 deletion(-)\n> >>>>>>\n> >>>>>> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> >>>>>> index 76c737cb9381..7ad9e0e48686 100644\n> >>>>>> --- a/src/libcamera/camera.cpp\n> >>>>>> +++ b/src/libcamera/camera.cpp\n> >>>>>> @@ -672,9 +672,11 @@ int Camera::configure(CameraConfiguration *config)\n> >>>>>> \tactiveStreams_.clear();\n> >>>>>> \tfor (const StreamConfiguration &cfg : *config) {\n> >>>>>> \t\tStream *stream = cfg.stream();\n> >>>>>> -\t\tif (!stream)\n> >>>>>> +\t\tif (!stream) {\n> >>>>>> \t\t\tLOG(Camera, Fatal)\n> >>>>>> \t\t\t\t<< \"Pipeline handler failed to update stream configuration\";\n> >>>>>> +\t\t\treturn -EINVAL;\n> >>>>>\n> >>>>> Overall this makes sense, but I wonder if -EINVAL is the appropriate\n> >>>>> error code. The issue at hand is that the pipeline handler is buggy. A\n> >>>>> specific error code to denote errors that are not supposed to happen\n> >>>>> ever would be best I think (and bonus points if we could use that error\n> >>>>> code consistently through libcamera).\n> >>>>\n> >>>> -EPIPE 'sounds' appropriate, because we have a broken pipe if we don't\n> >>>> have a stream pointer - as long as that doesn't get too confusing\n> >>>> against other types of pipe :)\n> >>>>\n> >>>> Or we could use: \"ESTRPIPE 86 Streams pipe error\"?\n> >>>>\n> >>>> But then again for a generic 'Fatal' error code we could choose one of:\n> >>>>\n> >>>> EOWNERDEAD 130 Owner died\n> >>>> ENOTRECOVERABLE 131 State not recoverable\n> >>>>\n> >>>> I quite like both of those for a 'fatal' error.\n> > \n> > I quite like ENOTRECOVERABLE too.\n> > \n> >>> Is this not an academic question? As LOG(..., Fatal) will never return \n> >>> and terminate the application?\n> >> \n> >> Currently, you are correct - but while discussing with Laurent on\n> >> another patch somewhere, or perhaps it was IRC - I can't recall, we\n> >> realised that we might have to make LOG(x, Fatal) *non-fatal* on release\n> >> builds.\n> >> \n> >> (Just as assertions are compiled out in release builds)\n> >> \n> >> It might be that we leave this to a packaging policy as well.\n> >> \n> >> Anyway, this is the only part of the code that in the event of the\n> >> failure being detected would then go on to do a null-dereference - so I\n> >> thought the small fix up was worth it.\n> >> \n> >> Perhaps I should add a comment before the return explaining that this\n> >> code is unreachable in debug builds, but could be enabled in future\n> >> release builds?\n> > \n> > I think mentioning it in the commit log is enough, there's no need to\n> > sprinkle copies of the information through the code.\n> \n> I'm not sure I like the idea of allowing continued execution after a \n> LOG(..., Fatal) in release builds. It feels a bit like Java, whenever \n> one looks at a process logs it's sprinkled with unhandled expectation \n> log messages and trying to narrow down a problem is impossible so one \n> just gives up and once prejudice against the language is reinforced ;-)\n> \n> I'm not against changing/removing LOG(..., Fatal) so that it always \n> allow continued execution after it's been called. But I think it should \n> be the same behavior always, we will have enough trouble as it is to \n> test all different error paths :-)\n> \n> ASSERT() for me is different, it's usually at the start of functions and \n> could be in hot paths and do not alter the execution order of functions \n> so it make sens to disable them in release builds as it decreases the \n> execution time. While I think of LOG(..., Fatal) as BUG() if it happens \n> we are in deep shit and there is no point in trying to recover from it.\n\n>From a libcamera point of view that's true, but from an application\npoint of view there may be a use for closing gracefully and saving data.\nFor me LOG(..., Fatal) is a LOG() + ASSERT(), but if we have to keep the\nsame behaviour in both release and debug builds, I would prefer removing\nthe std::abort() call completely from LOG(..., Fatal).\n\n> >>> If we need a return statement to keep analysers happy add one in the LOG \n> >>> macro. The return code could be EXIT_FAILURE.\n> >> \n> >> Eeeep - no - I really don't want to have a macro which returns implicitly.\n> >> \n> >> I would really prefer to keep return paths explicit.\n> > \n> > I also dislike hiding return statements in macros, even if it could help\n> > standardising the return value. Let's also note that LOG(..., Fatal) can\n> > be used in functions returning any type, so we can't simply return\n> > -EWHATEVER there.\n> > \n> >>>>>> +\t\t}\n> >>>>>> \n> >>>>>> \t\tstream->configuration_ = cfg;\n> >>>>>> \t\tactiveStreams_.insert(stream);","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 02B21615A7\n\tfor ;\n\tTue, 30 Jul 2019 07:49:18 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(ae138143.dynamic.ppp.asahi-net.or.jp [14.3.138.143])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BEE3ACC;\n\tTue, 30 Jul 2019 07:49:16 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1564465757;\n\tbh=bm8t2M1J/VPkgfKKXi9KlGC686Bn5gi+Oh2OjSQLAKQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=vt+eTNY2c68hwucwo/4fRBSSGOk32MLaHQHGuh5g8oaKbR5+N8WjxAJ1Vq7pj9k5w\n\tMJIGety5XK3D1YLAQBKMpL7n1MfcBx22/GhqMzNarRW/dpQQtY+TX+Bmh7uYko6Tbq\n\t1EaISJIsTi+dK9CkNNsvGE2o3st4ddfwUGbZKyOA=","Date":"Tue, 30 Jul 2019 08:49:13 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"Kieran Bingham ,\n\tLibCamera Devel ","Message-ID":"<20190730054913.GA4810@pendragon.ideasonboard.com>","References":"<20190718042754.26535-1-kieran.bingham@ideasonboard.com>\n\t<20190718135640.GB8641@pendragon.ideasonboard.com>\n\t<4680f40e-d94b-b310-d897-336d4578ce01@ideasonboard.com>\n\t<20190720132237.GJ1679@wyvern>\n\t\n\t<20190730040905.GC4852@pendragon.ideasonboard.com>\n\t<20190730053715.GI3186@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190730053715.GI3186@bigcity.dyn.berto.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH] libcamera: camera: Fail to configure\n\ton mis-configured streams","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, 30 Jul 2019 05:49:18 -0000"}}]