[libvirt] PATCH: 9/12: HAL/DevitKit node device impl

[David Lively]

On Fri, 2008-11-14 at 13:41 +0000, Mark McLoughlin wrote:
> The DeviceKit implementation seems to be much more of a work-in-progress
> than the HAL implementation - maybe disable it by default in configure?
> (i.e. with_devkit=no instead of with_devkit=check)

That's a good idea.  I don't think the devkit impl can go forward until
devkit itself starts moving forward.  (Hmmm ... I (now) see the devkit
mailing list has started getting some traffic.  Apparently they've just
released v2.  Perhaps it's time for another look, though I can't sign up
for that right now.)

> > +    interface = strrchr(sysfs_path, '/');
> > +    if (!interface && *interface && *(++interface))
> > +        return -1;
> 
> Was this meant to be:
> 
>   if (!interface || !*interface || !*(++interface))

Yes.  Or the equivalent (! (interface && *interface && *(++interface)).


> > +static void dev_create(void *_dkdev, void *_dkclient ATTRIBUTE_UNUSED)
> > +{
...
> > +
> > +    dev->privateData = dkdev;
> 
> You need need a privateFree() hook here to do g_object_unref(), I think

Yes.  Good catch, again.

> > +    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);
> 
> Need a g_list_free() here right? 

Yes ...

> > +    }
> > +
> > +    driverState->privateData = devkit_client;
> > +
> > +    // TODO: Register to get DeviceKit events on device changes and
> > +    //       coordinate updates with queries and other operations.
> 
> That's a pretty big missing feature - if both HAL and DevKit were
> available on a system, we'd still want HAL until this is fixed, right?

Absolutely.  I think this supports your request that this be disabled by
default.

> > +    (void)get_int_prop(ctx, udi, "pci.vendor_id", (int *)&d->pci_dev.vendor);
> > +    if (get_str_prop(ctx, udi, "pci.vendor", &d->pci_dev.vendor_name) != 0)
> > +        (void)get_str_prop(ctx, udi, "info.vendor", &d->pci_dev.vendor_name);
> > +    (void)get_int_prop(ctx, udi, "pci.product_id", (int *)&d->pci_dev.product);
> > +    if (get_str_prop(ctx, udi, "pci.product", &d->pci_dev.product_name) != 0)
> > +        (void)get_str_prop(ctx, udi, "info.product", &d->pci_dev.product_name);
> 
> By the way - vendor and product IDs are normally quoted in hex, not
> decimal - e.g. I'd know my NIC is 8086:10de, not 32902:4318
> 
> I guess most other integer values in libvirt XML are decimal, but might
> be worth adding a hex format for this?

I'd prefer hex for vid/pid as well; I just stuck with decimal since the
rest of libvirt does.  If this does get changed to output hex, I'd only
request that we prefix hex numbers with "0x" so people don't have to
remember which attrs are dumped in hex and which in decimal.


> > +static void device_prop_modified(LibHalContext *ctx ATTRIBUTE_UNUSED,
> > +                                 const char *udi,
> > +                                 const char *key,
> > +                                 dbus_bool_t is_removed ATTRIBUTE_UNUSED,
> > +                                 dbus_bool_t is_added ATTRIBUTE_UNUSED)
> > +{
> > +    const char *name = hal_name(udi);
> > +    virNodeDeviceObjPtr dev = virNodeDeviceFindByName(&driverState->devs,name);
> > +    DEBUG("%s %s", key, name);
> > +    if (dev) {
> > +        /* Simply "rediscover" device -- incrementally handling changes
> > +         * to properties (which are mapped into caps in very capability-
> > +         * specific ways) is nasty ... so avoid it.
> > +         */
> > +        virNodeDeviceObjRemove(&driverState->devs, dev);
> > +        dev_create(strdup(udi));
> > +    } else
> > +        DEBUG("no device named %s", name);
> > +}
> 
> Could use the same callback for both of these (with a cast), or simplify
> them to:
> 
> static void device_prop_modified(LibHalContext *ctx ATTRIBUTE_UNUSED,
>                                  const char *udi,
>                                  const char *key,
>                                  dbus_bool_t is_removed ATTRIBUTE_UNUSED,
>                                  dbus_bool_t is_added ATTRIBUTE_UNUSED)
> {
>     DEBUG("%s %s", key, name);
> 
>     /* Simply "rediscover" device -- incrementally handling changes
>      * to properties (which are mapped into caps in very capability-
>      * specific ways) is nasty ... so avoid it.
>      */
>     device_removed(ctx, udi);
>     device_added(ctx, udi);
> }

Yep, that's a good idea.

Thanks for the careful review,
Dave