[libvirt] udev node device backend

[Chris Lalancette]

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.

I haven't reviewed the code yet, but I did get this built and running on one of
my machines here.  I ran a script which collected the output of "virsh
nodedev-list" and "virsh nodedev-dumpxml <device>" against both the HAL backend
and the udev backend (the output from both is attached).  Then I did a
comparison.  Overall, it looks like you did really good work.  There are some
discrepancies, though, and a few random notes below.

1)  I did this on an F-11 x86_64 box, which has libudev 141 installed.  In order
for it to build, I had to change configure.in to allow 141, and I also had to
add #define LIBUDEV_I_KNOW_THE_API_IS_SUBJECT_TO_CHANGE.  Neither of these is an
issue for F-12, but it might be something to consider adding to the ./configure
checks so we can build it on slightly older setups.

2)  There are a few build-time warnings that you'll want to clean up:

  CC     libvirt_driver_nodedev_la-node_device_udev.lo
node_device/node_device_udev.c: In function 'udevGetUint64SysfsAttr':
node_device/node_device_udev.c:209: warning: passing argument 4 of
'virStrToLong_ull' from incompatible pointer type
../src/util/util.h:157: note: expected 'long long unsigned int *' but argument
is of type 'uint64_t *'
node_device/node_device_udev.c: In function 'udevProcessDisk':
node_device/node_device_udev.c:472: warning: passing argument 3 of
'udevGetUint64SysfsAttr' from incompatible pointer type
node_device/node_device_udev.c:199: note: expected 'uint64_t *' but argument is
of type 'long long unsigned int *'
node_device/node_device_udev.c:478: warning: passing argument 3 of
'udevGetUint64SysfsAttr' from incompatible pointer type
node_device/node_device_udev.c:199: note: expected 'uint64_t *' but argument is
of type 'long long unsigned int *'
node_device/node_device_udev.c: In function 'udevProcessCDROM':
node_device/node_device_udev.c:511: warning: passing argument 3 of
'udevGetUint64SysfsAttr' from incompatible pointer type
node_device/node_device_udev.c:199: note: expected 'uint64_t *' but argument is
of type 'long long unsigned int *'
node_device/node_device_udev.c:517: warning: passing argument 3 of
'udevGetUint64SysfsAttr' from incompatible pointer type
node_device/node_device_udev.c:199: note: expected 'uint64_t *' but argument is
of type 'long long unsigned int *'

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.

4)  I also took a look at the output for one of the bridges.  In my HAL backend,
I see:

<device>
  <name>net_00_13_20_f5_fa_e3_0</name>
  <parent>computer</parent>
  <capability type='net'>
    <interface>ovirtbr0</interface>
    <address>00:13:20:f5:fa:e3</address>
  </capability>
</device>

However, in the udev backend I am missing the parent link (in point of fact, the
parent link is missing for quite a few elements), and I also have an additional
"<capability type='80203'/>" element:

<device>
  <name>/sys/devices/virtual/net/ovirtbr0</name>
  <capability type='net'>
    <interface>ovirtbr0</interface>
    <address>00:13:20:f5:fa:e3</address>
    <capability type='80203'/>
  </capability>
</device>

I'm not sure if either of those is a problem.

5)  We are still missing the mapping of product/vendor id --> names.  This shows
up for instance in the parent of the eth0 device, where the HAL backend shows:

 <product id='0x10bd'>82566DM-2 Gigabit Network Connection</product>

and the udev backend shows nothing.  Probably not a show-stopper, but a
nice-to-have for human readers.

6)  SCSI device 1:0:0:0 (pci_8086_2920_scsi_host_0_scsi_device_lun0 in the HAL
backend, /sys/devices/pci0000:00/0000:00:1f.2/host1/target1:0:0/1:0:0:0 in the
udev backend) shows up with "<type>cdrom</type>" in HAL, but not in udev.

There are probably a few others, please take a look through my hal-output and
udev-output.  Like I said, very good work overall, this is just nit-picking and
cleanups that will make it more like the HAL backend.

-- 
Chris Lalancette
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: hal-output
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20091028/0c981714/attachment-0002.ksh>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: udev-output
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20091028/0c981714/attachment-0003.ksh>