[{"id":11777,"web_url":"https://patchwork.libcamera.org/comment/11777/","msgid":"<20200802211910.GG23801@pendragon.ideasonboard.com>","date":"2020-08-02T21:19:10","subject":"Re: [libcamera-devel] [PATCH v5 5/5] libcamera: camera_manager:\n\tEnforce unique camera names","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 Wed, Jul 29, 2020 at 01:50:55PM +0200, Niklas Söderlund wrote:\n> The camera name have always been documented that it should be unique but\n> it has never been enforced. Change this by refusing to add cameras to\n> the CameraManager that would create two cameras with the exact same\n> name.\n> \n> Signed-off-by: Niklas Söderlund \n> Reviewed-by: Jacopo Mondi \n> Reviewed-by: Kieran Bingham \n> ---\n> * Changes since v4\n> - Update string in error message.\n> \n> * Changes since v3\n> - Update commit message.\n> ---\n> src/libcamera/camera_manager.cpp | 6 +++---\n> 1 file changed, 3 insertions(+), 3 deletions(-)\n> \n> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp\n> index f60491d2c1a7500f..47e0f3129d346183 100644\n> --- a/src/libcamera/camera_manager.cpp\n> +++ b/src/libcamera/camera_manager.cpp\n> @@ -178,10 +178,10 @@ void CameraManager::Private::addCamera(std::shared_ptr camera,\n> \n> \tfor (std::shared_ptr c : cameras_) {\n> \t\tif (c->name() == camera->name()) {\n> -\t\t\tLOG(Camera, Warning)\n> -\t\t\t\t<< \"Registering camera with duplicate name '\"\n> +\t\t\tLOG(Camera, Error)\n> +\t\t\t\t<< \"Skip registration of a camera with a duplicated name '\"\n> \t\t\t\t<< camera->name() << \"'\";\n> -\t\t\tbreak;\n> +\t\t\treturn;\n\nShouldn't the error be propagated up to the pipelien handler ? Other\nthan that, this looks good to me, but I think we need to rename \"name\"\nto \"id\".\n\n> \t\t}\n> \t}\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 3E0B3BD86F\n\tfor ;\n\tSun, 2 Aug 2020 21:19:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CBB3E60999;\n\tSun, 2 Aug 2020 23:19:22 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 79BB560393\n\tfor ;\n\tSun, 2 Aug 2020 23:19:21 +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 E374E296;\n\tSun, 2 Aug 2020 23:19:20 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"av64GcOf\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1596403161;\n\tbh=Qm7kWskZjAjsweRpK/1Y7/2z6TfEkBNo6xLEoukj8RQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=av64GcOfRy5kJXRWcumbcnhDp/b8q/IMDndFF920yUtrNcX1vYgiDdLsr37BXtb7G\n\tut6+/Tzk2dzRdf/9ipu8dXYAMB08rLIDIX36VLos3lqBDxlLSV8ordutjNJVTVRn+r\n\tliWnLE6f1saww3GpgJxgqQfr2W5GiFquDhQUTVT4=","Date":"Mon, 3 Aug 2020 00:19:10 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20200802211910.GG23801@pendragon.ideasonboard.com>","References":"<20200729115055.3840110-1-niklas.soderlund@ragnatech.se>\n\t<20200729115055.3840110-6-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200729115055.3840110-6-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH v5 5/5] libcamera: camera_manager:\n\tEnforce unique camera names","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11782,"web_url":"https://patchwork.libcamera.org/comment/11782/","msgid":"<20200802231116.GC1945056@oden.dyn.berto.se>","date":"2020-08-02T23:11:16","subject":"Re: [libcamera-devel] [PATCH v5 5/5] libcamera: camera_manager:\n\tEnforce unique camera names","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 comments.\n\nOn 2020-08-03 00:19:10 +0300, Laurent Pinchart wrote:\n> Hi Niklas,\n> \n> Thank you for the patch.\n> \n> On Wed, Jul 29, 2020 at 01:50:55PM +0200, Niklas Söderlund wrote:\n> > The camera name have always been documented that it should be unique but\n> > it has never been enforced. Change this by refusing to add cameras to\n> > the CameraManager that would create two cameras with the exact same\n> > name.\n> > \n> > Signed-off-by: Niklas Söderlund \n> > Reviewed-by: Jacopo Mondi \n> > Reviewed-by: Kieran Bingham \n> > ---\n> > * Changes since v4\n> > - Update string in error message.\n> > \n> > * Changes since v3\n> > - Update commit message.\n> > ---\n> > src/libcamera/camera_manager.cpp | 6 +++---\n> > 1 file changed, 3 insertions(+), 3 deletions(-)\n> > \n> > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp\n> > index f60491d2c1a7500f..47e0f3129d346183 100644\n> > --- a/src/libcamera/camera_manager.cpp\n> > +++ b/src/libcamera/camera_manager.cpp\n> > @@ -178,10 +178,10 @@ void CameraManager::Private::addCamera(std::shared_ptr camera,\n> > \n> > \tfor (std::shared_ptr c : cameras_) {\n> > \t\tif (c->name() == camera->name()) {\n> > -\t\t\tLOG(Camera, Warning)\n> > -\t\t\t\t<< \"Registering camera with duplicate name '\"\n> > +\t\t\tLOG(Camera, Error)\n> > +\t\t\t\t<< \"Skip registration of a camera with a duplicated name '\"\n> > \t\t\t\t<< camera->name() << \"'\";\n> > -\t\t\tbreak;\n> > +\t\t\treturn;\n> \n> Shouldn't the error be propagated up to the pipelien handler ? Other\n> than that, this looks good to me, but I think we need to rename \"name\"\n> to \"id\".\n\nI'm not sure it's useful to propagate the error to the pipeline handler, \nwhat will it do with it? The pipeline can't change the ID and try adding \nthe camera again, as this would violate the requirements we have on IDs\nbeing stable and unique. All it can do is print yet another LOG message \nsaying a duplicated ID is found.\n\nI originally had this as a Fatal error as this really should not happen, \nI'm tempted to switch it back. What do you think?\n\n> \n> > \t\t}\n> > \t}\n> > \n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 0B52FBD87A\n\tfor ;\n\tSun, 2 Aug 2020 23:11:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9DF1C603F7;\n\tMon, 3 Aug 2020 01:11:20 +0200 (CEST)","from mail-lf1-x143.google.com (mail-lf1-x143.google.com\n\t[IPv6:2a00:1450:4864:20::143])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 950CD60398\n\tfor ;\n\tMon, 3 Aug 2020 01:11:18 +0200 (CEST)","by mail-lf1-x143.google.com with SMTP id x24so2187256lfe.11\n\tfor ;\n\tSun, 02 Aug 2020 16:11:18 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\to23sm4045080lfr.67.2020.08.02.16.11.17\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tSun, 02 Aug 2020 16:11:17 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"jI156WHQ\"; dkim-atps=neutral","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\tbh=y5t3HoRdxUkywxLGzKc7rWdscdG+lbFEW6SySFajjwc=;\n\tb=jI156WHQr7U67HOAfwpU7qxOqGMCbOq8kAe67GG7TTccrHvVPpkhPvNLC+GluFBJeZ\n\tlpeMJCbMUj/bJRwvV/NJlv8Uze0sany+HIcCeqNeHpYON3k1f5D+5G2ogVExV4HNQan5\n\tYLBrC50UEePe1rKF5+h7EzX9yCN+F3GZiYMBY68bOd5kCSHSDkFRr7f6rMeVwcFLTgtW\n\ttkG2mu4ZPe9YhXoE5Ici8YTAFBDc6nSi19rW+QkyYGLG8rHrQCbc1zZ1VCHBuX31xM5d\n\tfgaYeEOsTP5+CwVDiYAhEk00vSLbGSZojWJ2dfAL9Z6ZSqeErodblVINbttYnzW/xJ8O\n\tE2hA==","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;\n\tbh=y5t3HoRdxUkywxLGzKc7rWdscdG+lbFEW6SySFajjwc=;\n\tb=sxqxgujBRNSKkrUpk4a//x7vfWiCCeB/BGmeTQsfQNQ816C8hS0P74CKbeVYz3/mX5\n\tuHBT/A19XlVPmANHFmTOHmA4zl69Lw76m9yylguf0YxPRTyvUiSTo8Vs5gg8ek3QK/YE\n\tCQAD/qr8Ra6cCxEd7aKyuGehHzhGaSw4wNx69SnBmcY9P7z/fk6cxsWRaND9DZ3dfYRK\n\tVS/VVMN6Y2MK5bt1mChiX65E710nn00dxL3RnkUQ5Cro7Wvj3eSIuiwfsuUS/lMq6teV\n\twtJVXexZvVJa0w+FjGFsDDti0BQJJ2DA4ctJuQ7HotyTlZ8MphRUcFw3oAOmiR8vev7y\n\tVUZA==","X-Gm-Message-State":"AOAM530icsj5sJVHHfi5iPQCfi6iobml4oHZcCKv2zNZnisqtCUANOG/\n\tKVBR15/L7FT494Yz10cG7CBbyw==","X-Google-Smtp-Source":"ABdhPJy6D0vbodbHaf967WpkG++LTZIPKZA3Kc5E7oIZbe/k9tVWptC0pleqps8EV+je739b/qpFMw==","X-Received":"by 2002:a19:428c:: with SMTP id\n\tp134mr6995390lfa.70.1596409877896; \n\tSun, 02 Aug 2020 16:11:17 -0700 (PDT)","Date":"Mon, 3 Aug 2020 01:11:16 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Message-ID":"<20200802231116.GC1945056@oden.dyn.berto.se>","References":"<20200729115055.3840110-1-niklas.soderlund@ragnatech.se>\n\t<20200729115055.3840110-6-niklas.soderlund@ragnatech.se>\n\t<20200802211910.GG23801@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200802211910.GG23801@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v5 5/5] libcamera: camera_manager:\n\tEnforce unique camera names","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11783,"web_url":"https://patchwork.libcamera.org/comment/11783/","msgid":"<20200802231644.GJ23801@pendragon.ideasonboard.com>","date":"2020-08-02T23:16:44","subject":"Re: [libcamera-devel] [PATCH v5 5/5] libcamera: camera_manager:\n\tEnforce unique camera names","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nOn Mon, Aug 03, 2020 at 01:11:16AM +0200, Niklas Söderlund wrote:\n> On 2020-08-03 00:19:10 +0300, Laurent Pinchart wrote:\n> > On Wed, Jul 29, 2020 at 01:50:55PM +0200, Niklas Söderlund wrote:\n> > > The camera name have always been documented that it should be unique but\n> > > it has never been enforced. Change this by refusing to add cameras to\n> > > the CameraManager that would create two cameras with the exact same\n> > > name.\n> > > \n> > > Signed-off-by: Niklas Söderlund \n> > > Reviewed-by: Jacopo Mondi \n> > > Reviewed-by: Kieran Bingham \n> > > ---\n> > > * Changes since v4\n> > > - Update string in error message.\n> > > \n> > > * Changes since v3\n> > > - Update commit message.\n> > > ---\n> > > src/libcamera/camera_manager.cpp | 6 +++---\n> > > 1 file changed, 3 insertions(+), 3 deletions(-)\n> > > \n> > > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp\n> > > index f60491d2c1a7500f..47e0f3129d346183 100644\n> > > --- a/src/libcamera/camera_manager.cpp\n> > > +++ b/src/libcamera/camera_manager.cpp\n> > > @@ -178,10 +178,10 @@ void CameraManager::Private::addCamera(std::shared_ptr camera,\n> > > \n> > > \tfor (std::shared_ptr c : cameras_) {\n> > > \t\tif (c->name() == camera->name()) {\n> > > -\t\t\tLOG(Camera, Warning)\n> > > -\t\t\t\t<< \"Registering camera with duplicate name '\"\n> > > +\t\t\tLOG(Camera, Error)\n> > > +\t\t\t\t<< \"Skip registration of a camera with a duplicated name '\"\n> > > \t\t\t\t<< camera->name() << \"'\";\n> > > -\t\t\tbreak;\n> > > +\t\t\treturn;\n> > \n> > Shouldn't the error be propagated up to the pipelien handler ? Other\n> > than that, this looks good to me, but I think we need to rename \"name\"\n> > to \"id\".\n> \n> I'm not sure it's useful to propagate the error to the pipeline handler, \n> what will it do with it? The pipeline can't change the ID and try adding \n> the camera again, as this would violate the requirements we have on IDs\n> being stable and unique. All it can do is print yet another LOG message \n> saying a duplicated ID is found.\n\nI'm fine not propagating the error as long as we can ensure that no\nmemory leak or bad memory access will occur. The pipeline handler will\nthink registration succeeded, and will return true from match(). Can you\ncheck that nothing can crash ?\n\n> I originally had this as a Fatal error as this really should not happen, \n> I'm tempted to switch it back. What do you think?\n\nI think it could make sense, yes.\n\n> > > \t\t}\n> > > \t}\n> > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id CD254BD86F\n\tfor ;\n\tSun, 2 Aug 2020 23:16:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 65C4A60398;\n\tMon, 3 Aug 2020 01:16:57 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 438A860398\n\tfor ;\n\tMon, 3 Aug 2020 01:16:56 +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 ACBE7296;\n\tMon, 3 Aug 2020 01:16:55 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Vp0HZa21\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1596410215;\n\tbh=R5TsT38/4OkLwgMCDe34IqJJvJ35Vuufyc9+VJUrhJU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Vp0HZa21ElvqSKqjNgpiLLMsntyOIrepj2iVm5tkXnce60w4Tg/8E2tQz9I8q62aS\n\tVVy2Zows0AcOlbAd/Vh3PFP5InU0YFY3VeYvTVeuw4rxXg+LSTEY+GTON8GFbE/ZKC\n\tOgS3r1zPbsPVEPj5SHSdVD0gV1vzQtTlRCSv4Nh8=","Date":"Mon, 3 Aug 2020 02:16:44 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20200802231644.GJ23801@pendragon.ideasonboard.com>","References":"<20200729115055.3840110-1-niklas.soderlund@ragnatech.se>\n\t<20200729115055.3840110-6-niklas.soderlund@ragnatech.se>\n\t<20200802211910.GG23801@pendragon.ideasonboard.com>\n\t<20200802231116.GC1945056@oden.dyn.berto.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200802231116.GC1945056@oden.dyn.berto.se>","Subject":"Re: [libcamera-devel] [PATCH v5 5/5] libcamera: camera_manager:\n\tEnforce unique camera names","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]