[libvirt] Re: [PATCH 0/6] host ("node") device enumeration, take two

[David Lively]

On Thu, 2008-10-23 at 13:53 +0100, Daniel P. Berrange wrote:
> On Tue, Oct 21, 2008 at 01:45:47PM -0400, David Lively wrote:
> >   * using newfangled array-based lists for NodeDeviceObj's
> 
> The individual capabilities within a device are still using a linked list,
> though that's not a critical problem from the thread safety point of view.

Right.  This was intentional, since there are no per-capability
operations (and hence no need for per-capability locking granularity).

> >   * subscribe to HAL events & update dev state from them (next for me?)

BTW, I'm working on this now.  Should have something to submit in
another day or two.

> >   * implement pci_bus/usb_bus capabilities (for PCI/USB controllers)
> 
> While on the subject of USB, I've just noticed that HAL seems to
> expose 2 devices for every USB device - 'usb' and 'usb_device',
> which we're mapping into just a 'usb' capability. Unless there's
> compelling reason to have both we might consider just filtering
> out one of the two.

Yeah, I meant to bring this up.  HAL uses "usb_device" for the USB
devices, and "usb" for the USB interface(s) provided by the device.  The
"usb_device" is the parent of the "usb" interface(s).  The HAL "usb"
namespace mirrors the attributes of the parent "usb_device", and adds a
few of its own, specific to the interface.  Right now I'm reporting both
mostly so the device hierarchy (as defined by the parent relation) is
consistent.

I see two choices:
(a) Just report the interface ("usb") objects and fixup their parent to
be the parent of their owning USB device.  Each interface object (as in
HAL) will reflect the details of the parent USB device, as well as the
interface-specific bits.
(b) Split libvirt's "usb" capability into "usb_device" and
"usb_interface".  A usb_interface will always have a usb_device as it's
parent (and I would *not* replicate the usb_device details in the
usb_interface - that's the whole reason for keeping it as a separate
device).

I'm leaning fairly strongly towards (b).  What do you all think?

> 
> >   * DeviceKit implementation is barely a proof of concept
> 
> Noticed one problem - on my build it refuses to enumerate devices
> if you pass in a NULL for subsystem name list. I made a really
> quick & dirty hack to just try it out
> 
> @@ -300,13 +301,18 @@ static int devkitNodeDriverStartup(void)
>      }
>  
>      /* Populate with known devices */
> -    devs = devkit_client_enumerate_by_subsystem(devkit_client, NULL, &err);
> -    if (err) {
> -        DEBUG0("devkit_client_enumerate_by_subsystem failed");
> -        devs = NULL;
> -        goto failure;
> +    for (i = 0 ; i < ARRAY_CARDINALITY(caps_tbl) ; i++) {
> +        const char *caps[] = { caps_tbl[i].cap_name, NULL };
> +        devs = devkit_client_enumerate_by_subsystem(devkit_client,
> +                                                    caps,
> +                                                    &err);
> +        if (err) {
> +            DEBUG0("devkit_client_enumerate_by_subsystem failed");
> +            devs = NULL;
> +            goto failure;
> +        }
> +        g_list_foreach(devs, dev_create, devkit_client);

Oh yeah.  I forgot I fixed devkit_client_enumerate_by_subsystem to work
as advertised with a NULL subsystem.  I submitted the fix a couple
months ago to devkit-devel at lists.freedesktop.org, but never got any
acknowledgment, and haven't seen any activity on the list whatsoever, so
I'm not surprised it hasn't made it in.  Any idea where I should submit
devkit fixes?

In any case, I'll put your workaround in for now.  It won't pick up
devices in subsystems not listed in caps_tbl, but those are fairly
useless devices anyway (since we won't recognize any device caps if we
don't explicitly handle that subsystem in caps_tbl).

>      }
> -    g_list_foreach(devs, dev_create, devkit_client);
>  
>      driverState->privateData = devkit_client;
>  
> 
> 
> >     * need to resolve naming & other issues (see
> > https://www.redhat.com/archives/libvir-list/2008-September/msg00430.html
> >     * ... and then fill in impl (no hurry; devkit immature now)
> 
> I'm still wondering if it is worth us santizing the device names ourselves
> somehow, though it might end up being rather a large job.  Will have to
> think some more about it.

Yeah, it's a nasty issue.  In a lot of ways, the HAL names are pretty
nice.  For example, I really like NICs named by MAC address (so knowing
that a particular NIC is "eth0" is *not* encoded in the device name, but
rather in a capability).  This is also much nicer than naming by (e.g.)
sysfs path, which encodes too much info about where the card is plugged
in (that's what parent info is for).

But I don't see any sign that Devkit is exposing HALish names, though
I'd imagine (if only to make HAL->Devkit transition easier) that they
will need to expose a device property like "HAL_UDI".  With no activity
on the devkit-devel list, I'm not sure where they're going.

Thanks for the comments.  I'll incorporate your devkit workaround, and
wait on further comment before submitting the next take.

Dave