[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



On Thu, 2008-11-13 at 17:33 +0000, Daniel P. Berrange wrote:

> This is the main implementation of the local device enumation driver. The
> main change since David's patch is that we hav a single nodedevRegister()
> API call, instead of initializing HAL/DevKit separtely. This was needed
> to make the dlopen() support easier to implement. Functionally the end
> result is the same.

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)

> diff -r acac4fc31665 src/node_device.c
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/src/node_device.c	Thu Nov 13 17:10:30 2008 +0000
...
> +static int nodeListDevicesByCap(virConnectPtr conn,
> +                                const char *cap,
> +                                char **const names,
> +                                int maxnames,
> +                                unsigned int flags ATTRIBUTE_UNUSED)
> +{
> +    virDeviceMonitorStatePtr driver = conn->devMonPrivateData;
> +    int ndevs = 0;
> +    unsigned int i;
> +
> +    for (i = 0; i < driver->devs.count && ndevs < maxnames; i++)
> +        if (dev_has_cap(driver->devs.objs[i], cap))
> +            if ((names[ndevs++] = strdup(driver->devs.objs[i]->def->name)) == NULL)
> +                goto failure;

Over 80 columns here and would be easier to read if split up

> +
> +    return ndevs;
> +
> + failure:
> +    --ndevs;
> +    while (--ndevs >= 0)
> +        VIR_FREE(names[ndevs]);
> +    return -1;
> +}
> +
...
> diff -r acac4fc31665 src/node_device_devkit.c
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/src/node_device_devkit.c	Thu Nov 13 17:10:30 2008 +0000
...
> +static int gather_net_cap(DevkitDevice *dkdev,
> +                          union _virNodeDevCapData *d)
> +{
> +    const char *sysfs_path = devkit_device_get_native_path(dkdev);
> +    const char *interface;
> +
> +    if (sysfs_path == NULL)
> +        return -1;
> +    interface = strrchr(sysfs_path, '/');
> +    if (!interface && *interface && *(++interface))
> +        return -1;

Was this meant to be:

  if (!interface || !*interface || !*(++interface))

> +    if ((d->net.interface = strdup(interface)) == NULL)
> +        return -1;
> +
> +    d->net.subtype = VIR_NODE_DEV_CAP_NET_LAST;
> +
> +    return 0;
> +}
> +
> +
...
> +static void dev_create(void *_dkdev, void *_dkclient ATTRIBUTE_UNUSED)
> +{
> +    DevkitDevice *dkdev = _dkdev;
> +    const char *sysfs_path = devkit_device_get_native_path(dkdev);
> +    virNodeDeviceObjPtr dev = NULL;
> +    const char *name;
> +    int rv;
> +
> +    if (sysfs_path == NULL)
> +        /* Currently using basename(sysfs_path) as device name (key) */
> +        return;
> +
> +    name = strrchr(sysfs_path, '/');
> +    if (name == NULL)
> +        name = sysfs_path;
> +    else
> +        ++name;
> +
> +    if (VIR_ALLOC(dev) < 0 || VIR_ALLOC(dev->def) < 0)
> +        goto failure;
> +
> +    dev->privateData = dkdev;

You need need a privateFree() hook here to do g_object_unref(), I think

> +
> +    if ((dev->def->name = strdup(name)) == NULL)
> +        goto failure;
> +
> +    // TODO: Find device parent, if any
> +
> +    rv = gather_capabilities(dkdev, &dev->def->caps);
> +    if (rv != 0) goto failure;
> +
> +    if (VIR_REALLOC_N(driverState->devs.objs, driverState->devs.count + 1) < 0)
> +        goto failure;
> +
> +    driverState->devs.objs[driverState->devs.count++] = dev;

Add a virNodeDeviceObjAdd() function for this?

> +
> +    return;
> +
> + failure:
> +    DEBUG("FAILED TO ADD dev %s", name);
> +    if (dev)
> +        virNodeDeviceDefFree(dev->def);
> +    VIR_FREE(dev);
> +}
> +
> +
> +static int devkitDeviceMonitorStartup(void)
> +{
> +    size_t caps_tbl_len = sizeof(caps_tbl) / sizeof(caps_tbl[0]);
> +    DevkitClient *devkit_client = NULL;
> +    GError *err = NULL;
> +    GList *devs;
> +    int i;
> +
> +    /* Ensure caps_tbl is sorted by capability name */
> +    qsort(caps_tbl, caps_tbl_len, sizeof(caps_tbl[0]), cmpstringp);
> +
> +    if (VIR_ALLOC(driverState) < 0)
> +        return -1;
> +
> +    // TODO: Is it really ok to call this multiple times??
> +    //       Is there something analogous to call on close?
> +    g_type_init();

Yep, it's fine to call multiple times and there's no shutdown version.

> +
> +    /* Get new devkit_client and connect to daemon */
> +    devkit_client = devkit_client_new(NULL);
> +    if (devkit_client == NULL) {
> +        DEBUG0("devkit_client_new returned NULL");
> +        goto failure;
> +    }
> +    if (!devkit_client_connect(devkit_client, &err)) {
> +        DEBUG0("devkit_client_connect failed");
> +        goto failure;
> +    }
> +
> +    /* Populate with known devices.
> +     *
> +     * This really should be:
> +        devs = devkit_client_enumerate_by_subsystem(devkit_client, NULL, &err);
> +        if (err) {
> +            DEBUG0("devkit_client_enumerate_by_subsystem failed");
> +            devs = NULL;
> +            goto failure;
> +        }
> +        g_list_foreach(devs, dev_create, devkit_client);
> +    * but devkit_client_enumerate_by_subsystem currently fails when the second
> +    * arg is null (contrary to the API documentation).  So the following code
> +    * (from Dan B) works around this by listing devices per handled subsystem.
> +    */
> +
> +    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? 

> +    }
> +
> +    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?

