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

Re: [libvirt] udev node device backend



Dave Allan wrote:
Chris Lalancette wrote:
Daniel P. Berrange wrote:
On Wed, Oct 28, 2009 at 12:16:40PM +0100, Chris Lalancette wrote:
Dave Allan wrote:
Attached is a fully functional version of the node device udev based backend, incorporating all the feedback from earlier revisions. I broke the new capability fields out into a separate patch per Dan's suggestion, and I have also included a patch removing the DevKit backend.
3) I took a look at how the network is represented in the XML. In the HAL
backend, we get something that looks like:

<device>
  <name>net_00_13_20_f5_fa_e3</name>
  <parent>pci_8086_10bd</parent>
  <capability type='net'>
    <interface>eth0</interface>
    <address>00:13:20:f5:fa:e3</address>
    <capability type='80203'/>
  </capability>
</device>

That "<capability type='80203'/>" looks to be bogus (although I could be wrong; that same XML is encoded into the tests, so maybe there is something else going on). You are already in a <capability> block, so that should probably just be
"<type='80203'/>".  The same problem occurs in the udev backend.
Why do you think the '<capability type='80203'/>' bit is bogus ? That looks correct to me, showing that eth0 is a ethernet device (as opposed to a 80211
wireless, or neither)

Oh, I think the concept is useful, it's just that the way it is represented in
the XML looks weird:

<capability type='net'>
    ...
    <capability type='80203'/>
</capability>

Shouldn't this rather be:

<capability type='net'>
    ...
    <type>80203</type>
</capability>

Or:

<capability type='net' subtype='80203'>
    ...
</capability>

Or something like that?


Here's the latest version of the udev backend. I think I've addressed all of everybody's concerns, except for the translation of PCI ids to strings and the bogosity in the ethernet types. I've got working code for the PCI ids translation, but it's completely separate and involves modifying the build system again, so I'd like to get this set of patches in first. I'll sort out the ethernet types in a follow up patch as well. There's some additional follow up work to make the device tree look really nice, but that's also completely separate.

Dave

Attached are two additional patches. The first fixes a bug I found where I was reading the wrong sysfs attribute name, so the PCI device ID wasn't getting populated. The second uses libpciaccess to translate the PCI vendor and device IDs into their human readable names.

Dave

>From 2d22b32055a25b03d25c0ab164fc654a503c2795 Mon Sep 17 00:00:00 2001
From: David Allan <dallan redhat com>
Date: Wed, 4 Nov 2009 03:06:05 -0500
Subject: [PATCH 1/2] Fix mis-named PCI device ID property

---
 src/node_device/node_device_udev.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
index 10d09fe..ed36da0 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -304,7 +304,7 @@ static int udevProcessPCI(struct udev_device *device,
         goto out;
     }

