[libcamera-devel] [RFC PATCH v1] libcamera: device_enumerator_udev: remove devnum from dependency map

[Laurent Pinchart]

Hello Barnabás,

On Mon, Apr 17, 2023 at 05:41:58PM +0000, Barnabás Pőcze via libcamera-devel wrote:
> 2023. április 17., hétfő 8:30 keltezéssel, Jacopo Mondi írta:
> > [...]
> > > diff --git a/src/libcamera/device_enumerator_udev.cpp b/src/libcamera/device_enumerator_udev.cpp
> > > index 5317afbd..a63cd360 100644
> > > --- a/src/libcamera/device_enumerator_udev.cpp
> > > +++ b/src/libcamera/device_enumerator_udev.cpp
> > > @@ -315,6 +315,7 @@ int DeviceEnumeratorUdev::addV4L2Device(dev_t devnum)
> > >  	 * enumerator.
> > >  	 */
> > >  	deps->deps_.erase(devnum);
> > > +	devMap_.erase(it);
> > 
> > Shouldn't this be done only when all the dependencies are actually
> > satisfied ?
> 
> Every dependency has its own entry in `devMap_` with a pointer to
> the corresponding `MediaDeviceDeps` object. I think when one dependency
> appears it should be removed from the map, just like it is removed
> from `MediaDeviceDeps::deps_` (in the previous line).
> 
> > >  	if (deps->deps_.empty()) {
> > 
> > Likely inside this if() clause ?
> 
> `deps->deps_` is empty at this point, so we cannot efficiently find all
> entries in `devMap_` that belong to `deps`.

We could iterate over the whole map to remove all entries that point to
deps, but I don't think that's needed.

> > Otherwise, if there still are dependencies to meet for the devnum at
> > hand, the next time addV4L2Device(devnum) is called it won't find any item
> > on the devMap_ for that key.
> 
> My reading of the code suggests that devnum is unique and each v4l2 device
> can be a dependency of at most 1 media device. Is that not correct?

Your assumption is correct. The only issue I can see would be if the
V4L2 device is removed and re-added without the media device being
removed too. addV4L2Device() wouldn't find the dependency in that case,
but this is a completely unsupported use case that would cause other
problems anyway. I think this patch is fine.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> > >  		LOG(DeviceEnumerator, Debug)

-- 
Regards,

Laurent Pinchart