> +
> +    return 0;
> +
> + failure:
> +    if (err) {
> +        DEBUG("\terror[%d]: %s", err->code, err->message);
> +        g_error_free(err);
> +    }
> +    if (devs) {
> +        g_list_foreach(devs, (GFunc)g_object_unref, NULL);
> +        g_list_free(devs);
> +    }
> +    if (devkit_client)
> +        g_object_unref(devkit_client);
> +    VIR_FREE(driverState);
> +
> +    return -1;
> +}
...
> diff -r acac4fc31665 src/node_device_hal.c
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/src/node_device_hal.c	Thu Nov 13 17:10:30 2008 +0000
...
> +static int gather_pci_cap(LibHalContext *ctx, const char *udi,
> +                          union _virNodeDevCapData *d)
> +{
> +    char *sysfs_path;
> +
> +    if (get_str_prop(ctx, udi, "pci.linux.sysfs_path", &sysfs_path) == 0) {
> +        char *p = strrchr(sysfs_path, '/');
> +        if (p) {
> +            (void)virStrToLong_ui(p+1, &p, 16, &d->pci_dev.domain);
> +            (void)virStrToLong_ui(p+1, &p, 16, &d->pci_dev.bus);
> +            (void)virStrToLong_ui(p+1, &p, 16, &d->pci_dev.slot);
> +            (void)virStrToLong_ui(p+1, &p, 16, &d->pci_dev.function);
> +        }
> +        VIR_FREE(sysfs_path);
> +    }
> +    (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?

> +    return 0;
> +}
...
> +static void device_cap_lost(LibHalContext *ctx ATTRIBUTE_UNUSED,
> +                            const char *udi,
> +                            const char *cap)
> +{
> +    const char *name = hal_name(udi);
> +    virNodeDeviceObjPtr dev = virNodeDeviceFindByName(&driverState->devs,name);
> +    DEBUG("%s %s", cap, name);
> +    if (dev) {
> +        /* Simply "rediscover" device -- incrementally handling changes
> +         * to sub-capabilities (like net.80203) is nasty ... so avoid it.
> +         */
> +        virNodeDeviceObjRemove(&driverState->devs, dev);
> +        dev_create(strdup(udi));
> +    } else
> +        DEBUG("no device named %s", name);
> +}
> +
> +
> +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);
}

Cheers,
Mark.


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]