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