[libvirt] udev node device backend
Dave Allan
dallan at redhat.com
Thu Nov 12 21:11:08 UTC 2009
Daniel P. Berrange wrote:
> 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 at 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.
Removed entirely.
>> + 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.
I thought it might be used to describe the actual system at some point,
but it's not important. Removed.
>> 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 ?
Removed.
>> >From ecb4c2c2a0e42ed5f7578441b5290980663c549c Mon Sep 17 00:00:00 2001
>> From: David Allan <dallan at 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
Done.
>> +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
Fixed.
>> 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
Fixed in the later patch reflecting Cole's feedback; I've rolled those
changes into the main patch.
> <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
Good point--done.
>> +#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.
Fixed.
>> 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 ?
I'm not sure what's up with this--I'm fairly certain that I was losing
devices from the tree view of virsh nodedev-list without it, but I'm
currently seeing everything I should be, so I'm clearly losing my mind.
:) In any case, I've reverted this change and everything seems to be
working ok.
>> @@ -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.
Removed from the XML.
> 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, _
Good idea. Done, and added the additional bit of disambiguation of the
udev device name so devices have the form:
net_eth0_54_52_00_02_c4_46
Updated patch set attached.
Dave
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0001-Add-several-fields-to-node-device-capabilities.patch
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20091112/a76ef58d/attachment-0005.ksh>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0002-Implement-a-node-device-backend-using-libudev.patch
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20091112/a76ef58d/attachment-0006.ksh>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0003-Add-scsi_target-device-type.patch
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20091112/a76ef58d/attachment-0007.ksh>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0004-Remove-DevKit-node-device-backend.patch
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20091112/a76ef58d/attachment-0008.ksh>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0005-Add-translation-of-PCI-vendor-and-product-IDs.patch
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20091112/a76ef58d/attachment-0009.ksh>
More information about the libvir-list
mailing list