4:26 p.m.
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(a)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(a)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