[{"id":892,"web_url":"https://patchwork.libcamera.org/comment/892/","msgid":"<20190226172016.qzjuqchijjf7w4xn@uno.localdomain>","date":"2019-02-26T17:20:16","subject":"Re: [libcamera-devel] [PATCH 7/8] libcamera: camera: ensure streams\n\tbelong to camera","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n great! If this needs to be checked in other places, you could\n consider a method for this in future.\n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\nOn Tue, Feb 26, 2019 at 03:18:56AM +0100, Niklas Söderlund wrote:\n> Before calling into the pipeline handler make sure the streams provided\n> by the application actually belongs to the camera.\n>\n> Signed-off-by: Niklas Söderlund \n> ---\n> src/libcamera/camera.cpp | 8 ++++++++\n> 1 file changed, 8 insertions(+)\n>\n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 938f21fe80ef8ceb..ba8638009992170f 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -253,6 +253,10 @@ Camera::streamConfiguration(std::set &streams)\n> \tif (!stateIsAtleast(Free) || !streams.size())\n> \t\treturn std::map{};\n>\n> +\tfor (Stream *stream : streams)\n> +\t\tif (streams_.find(stream) == streams_.end())\n> +\t\t\treturn std::map{};\n> +\n> \treturn pipe_->streamConfiguration(this, streams);\n> }\n>\n> @@ -290,6 +294,10 @@ int Camera::configureStreams(std::map &config)\n> \t\treturn -EINVAL;\n> \t}\n>\n> +\tfor (auto const &iter : config)\n> +\t\tif (streams_.find(iter.first) == streams_.end())\n> +\t\t\treturn -EINVAL;\n> +\n> \tret = pipe_->configureStreams(this, config);\n> \tif (ret)\n> \t\treturn ret;\n> --\n> 2.20.1\n>\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 relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[217.70.183.196])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 34161610B2\n\tfor ;\n\tTue, 26 Feb 2019 18:19:48 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay4-d.mail.gandi.net (Postfix) with ESMTPSA id C3357E0004;\n\tTue, 26 Feb 2019 17:19:47 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Tue, 26 Feb 2019 18:20:16 +0100","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190226172016.qzjuqchijjf7w4xn@uno.localdomain>","References":"<20190226021857.28255-1-niklas.soderlund@ragnatech.se>\n\t<20190226021857.28255-8-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"o5to42xcvcsxs4gw\"","Content-Disposition":"inline","In-Reply-To":"<20190226021857.28255-8-niklas.soderlund@ragnatech.se>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 7/8] libcamera: camera: ensure streams\n\tbelong to camera","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, 26 Feb 2019 17:19:48 -0000"}},{"id":925,"web_url":"https://patchwork.libcamera.org/comment/925/","msgid":"<20190227164953.GH4813@pendragon.ideasonboard.com>","date":"2019-02-27T16:49:53","subject":"Re: [libcamera-devel] [PATCH 7/8] libcamera: camera: ensure streams\n\tbelong to camera","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Tue, Feb 26, 2019 at 03:18:56AM +0100, Niklas Söderlund wrote:\n> Before calling into the pipeline handler make sure the streams provided\n> by the application actually belongs to the camera.\n> \n> Signed-off-by: Niklas Söderlund \n> ---\n> src/libcamera/camera.cpp | 8 ++++++++\n> 1 file changed, 8 insertions(+)\n> \n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 938f21fe80ef8ceb..ba8638009992170f 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -253,6 +253,10 @@ Camera::streamConfiguration(std::set &streams)\n> \tif (!stateIsAtleast(Free) || !streams.size())\n> \t\treturn std::map{};\n> \n> +\tfor (Stream *stream : streams)\n> +\t\tif (streams_.find(stream) == streams_.end())\n> +\t\t\treturn std::map{};\n> +\n\nCould we have braces for the for statement ?\n\nI'm beginning to wonder if the function shouldn't return an error value\nto differentiate between the differents errors, and take the std::map as\nan output parameter.\n\nI also wonder if this check is worth it. Do you foresee this problem as\ncommon, or even possible without the application going through great\nefforts to make this happen ?\n\n> \treturn pipe_->streamConfiguration(this, streams);\n> }\n> \n> @@ -290,6 +294,10 @@ int Camera::configureStreams(std::map &config)\n> \t\treturn -EINVAL;\n> \t}\n> \n> +\tfor (auto const &iter : config)\n> +\t\tif (streams_.find(iter.first) == streams_.end())\n> +\t\t\treturn -EINVAL;\n> +\n\nBraces here too please.\n\n> \tret = pipe_->configureStreams(this, config);\n> \tif (ret)\n> \t\treturn ret;","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 4A5A7610B3\n\tfor ;\n\tWed, 27 Feb 2019 17:50:09 +0100 (CET)","from pendragon.ideasonboard.com (unknown [83.145.195.18])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 49AA549;\n\tWed, 27 Feb 2019 17:50:06 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1551286208;\n\tbh=LW6xlESwcFMac5JGKcpj62dNMmGMaG3na3AGgb2Zxpo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=sGHfM/ZOz6POaZoJjQeD8067sOCwQZKvDZXV6GdQhAEaR4rlWIfL/hSm2uRnQO1Tf\n\t7yyiTlRMp1eTusUa9kU43ILDfxlJLjtwVP0yYXknamNSjFS8zjbiVAcejkgFpza6N8\n\tblIcPzc7ZhEWm4XV2JarBh56hfFDDGX3asqXt7JQ=","Date":"Wed, 27 Feb 2019 18:49:53 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190227164953.GH4813@pendragon.ideasonboard.com>","References":"<20190226021857.28255-1-niklas.soderlund@ragnatech.se>\n\t<20190226021857.28255-8-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190226021857.28255-8-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 7/8] libcamera: camera: ensure streams\n\tbelong to camera","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":"Wed, 27 Feb 2019 16:50:09 -0000"}},{"id":939,"web_url":"https://patchwork.libcamera.org/comment/939/","msgid":"<20190228015550.GX899@bigcity.dyn.berto.se>","date":"2019-02-28T01:55:50","subject":"Re: [libcamera-devel] [PATCH 7/8] libcamera: camera: ensure streams\n\tbelong to camera","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your feedback.\n\nOn 2019-02-27 18:49:53 +0200, Laurent Pinchart wrote:\n> Hi Niklas,\n> \n> Thank you for the patch.\n> \n> On Tue, Feb 26, 2019 at 03:18:56AM +0100, Niklas Söderlund wrote:\n> > Before calling into the pipeline handler make sure the streams provided\n> > by the application actually belongs to the camera.\n> > \n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > src/libcamera/camera.cpp | 8 ++++++++\n> > 1 file changed, 8 insertions(+)\n> > \n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index 938f21fe80ef8ceb..ba8638009992170f 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -253,6 +253,10 @@ Camera::streamConfiguration(std::set &streams)\n> > \tif (!stateIsAtleast(Free) || !streams.size())\n> > \t\treturn std::map{};\n> > \n> > +\tfor (Stream *stream : streams)\n> > +\t\tif (streams_.find(stream) == streams_.end())\n> > +\t\t\treturn std::map{};\n> > +\n> \n> Could we have braces for the for statement ?\n\nOf course.\n\n> \n> I'm beginning to wonder if the function shouldn't return an error value\n> to differentiate between the differents errors, and take the std::map as\n> an output parameter.\n> \n> I also wonder if this check is worth it. Do you foresee this problem as\n> common, or even possible without the application going through great\n> efforts to make this happen ?\n\nI'm not sure, we discussed this in Brussels and I recall we thought this \nwas a good idea at the time. The rational was to guarantee that pipeline \nhandlers wound not be tempted to implement this check on there own. I \nwill keep this for next version but put it at the end so it can be \ndropped if we choose it's not needed.\n\n> \n> > \treturn pipe_->streamConfiguration(this, streams);\n> > }\n> > \n> > @@ -290,6 +294,10 @@ int Camera::configureStreams(std::map &config)\n> > \t\treturn -EINVAL;\n> > \t}\n> > \n> > +\tfor (auto const &iter : config)\n> > +\t\tif (streams_.find(iter.first) == streams_.end())\n> > +\t\t\treturn -EINVAL;\n> > +\n> \n> Braces here too please.\n> \n> > \tret = pipe_->configureStreams(this, config);\n> > \tif (ret)\n> > \t\treturn ret;\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from mail-lj1-x235.google.com (mail-lj1-x235.google.com\n\t[IPv6:2a00:1450:4864:20::235])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6E85E610B2\n\tfor ;\n\tThu, 28 Feb 2019 02:55:53 +0100 (CET)","by mail-lj1-x235.google.com with SMTP id z25so15661429ljk.8\n\tfor ;\n\tWed, 27 Feb 2019 17:55:53 -0800 (PST)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\th12sm3349421lji.63.2019.02.27.17.55.51\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tWed, 27 Feb 2019 17:55:51 -0800 (PST)"],"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=unRr3ovgfwLrTiQufMleXRHoJIucAV80OdtiV8cmvyo=;\n\tb=Vewr/av+Od7c3eRnTgkiizTPk6cCMzdEA0z1FZBJj+A4cRQ58um/NRUjKkaZr/hj9x\n\tcY22ENavyWPgkxuKpWhtYoyvQMEHZ3WKNmWHWsVbcxmm4sNCskjDiUBWG7wlp0VzXxjk\n\tJhPXFqw53NvgPJel62X7/m/LeesjvNkJ6ciDbm12++nCJVZvZABcZtGMC+aTHb+28AQR\n\tGeUST5TrmF0Kcf/x77nDHsvlncl3C6CJjKqWP1ZAKNmM54QWsiAHoNcVzw9lLJSNlgEX\n\tFfgcoQAQGBa2pTEJuB6hmw02ClhDkdeHDSqCYYOPvSjErcQ9EWD+L9N/mUnppJoN/GIv\n\t7EPQ==","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=unRr3ovgfwLrTiQufMleXRHoJIucAV80OdtiV8cmvyo=;\n\tb=m0NTezaVNVqKdh6L2rG3Q9c/OrYsB5Jci+PuURtyJwZvUj2sImuMdDYXbC16GieF9C\n\tko09EhF+amX26I0QoNOT3XHMz0U/G1HYF/a/ifXZJSG7nJ/9NQnZuuM4Si/McdMkpbIr\n\tljCwnQkjZMsU+7XeKukgORtDJFhW/XOp1RGFa+XqQGnoIe41cHlNpjg5hOWbgyAccMQJ\n\tiQUATMUOGgqhC+TIzOvxWCNn0ZqT3bBdIodMBqS0n8au/vOzH6qP3tZ9a5+VoEwlCM6Q\n\t8hXXkvbn2eydFHwNjr7SgdhAp9Eub/pERbEIVGYx5USC7MiHvxTXjE+L00qpr0pAeKc6\n\tD1Ew==","X-Gm-Message-State":"APjAAAUzlVVUCNQGnmg+8/M/EHNYDmImJy05USk5YqDr5j+cuMxGw5ap\n\tSBkXz21eDzr2x1HZ9j8lZH6SfS14x0g=","X-Google-Smtp-Source":"APXvYqwRTQ34yyIDK3Mn7jFCsaM4KSvCNem9CrsszlbUvhNbTILhGUA7oh2sRZ3WkhULOudgOHUk4A==","X-Received":"by 2002:a2e:4c11:: with SMTP id\n\tz17mr3101385lja.134.1551318952514; \n\tWed, 27 Feb 2019 17:55:52 -0800 (PST)","Date":"Thu, 28 Feb 2019 02:55:50 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190228015550.GX899@bigcity.dyn.berto.se>","References":"<20190226021857.28255-1-niklas.soderlund@ragnatech.se>\n\t<20190226021857.28255-8-niklas.soderlund@ragnatech.se>\n\t<20190227164953.GH4813@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":"<20190227164953.GH4813@pendragon.ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 7/8] libcamera: camera: ensure streams\n\tbelong to camera","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, 28 Feb 2019 01:55:53 -0000"}},{"id":955,"web_url":"https://patchwork.libcamera.org/comment/955/","msgid":"<20190228171913.GB7811@pendragon.ideasonboard.com>","date":"2019-02-28T17:19:13","subject":"Re: [libcamera-devel] [PATCH 7/8] libcamera: camera: ensure streams\n\tbelong to camera","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nOn Thu, Feb 28, 2019 at 02:55:50AM +0100, Niklas Söderlund wrote:\n> On 2019-02-27 18:49:53 +0200, Laurent Pinchart wrote:\n> > On Tue, Feb 26, 2019 at 03:18:56AM +0100, Niklas Söderlund wrote:\n> > > Before calling into the pipeline handler make sure the streams provided\n> > > by the application actually belongs to the camera.\n> > > \n> > > Signed-off-by: Niklas Söderlund \n> > > ---\n> > > src/libcamera/camera.cpp | 8 ++++++++\n> > > 1 file changed, 8 insertions(+)\n> > > \n> > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > index 938f21fe80ef8ceb..ba8638009992170f 100644\n> > > --- a/src/libcamera/camera.cpp\n> > > +++ b/src/libcamera/camera.cpp\n> > > @@ -253,6 +253,10 @@ Camera::streamConfiguration(std::set &streams)\n> > > \tif (!stateIsAtleast(Free) || !streams.size())\n> > > \t\treturn std::map{};\n> > > \n> > > +\tfor (Stream *stream : streams)\n> > > +\t\tif (streams_.find(stream) == streams_.end())\n> > > +\t\t\treturn std::map{};\n> > > +\n> > \n> > Could we have braces for the for statement ?\n> \n> Of course.\n> \n> > I'm beginning to wonder if the function shouldn't return an error value\n> > to differentiate between the differents errors, and take the std::map as\n> > an output parameter.\n> > \n> > I also wonder if this check is worth it. Do you foresee this problem as\n> > common, or even possible without the application going through great\n> > efforts to make this happen ?\n> \n> I'm not sure, we discussed this in Brussels and I recall we thought this \n> was a good idea at the time. The rational was to guarantee that pipeline \n> handlers wound not be tempted to implement this check on there own. I \n> will keep this for next version but put it at the end so it can be \n> dropped if we choose it's not needed.\n\nSorry, I should have replied to this e-mail before 4/4 of v2. I fully\nagree that pipeline handlers should not implement this on their own, but\nI still wonder if application can still get this wrong in a plausible\nway.\n\n> > > \treturn pipe_->streamConfiguration(this, streams);\n> > > }\n> > > \n> > > @@ -290,6 +294,10 @@ int Camera::configureStreams(std::map &config)\n> > > \t\treturn -EINVAL;\n> > > \t}\n> > > \n> > > +\tfor (auto const &iter : config)\n> > > +\t\tif (streams_.find(iter.first) == streams_.end())\n> > > +\t\t\treturn -EINVAL;\n> > > +\n> > \n> > Braces here too please.\n> > \n> > > \tret = pipe_->configureStreams(this, config);\n> > > \tif (ret)\n> > > \t\treturn ret;","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 5B7D2610B6\n\tfor ;\n\tThu, 28 Feb 2019 18:19:19 +0100 (CET)","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 C056D49;\n\tThu, 28 Feb 2019 18:19:18 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1551374359;\n\tbh=IXLHDTFvjC2h2Zz4wDHYGKWxGabw1Kgr6i/+kYXMCMk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ZXrnNDENNPg1sofK7SmufGXSnGFTwX0f7Mk0RNa6KFlfW/IUD+zQG0jUQ2ZCFLwYL\n\tlSzQGWmm5q4isTGUKZM1PSU172bpC/Qft1Ooho2ZTGkUS5+m312jU8acJToCZjQALu\n\tbm32z83xdeSaAM0RaKP72uYNoKxlqW2ZdN5LVo1s=","Date":"Thu, 28 Feb 2019 19:19:13 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190228171913.GB7811@pendragon.ideasonboard.com>","References":"<20190226021857.28255-1-niklas.soderlund@ragnatech.se>\n\t<20190226021857.28255-8-niklas.soderlund@ragnatech.se>\n\t<20190227164953.GH4813@pendragon.ideasonboard.com>\n\t<20190228015550.GX899@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":"<20190228015550.GX899@bigcity.dyn.berto.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 7/8] libcamera: camera: ensure streams\n\tbelong to camera","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, 28 Feb 2019 17:19:19 -0000"}}]