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.