-    if (udevGetUintSysfsAttr(device, "product", &data->pci_dev.product, 0)
+    if (udevGetUintSysfsAttr(device, "device", &data->pci_dev.product, 0)
         == PROPERTY_ERROR) {
         goto out;
     }
-- 
1.6.5.1

>From c3bcc186cf76f7a3c717c4cb43e130b19bbd985f Mon Sep 17 00:00:00 2001
From: David Allan <dallan redhat com>
Date: Wed, 4 Nov 2009 03:05:30 -0500
Subject: [PATCH 2/2] Add translation of PCI vendor and product IDs

This patch uses libpciaccess to provide human readable names for PCI vendor and device IDs.
---
 configure.in                       |    9 +++++++
 src/Makefile.am                    |    4 +-
 src/node_device/node_device_udev.c |   45 ++++++++++++++++++++++++++++++++++-
 3 files changed, 54 insertions(+), 4 deletions(-)

diff --git a/configure.in b/configure.in
index 8254e94..b70e628 100644
--- a/configure.in
+++ b/configure.in
@@ -1727,10 +1727,19 @@ if test "x$with_udev" = "xyes" -o "x$with_udev" = "xcheck"; then
     CFLAGS="$old_CFLAGS"
     LDFLAGS="$old_LDFLAGS"
   fi
+  PCIACCESS_REQUIRED=0.10.0
+  PCIACCESS_CFLAGS=
+  PCIACCESS_LIBS=
+  PKG_CHECK_MODULES([PCIACCESS], [pciaccess >= $PCIACCESS_REQUIRED], [], [PCIACCESS_FOUND=no])
+  if test "$PCIACCESS_FOUND" = "no" ; then
+     AC_MSG_ERROR([You must install libpciaccess/libpciaccess-devel >= $PCIACCESS_REQUIRED to compile libvirt])
+   fi
 fi
 AM_CONDITIONAL([HAVE_UDEV], [test "x$with_udev" = "xyes"])
 AC_SUBST([UDEV_CFLAGS])
 AC_SUBST([UDEV_LIBS])
+AC_SUBST([PCIACCESS_CFLAGS])
+AC_SUBST([PCIACCESS_LIBS])

 with_nodedev=no;
 if test "$with_hal" = "yes" -o "$with_udev" = "yes";
diff --git a/src/Makefile.am b/src/Makefile.am
index 4459ad8..12fb2a9 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -644,8 +644,8 @@ libvirt_driver_nodedev_la_LDFLAGS += $(HAL_LIBS)
 endif
 if HAVE_UDEV
 libvirt_driver_nodedev_la_SOURCES += $(NODE_DEVICE_DRIVER_UDEV_SOURCES)
-libvirt_driver_nodedev_la_CFLAGS += $(UDEV_CFLAGS)
-libvirt_driver_nodedev_la_LDFLAGS += $(UDEV_LIBS)
+libvirt_driver_nodedev_la_CFLAGS += $(UDEV_CFLAGS) $(PCIACCESS_CFLAGS)
+libvirt_driver_nodedev_la_LDFLAGS += $(UDEV_LIBS) $(PCIACCESS_LIBS)
 endif

 if WITH_DRIVER_MODULES
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
index ed36da0..c749187 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -22,6 +22,7 @@

 #include <config.h>
 #include <libudev.h>
+#include <pciaccess.h>
 #include <scsi/scsi.h>

 #include "node_device_udev.h"
@@ -251,6 +252,42 @@ static void udevLogFunction(struct udev *udev ATTRIBUTE_UNUSED,
 }


+static int udevTranslatePCIIds(unsigned int vendor,
+                               unsigned int product,
+                               char **vendor_string,
+                               char **product_string)
+{
+    int ret = 0;
+    struct pci_id_match m;
+
+    ret = pci_system_init();
+    if (ret != 0) {
+        goto out;
+    }
+
+    m.vendor_id = vendor;
+    m.device_id = product;
+    m.subvendor_id = PCI_MATCH_ANY;
+    m.subdevice_id = PCI_MATCH_ANY;
+    m.device_class = 0;
+    m.device_class_mask = 0;
+    m.match_data = 0;
+
+    /* pci_get_strings returns void */
+    pci_get_strings(&m,
+                    (const char **)product_string,
+                    (const char **)vendor_string,
+                    NULL,
+                    NULL);
+
+    /* pci_system_cleanup returns void */
+    pci_system_cleanup();
+
+out:
+    return ret;
+}
+
+
 static int udevProcessPCI(struct udev_device *device,
                           virNodeDeviceDefPtr def)
 {
@@ -309,8 +346,12 @@ static int udevProcessPCI(struct udev_device *device,
         goto out;
     }

-    /* XXX FIXME: to do the vendor name and product name, we have to
-     * parse /usr/share/hwdata/pci.ids.  Use libpciaccess perhaps? */
+    if (udevTranslatePCIIds(data->pci_dev.vendor,
+                            data->pci_dev.product,
+                            &data->pci_dev.vendor_name,
+                            &data->pci_dev.product_name) != 0) {
+        goto out;
+    }

     if (udevGenerateDeviceName(device, def, NULL) != 0) {
         goto out;
-- 
1.6.5.1


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