[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] udev node device backend



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 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]