On Wed, Nov 11, 2009 at 05:06:18PM -0500, Dave Allan wrote:
From 94d99c19668d3c804c84ff878023b0f93560dc81 Mon Sep 17 00:00:00 2001 From: David Allan <dallan@redhat.com> Date: Fri, 16 Oct 2009 16:52:40 -0400 Subject: [PATCH 1/6] Add several fields to node device capabilities
--- src/conf/node_device_conf.c | 22 ++++++++++++++++++++++ src/conf/node_device_conf.h | 7 +++++++ 2 files changed, 29 insertions(+), 0 deletions(-)
diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index c5083cc..ece339f 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -248,6 +248,12 @@ char *virNodeDeviceDefFormat(virConnectPtr conn, if (data->system.product_name) virBufferEscapeString(&buf, " <product>%s</product>\n", data->system.product_name); + if (data->system.dmi_devpath) + virBufferEscapeString(&buf, " <dmi_devpath>%s</dmi_devpath>\n", + data->system.dmi_devpath);
I don't think we should be exposing this in the XML. It is a linux specific concepts. We expose some relevant bits of DMI data in the XML elsewhere, so would rather we added more data, than point clients to sysfs.
+ if (data->system.description) + virBufferEscapeString(&buf, " <description>%s</description>\n", + data->system.description);
I'm also not sure what this is useful for ? All it seems to output is <description>fictional device to root the node device tree</description> which is really just documentation about the schema, not something that needs to be included in actual document output.
virBufferAddLit(&buf, " <hardware>\n"); if (data->system.hardware.vendor_name) virBufferEscapeString(&buf, " <vendor>%s</vendor>\n", @@ -325,6 +331,9 @@ char *virNodeDeviceDefFormat(virConnectPtr conn, data->usb_if.subclass); virBufferVSprintf(&buf, " <protocol>%d</protocol>\n", data->usb_if.protocol); + if (data->usb_if.interface_name) + virBufferVSprintf(&buf, " <interface_name>%s</interface_name>\n", + data->usb_if.interface_name);
What are the semantics of this element ? On my system it comes out as <interface_name>9/0/0</interface_name> which is pretty much duplicating info already available in a structured format <class>9</class> <subclass>0</subclass> <protocol>0</protocol> So do we actually need to add this new element ?
From ecb4c2c2a0e42ed5f7578441b5290980663c549c Mon Sep 17 00:00:00 2001 From: David Allan <dallan@redhat.com> Date: Tue, 3 Nov 2009 21:16:51 -0500 Subject: [PATCH 2/6] Implement a node device backend using libudev.
Monitoring for addition and removal of devices is implemented.
There is a lot of detail work in this code, so we should try to get people running it on a wide variety of hardware so we can shake out the differences in implementation between the HAL and libudev backends.
I have moved the new fields in the node device capabilities to a separate patch.
This version contains changes per all the feedback I've received on earlier versions. ---
diff --git a/configure.in b/configure.in index 7ad1a90..4e5afef 100644 --- a/configure.in +++ b/configure.in @@ -1654,7 +1654,7 @@ test "$enable_shared" = no && lt_cv_objdir=. LV_LIBTOOL_OBJDIR=${lt_cv_objdir-.} AC_SUBST([LV_LIBTOOL_OBJDIR])
-dnl HAL or DeviceKit library for host device enumeration +dnl HAL, DeviceKit, or libudev library for host device enumeration HAL_REQUIRED=0.0 HAL_CFLAGS= HAL_LIBS= @@ -1748,8 +1748,46 @@ AM_CONDITIONAL([HAVE_DEVKIT], [test "x$with_devkit" = "xyes"]) AC_SUBST([DEVKIT_CFLAGS]) AC_SUBST([DEVKIT_LIBS])
+UDEV_REQUIRED=143
Already agreed on IRC that we should increase this to 145
+UDEV_CFLAGS= +UDEV_LIBS= +AC_ARG_WITH([udev], + [ --with-udev use libudev for host device enumeration], + [], + [with_udev=check]) + +if test "$with_libvirtd" = "no" ; then + with_udev=no +fi +if test "x$with_udev" = "xyes" -o "x$with_udev" = "xcheck"; then + PKG_CHECK_MODULES(UDEV, libudev >= $UDEV_REQUIRED, + [with_udev=yes], [ + if test "x$with_udev" = "xcheck" ; then + with_udev=no + else + AC_MSG_ERROR( + [You must install udev-devel >= $UDEV_REQUIRED to compile libvirt])
Typo here - libudev-devel
diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index ece339f..4d1bfda 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -91,6 +91,26 @@ int virNodeDeviceHasCap(const virNodeDeviceObjPtr dev, const char *cap) return 0; }
+ +virNodeDeviceObjPtr +virNodeDeviceFindByUdevName(const virNodeDeviceObjListPtr devs, + const char *udev_name) +{ + unsigned int i; + + for (i = 0; i < devs->count; i++) { + virNodeDeviceObjLock(devs->objs[i]); + if ((devs->objs[i]->def->udev_name != NULL) && + (STREQ(devs->objs[i]->def->udev_name, udev_name))) { + return devs->objs[i]; + } + virNodeDeviceObjUnlock(devs->objs[i]); + } + + return NULL; +} + + virNodeDeviceObjPtr virNodeDeviceFindByName(const virNodeDeviceObjListPtr devs, const char *name) { @@ -117,6 +137,8 @@ void virNodeDeviceDefFree(virNodeDeviceDefPtr def) VIR_FREE(def->name); VIR_FREE(def->parent); VIR_FREE(def->driver); + VIR_FREE(def->udev_name); + VIR_FREE(def->parent_udev_name);
caps = def->caps; while (caps) { @@ -228,9 +250,17 @@ char *virNodeDeviceDefFormat(virConnectPtr conn,
virBufferAddLit(&buf, "<device>\n"); virBufferEscapeString(&buf, " <name>%s</name>\n", def->name); - - if (def->parent) + if (def->udev_name != NULL) { + virBufferEscapeString(&buf, " <udev_name>%s</udev_name>\n", + def->udev_name); + } + if (def->parent) { virBufferEscapeString(&buf, " <parent>%s</parent>\n", def->parent); + } + if (def->parent_udev_name != NULL) { + virBufferEscapeString(&buf, " <parent_udev_name>%s</parent_udev_name>\n", + def->parent_udev_name); + } if (def->driver) { virBufferAddLit(&buf, " <driver>\n"); virBufferEscapeString(&buf, " <name>%s</name>\n", def->driver); diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h index f70184d..91ef94e 100644 --- a/src/conf/node_device_conf.h +++ b/src/conf/node_device_conf.h @@ -164,7 +164,9 @@ typedef struct _virNodeDeviceDef virNodeDeviceDef; typedef virNodeDeviceDef *virNodeDeviceDefPtr; struct _virNodeDeviceDef { char *name; /* device name (unique on node) */ + char *udev_name; /* udev name/sysfs path */ char *parent; /* optional parent device name */ + char *parent_udev_name; /* udev parent name/sysfs path */ char *driver; /* optional driver name */ virNodeDevCapsDefPtr caps; /* optional device capabilities */ };
+ +static int udevProcessStorage(struct udev_device *device, + virNodeDeviceDefPtr def) +{ + union _virNodeDevCapData *data = &def->caps->data; + int ret = -1; + + data->storage.block = strdup(udev_device_get_devnode(device)); + if (udevGetStringProperty(device, + "DEVNAME", + &data->storage.block) == PROPERTY_ERROR) { + goto out; + } + if (udevGetStringProperty(device, + "ID_BUS", + &data->storage.bus) == PROPERTY_ERROR) { + goto out; + } + if (udevGetStringProperty(device, + "ID_SERIAL", + &data->storage.serial) == PROPERTY_ERROR) { + goto out; + } + if (udevGetStringSysfsAttr(device, + "device/vendor", + &data->storage.vendor) == PROPERTY_ERROR) { + goto out; + } + udevStripSpaces(def->caps->data.storage.vendor); + if (udevGetStringSysfsAttr(device, + "device/model", + &data->storage.model) == PROPERTY_ERROR) { + goto out; + } + udevStripSpaces(def->caps->data.storage.model); + /* There is no equivalent of the hotpluggable property in libudev, + * but storage is going toward a world in which hotpluggable is + * expected, so I don't see a problem with not having a property + * for it. */ + + if (udevGetStringProperty(device, + "ID_TYPE", + &data->storage.drive_type) != PROPERTY_FOUND) { + /* If udev doesn't have it, perhaps we can guess it. */ + if (udevKludgeStorageType(def) != 0) { + goto out; + } + } + + /* NB: drive_type has changed from HAL; now it's "cd" instead of "cdrom" */ + if (STREQ(def->caps->data.storage.drive_type, "cd")) { + ret = udevProcessCDROM(device, def);
Is this comment still accurate ? It appears to show 'cdrom' for me when using udev <device> <name>block_sr0</name> <sysfs_path>/sys/devices/pci0000:00/0000:00:01.1/host1/target1:0:0/1:0:0:0/block/sr0</sysfs_path> <parent>scsi_1:0:0:0</parent> <parent_sysfs_path>/sys/devices/pci0000:00/0000:00:01.1/host1/target1:0:0/1:0:0:0</parent_sysfs_path> <capability type='storage'> <block>/dev/sr0</block> <bus>scsi</bus> <drive_type>cdrom</drive_type> <model>QEMU DVD-ROM</model> <vendor>QEMU</vendor> <capability type='removable'> <media_available>0</media_available> <media_size>0</media_size> <logical_block_size>0</logical_block_size> <num_blocks>0</num_blocks> </capability> </capability> </device>
+ +static int udevSetupSystemDev(void) +{ + virNodeDeviceDefPtr def = NULL; + virNodeDeviceObjPtr dev = NULL; + struct udev *udev = NULL; + struct udev_device *device = NULL; + union _virNodeDevCapData *data = NULL; + char *tmp = NULL; + int ret = -1; + + if (VIR_ALLOC(def) != 0) { + goto out; + } + + def->name = strdup("computer"); + if (def->name == NULL) { + goto out; + } + + if (VIR_ALLOC(def->caps) != 0) { + goto out; + } + + udev = udev_monitor_get_udev(DRV_STATE_UDEV_MONITOR(driverState)); + device = udev_device_new_from_syspath(udev, DMI_DEVPATH); + if (device == NULL) { + goto out; + } + + data = &def->caps->data; + + data->system.dmi_devpath = strdup(DMI_DEVPATH); + data->system.description = strdup(SYSTEM_DESCRIPTION); + + if (udevGetStringSysfsAttr(device, + "product_name", + &data->system.product_name) == PROPERTY_ERROR) { + goto out; + } + if (udevGetStringSysfsAttr(device, + "sys_vendor", + &data->system.hardware.vendor_name) + == PROPERTY_ERROR) { + goto out; + } + if (udevGetStringSysfsAttr(device, + "product_version", + &data->system.hardware.version) + == PROPERTY_ERROR) { + goto out; + } + if (udevGetStringSysfsAttr(device, + "product_serial", + &data->system.hardware.serial) + == PROPERTY_ERROR) { + goto out; + } + + if (udevGetStringSysfsAttr(device, + "product_uuid", + &tmp) == PROPERTY_ERROR) { + goto out; + }
The udevGetStringSysfsAttr() method is returning empty string for many of these, while HAL fills the fields with NULL. This causes the udev generated XML to included many emptry elements. <hardware> <vendor></vendor> <version></version> <serial></serial> <uuid>a6f7e16a-6e5e-a930-bca7-cc597167fab4</uuid> </hardware> So I think udevGetStringSysfsAttr() should convert "" into NULL
+#define virBuildPath(path, ...) virBuildPathInternal(path, __VA_ARGS__, NULL) +int virBuildPathInternal(char **path, ...) __attribute__ ((sentinel));
This should use ATTRIBUTE_SENTINAL, otherwise it won't compile with non-gcc, or older gcc.
diff --git a/tools/virsh.c b/tools/virsh.c index 0d0ebca..16b3f1c 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -5795,9 +5795,17 @@ cmdNodeListDevices (vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) char **parents = vshMalloc(ctl, sizeof(char *) * num_devices); for (i = 0; i < num_devices; i++) { virNodeDevicePtr dev = virNodeDeviceLookupByName(ctl->conn, devices[i]); + virNodeDevicePtr parent_dev = NULL; + if (dev && STRNEQ(devices[i], "computer")) { const char *parent = virNodeDeviceGetParent(dev); - parents[i] = parent ? strdup(parent) : NULL; + parent_dev = virNodeDeviceLookupByName(ctl->conn, parent); + if (parent_dev) { + parents[i] = strdup(parent); + } else { + parents[i] = strdup("computer"); + } + virNodeDeviceFree(parent_dev);
Why do we need lookup the actual device object here ? Is there really a time when we would report a parent which doesn't actually exist ?
@@ -251,16 +251,16 @@ char *virNodeDeviceDefFormat(virConnectPtr conn,
virBufferAddLit(&buf, "<device>\n"); virBufferEscapeString(&buf, " <name>%s</name>\n", def->name); - if (def->udev_name != NULL) { - virBufferEscapeString(&buf, " <udev_name>%s</udev_name>\n", - def->udev_name); + if (def->sysfs_path != NULL) { + virBufferEscapeString(&buf, " <sysfs_path>%s</sysfs_path>\n", + def->sysfs_path); } if (def->parent) { virBufferEscapeString(&buf, " <parent>%s</parent>\n", def->parent); } - if (def->parent_udev_name != NULL) { - virBufferEscapeString(&buf, " <parent_udev_name>%s</parent_udev_name>\n", - def->parent_udev_name); + if (def->parent_sysfs_path != NULL) { + virBufferEscapeString(&buf, " <parent_sysfs_path>%s</parent_sysfs_path>\n", + def->parent_sysfs_path); }
I'm not all that keen on us exposing an XML element named sysfs_path here, since again that's Linux specific concept, and if an app needed more metadata about a device then we ought to provide it directly, since most apps using libvirt run remotely & so can't access /sysfs aanyway. One final thought which doesn't really fit elsehwere. In the device names # virsh nodedev-list block_QEMU_HARDDISK_QM00001 block_sr0 computer net_54:52:00:39:ee:20 pci_0000:00:00.0 pci_0000:00:01.0 pci_0000:00:01.1 pci_0000:00:01.2 pci_0000:00:01.3 pci_0000:00:02.0 pci_0000:00:03.0 pci_0000:00:04.0 scsi_0:0:0:0 scsi_1:0:0:0 scsi_host0 scsi_host1 scsi_target0:0:0 scsi_target1:0:0 usb_1-0:1.0 usb_1-2 usb_1-2:1.0 usb_usb1 I think it would be worth getting rid of the punctuation characters, just doing a straight search & replace with '_', for anything which isn't in the set 0-9, a-Z, _ Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|