7:41 a.m.
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.