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.
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list