[{"id":2644,"web_url":"https://patchwork.libcamera.org/comment/2644/","msgid":"","date":"2019-09-13T08:50:23","subject":"Re: [libcamera-devel] [PATCH 3/4] libcamera:\n\tdevice_enumerator_udev: Avoid double list lookup","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 12/09/2019 21:03, Laurent Pinchart wrote:\n> DeviceEnumeratorUdev::populateMediaDevice() searches for orphan devices\n> in an std::list, and if found removes them using std::list::remove().\n> This ends up looking up the entry twice. Replace the remove() call with\n> erase() to fix it.\n> \n> Signed-off-by: Laurent Pinchart \n> ---\n> src/libcamera/device_enumerator_udev.cpp | 5 +++--\n> 1 file changed, 3 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/libcamera/device_enumerator_udev.cpp b/src/libcamera/device_enumerator_udev.cpp\n> index 210f5c1f2870..c40770911d3d 100644\n> --- a/src/libcamera/device_enumerator_udev.cpp\n> +++ b/src/libcamera/device_enumerator_udev.cpp\n> @@ -182,7 +182,8 @@ int DeviceEnumeratorUdev::populateMediaDevice(const std::shared_ptr\n> \t\t\t\t entity->deviceMinor());\n> \n> \t\t/* Take device from orphan list first, if it is in the list. */\n> -\t\tif (std::find(orphans_.begin(), orphans_.end(), devnum) != orphans_.end()) {\n> +\t\tauto orphan = std::find(orphans_.begin(), orphans_.end(), devnum);\n> +\t\tif (orphan != orphans_.end()) {\n> \t\t\tstd::string deviceNode = lookupDeviceNode(devnum);\n> \t\t\tif (deviceNode.empty())\n> \t\t\t\treturn -EINVAL;\n> @@ -191,7 +192,7 @@ int DeviceEnumeratorUdev::populateMediaDevice(const std::shared_ptr\n> \t\t\tif (ret)\n> \t\t\t\treturn ret;\n> \n> -\t\t\torphans_.remove(devnum);\n> +\t\t\torphans_.erase(orphan);\n\nOuch, I had to look up the difference between remove and erase. That's a\nsubtle but important difference :D And I'm sure no one ever gets bitten\nby the fact that .remove() does not 'remove' an entry from a container :-D\n\n\nReviewed-by: Kieran Bingham \n\n> \t\t\tcontinue;\n> \t\t}\n> \n>","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E142E60BB0\n\tfor ;\n\tFri, 13 Sep 2019 10:50:26 +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 4617A33A;\n\tFri, 13 Sep 2019 10:50:26 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1568364626;\n\tbh=02JJb/II54Te7Qv7dUfPZwxJyH24ugMWN1oNknnTILc=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=Z76WtfyE65z3T9im2B2K6M0tBOdMiCU5KpEksJRxZj1lQyD4r/Xq5+thMtdj8MGd8\n\tQJu3NyKP5Ho82HTx/20cGlqs01C7SEsFNCrOR1ezc2SgCcMhgLDcyHZZgZTr/a//xF\n\tOOyg5zjYw52TTYZ2liTw2mcUUxHQiRSt3p3NBCJk=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20190912200330.19004-1-laurent.pinchart@ideasonboard.com>\n\t<20190912200330.19004-4-laurent.pinchart@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\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCJQQYAQoADwIbDAUCWcOUawUJ\n\tB4D+AgAKCRChHkZyEKRh/XJhEACr5iidt/0MZ0rWRMCbZFMWD7D2g6nZeOp+F2zY8CEUW+sd\n\tCDVd9BH9QX9KN5SZo6YtJzMzSzpcx45VwTvtQW0n/6Eujg9EUqblfU9xqvqDmbjEapr5d/OL\n\t21GTALb0owKhA5qDUGEcKGCphpQffKhTNo/BP99jvmJUj7IPSKH97qPypi8/ym8bAxB+uY31\n\tgHTMHf1jMJJ1pRo2tYYPeIIHGDqXBI4sp5GHHF+JcIhgR/e/A6w/dgzHYmQPl2ix5eZYEZbV\n\tTRP+gkX4NV8oHqa/lR+xPOlWElGB57viOSOoWriqxQbFy8XbG1GR8cWlkNtGBGVWaJaSoORP\n\tiowD7irXL91bCyFIqL+7BVk3Jy4uzP744PzE80KwxOp5SQAp9sPzFbgsJrLev90PZySjFHG0\n\twP144DK7nBjOj/J0g9OHVASP1JjK+nw7SDoKnETDIdRC0XmiHXk7TXzPdkvO0UkpHdEPjZUp\n\tWyuc0MqehjR/hTTPt4m/Y14XzEcy6JREIjOrFfUZVho2QpOdv9CNryGdieRTNjUtz463CIaZ\n\tdPBiw9mOMBoNffkn9FIoCjLnAaj9gUAnEHWBZOEviQ5NuyqpeP0YtzI4iaRbSUkYZHej99X3\n\tVmHrdLlMqd/ZgYYbPGSL4AN3FVACb5CxuxEHwo029VcE5U3CSjzqtCoX12tm7A==","Organization":"Ideas on Board","Message-ID":"","Date":"Fri, 13 Sep 2019 09:50:23 +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":"<20190912200330.19004-4-laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH 3/4] libcamera:\n\tdevice_enumerator_udev: Avoid double list lookup","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, 13 Sep 2019 08:50:27 -0000"}},{"id":2647,"web_url":"https://patchwork.libcamera.org/comment/2647/","msgid":"<20190913091955.qbaxn4qlblspkuh4@uno.localdomain>","date":"2019-09-13T09:19:55","subject":"Re: [libcamera-devel] [PATCH 3/4] libcamera:\n\tdevice_enumerator_udev: Avoid double list lookup","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Kieran,\n\nOn Fri, Sep 13, 2019 at 09:50:23AM +0100, Kieran Bingham wrote:\n> Hi Laurent,\n>\n> On 12/09/2019 21:03, Laurent Pinchart wrote:\n> > DeviceEnumeratorUdev::populateMediaDevice() searches for orphan devices\n> > in an std::list, and if found removes them using std::list::remove().\n> > This ends up looking up the entry twice. Replace the remove() call with\n> > erase() to fix it.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > src/libcamera/device_enumerator_udev.cpp | 5 +++--\n> > 1 file changed, 3 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/src/libcamera/device_enumerator_udev.cpp b/src/libcamera/device_enumerator_udev.cpp\n> > index 210f5c1f2870..c40770911d3d 100644\n> > --- a/src/libcamera/device_enumerator_udev.cpp\n> > +++ b/src/libcamera/device_enumerator_udev.cpp\n> > @@ -182,7 +182,8 @@ int DeviceEnumeratorUdev::populateMediaDevice(const std::shared_ptr\n> > \t\t\t\t entity->deviceMinor());\n> >\n> > \t\t/* Take device from orphan list first, if it is in the list. */\n> > -\t\tif (std::find(orphans_.begin(), orphans_.end(), devnum) != orphans_.end()) {\n> > +\t\tauto orphan = std::find(orphans_.begin(), orphans_.end(), devnum);\n> > +\t\tif (orphan != orphans_.end()) {\n> > \t\t\tstd::string deviceNode = lookupDeviceNode(devnum);\n> > \t\t\tif (deviceNode.empty())\n> > \t\t\t\treturn -EINVAL;\n> > @@ -191,7 +192,7 @@ int DeviceEnumeratorUdev::populateMediaDevice(const std::shared_ptr\n> > \t\t\tif (ret)\n> > \t\t\t\treturn ret;\n> >\n> > -\t\t\torphans_.remove(devnum);\n> > +\t\t\torphans_.erase(orphan);\n>\n> Ouch, I had to look up the difference between remove and erase. That's a\n> subtle but important difference :D And I'm sure no one ever gets bitten\n> by the fact that .remove() does not 'remove' an entry from a container :-D\n\nWhat difference are you referring to? My understanding is that this\npatch just saves one lookup by saving the result of std::find() in the\norphan variable, so that it can be reused at erase time.\n\nAm I missing something?\n\n>\n>\n> Reviewed-by: Kieran Bingham \n>\n> > \t\t\tcontinue;\n> > \t\t}\n> >\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 relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[217.70.183.193])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C14EC60BB0\n\tfor ;\n\tFri, 13 Sep 2019 11:18:22 +0200 (CEST)","from uno.localdomain (bl10-204-24.dsl.telepac.pt [85.243.204.24])\n\t(Authenticated sender: jacopo@jmondi.org)\n\tby relay1-d.mail.gandi.net (Postfix) with ESMTPSA id CAAD3240004;\n\tFri, 13 Sep 2019 09:18:21 +0000 (UTC)"],"X-Originating-IP":"85.243.204.24","Date":"Fri, 13 Sep 2019 11:19:55 +0200","From":"Jacopo Mondi ","To":"Kieran Bingham ","Cc":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20190913091955.qbaxn4qlblspkuh4@uno.localdomain>","References":"<20190912200330.19004-1-laurent.pinchart@ideasonboard.com>\n\t<20190912200330.19004-4-laurent.pinchart@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"k4w6iwbpx5vdtiwu\"","Content-Disposition":"inline","In-Reply-To":"","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 3/4] libcamera:\n\tdevice_enumerator_udev: Avoid double list lookup","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, 13 Sep 2019 09:18:22 -0000"}},{"id":2648,"web_url":"https://patchwork.libcamera.org/comment/2648/","msgid":"<20190913092225.GE29992@pendragon.ideasonboard.com>","date":"2019-09-13T09:22:25","subject":"Re: [libcamera-devel] [PATCH 3/4] libcamera:\n\tdevice_enumerator_udev: Avoid double list lookup","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Fri, Sep 13, 2019 at 11:19:55AM +0200, Jacopo Mondi wrote:\n> On Fri, Sep 13, 2019 at 09:50:23AM +0100, Kieran Bingham wrote:\n> > On 12/09/2019 21:03, Laurent Pinchart wrote:\n> > > DeviceEnumeratorUdev::populateMediaDevice() searches for orphan devices\n> > > in an std::list, and if found removes them using std::list::remove().\n> > > This ends up looking up the entry twice. Replace the remove() call with\n> > > erase() to fix it.\n> > >\n> > > Signed-off-by: Laurent Pinchart \n> > > ---\n> > > src/libcamera/device_enumerator_udev.cpp | 5 +++--\n> > > 1 file changed, 3 insertions(+), 2 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/device_enumerator_udev.cpp b/src/libcamera/device_enumerator_udev.cpp\n> > > index 210f5c1f2870..c40770911d3d 100644\n> > > --- a/src/libcamera/device_enumerator_udev.cpp\n> > > +++ b/src/libcamera/device_enumerator_udev.cpp\n> > > @@ -182,7 +182,8 @@ int DeviceEnumeratorUdev::populateMediaDevice(const std::shared_ptr\n> > > \t\t\t\t entity->deviceMinor());\n> > >\n> > > \t\t/* Take device from orphan list first, if it is in the list. */\n> > > -\t\tif (std::find(orphans_.begin(), orphans_.end(), devnum) != orphans_.end()) {\n> > > +\t\tauto orphan = std::find(orphans_.begin(), orphans_.end(), devnum);\n> > > +\t\tif (orphan != orphans_.end()) {\n> > > \t\t\tstd::string deviceNode = lookupDeviceNode(devnum);\n> > > \t\t\tif (deviceNode.empty())\n> > > \t\t\t\treturn -EINVAL;\n> > > @@ -191,7 +192,7 @@ int DeviceEnumeratorUdev::populateMediaDevice(const std::shared_ptr\n> > > \t\t\tif (ret)\n> > > \t\t\t\treturn ret;\n> > >\n> > > -\t\t\torphans_.remove(devnum);\n> > > +\t\t\torphans_.erase(orphan);\n> >\n> > Ouch, I had to look up the difference between remove and erase. That's a\n> > subtle but important difference :D And I'm sure no one ever gets bitten\n> > by the fact that .remove() does not 'remove' an entry from a container :-D\n> \n> What difference are you referring to? My understanding is that this\n> patch just saves one lookup by saving the result of std::find() in the\n> orphan variable, so that it can be reused at erase time.\n> \n> Am I missing something?\n\nerase() removes the element referenced by an iterator. remove() removes\nall elements whose value is identical to the passed value. As all\nelements in the list should have different values the effect of both\nfunctions should be identical, but erase() is O(1) while remove() is\nO(n).\n\n> > Reviewed-by: Kieran Bingham \n> >\n> > > \t\t\tcontinue;\n> > > \t\t}\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 C7EF260BB0\n\tfor ;\n\tFri, 13 Sep 2019 11:22:32 +0200 (CEST)","from pendragon.ideasonboard.com (bl10-204-24.dsl.telepac.pt\n\t[85.243.204.24])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 156BD33A;\n\tFri, 13 Sep 2019 11:22:31 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1568366552;\n\tbh=9xYBdEJawo1GYB3aPmmz8to6BSbiAwKpHJ0kCd6P7hQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=XFkLjQBzGB4Y0hPFx7TXSIPkJxqhiUnNDafZHLS9OpTGWCv5aYOhnWb+djEgbhu6k\n\thH3RzU9yNYiUu2xdbyp7RYbkNVcTzO+npyMsDW0SvAYJKASyJqmrl9AyF1ENm5Q44P\n\tPMD1BXK9NMJEqc7mm9fDUjI/VDSDrO5Y9J9oENrE=","Date":"Fri, 13 Sep 2019 12:22:25 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20190913092225.GE29992@pendragon.ideasonboard.com>","References":"<20190912200330.19004-1-laurent.pinchart@ideasonboard.com>\n\t<20190912200330.19004-4-laurent.pinchart@ideasonboard.com>\n\t\n\t<20190913091955.qbaxn4qlblspkuh4@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190913091955.qbaxn4qlblspkuh4@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 3/4] libcamera:\n\tdevice_enumerator_udev: Avoid double list lookup","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, 13 Sep 2019 09:22:33 -0000"}}]