[libcamera-devel] [PATCH 3/4] libcamera: device_enumerator_udev: Avoid double list lookup

[Jacopo Mondi]

Hi Kieran,

On Fri, Sep 13, 2019 at 09:50:23AM +0100, Kieran Bingham wrote:
> Hi Laurent,
>
> On 12/09/2019 21:03, Laurent Pinchart wrote:
> > DeviceEnumeratorUdev::populateMediaDevice() searches for orphan devices
> > in an std::list, and if found removes them using std::list::remove().
> > This ends up looking up the entry twice. Replace the remove() call with
> > erase() to fix it.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  src/libcamera/device_enumerator_udev.cpp | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/libcamera/device_enumerator_udev.cpp b/src/libcamera/device_enumerator_udev.cpp
> > index 210f5c1f2870..c40770911d3d 100644
> > --- a/src/libcamera/device_enumerator_udev.cpp
> > +++ b/src/libcamera/device_enumerator_udev.cpp
> > @@ -182,7 +182,8 @@ int DeviceEnumeratorUdev::populateMediaDevice(const std::shared_ptr<MediaDevice>
> >  				       entity->deviceMinor());
> >
> >  		/* Take device from orphan list first, if it is in the list. */
> > -		if (std::find(orphans_.begin(), orphans_.end(), devnum) != orphans_.end()) {
> > +		auto orphan = std::find(orphans_.begin(), orphans_.end(), devnum);
> > +		if (orphan != orphans_.end()) {
> >  			std::string deviceNode = lookupDeviceNode(devnum);
> >  			if (deviceNode.empty())
> >  				return -EINVAL;
> > @@ -191,7 +192,7 @@ int DeviceEnumeratorUdev::populateMediaDevice(const std::shared_ptr<MediaDevice>
> >  			if (ret)
> >  				return ret;
> >
> > -			orphans_.remove(devnum);
> > +			orphans_.erase(orphan);
>
> Ouch, I had to look up the difference between remove and erase. That's a
> subtle but important difference :D And I'm sure no one ever gets bitten
> by the fact that .remove() does not 'remove' an entry from a container :-D

What difference are you referring to? My understanding is that this
patch just saves one lookup by saving the result of std::find() in the
orphan variable, so that it can be reused at erase time.

Am I missing something?

>
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> >  			continue;
> >  		}
> >
> >
>
> --
> Regards
> --
> Kieran
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190913/008d7d72/attachment.sig>