Re: [libvirt] udev node device backend

Thursday, 12 November 2009

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(a)redhat.com&gt;
 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(a)redhat.com&gt;
 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 :|

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

2008

2007

2006

2005

Re: [libvirt] udev node device backend