3:46 p.m.
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)
> 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.
The missing parent link is a problem - if all else fails, it should at
least point back to the top level 'computer' device. In this case you
are correct that <capability type='80203'/> is bogus, since this is
a bridge device, not an physical ethernet device.
Udev has an odd characteristic which is that it often sets the parent
name to a string that is not the name of a device that it recognizes,
i.e., you have a parent name, but looking it up returns nothing. I have
been setting the parent pointer to the string provided by udev, and then
setting the parent string to computer when I do things like dump the
tree if the parent device doesn't actually exist. As I think about it,
and the related device naming point that you raise below, the more I
think parent should be a libvirt concept, set as you describe: if the
parent device is NULL or is not a device known to libvirt, set the
parent to computer. I'll create a separate attribute for the udev
parent string.
> 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.
Yeah, we shouldn't hold this up just for missing human names - unique IDs
are the important thing. Even HAL could miss the names if the PCI IDs database
was not up2date.
Agreed. I looked at the ids file and didn't relish the idea of parsing
it myself, and the vendor names are not available through udev. Is
libpciaccess a viable option? It has functions that will do what we
want, but I haven't worked with it, so I don't know if it is suitable
for use in libvirt.
> 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.
The type=cdrom thing is a nice to have but not a show stopper. If we
commit without it, then just open a BZ so we remember to fix it some
time.
Hmm. I'll look into that.
> computer
> net_00_13_20_f5_fa_e3
> net_00_13_20_f5_fa_e3_0
> net_1e_85_27_2d_a9_b0
> net_computer_loopback
> pci_8086_10bd
> pci_8086_244e
> pci_8086_2910
> pci_8086_2920
> pci_8086_2920_scsi_host
> pci_8086_2920_scsi_host_0
> pci_8086_2920_scsi_host_0_scsi_device_lun0
> pci_8086_2920_scsi_host_0_scsi_host
> pci_8086_2920_scsi_host_scsi_device_lun0
> pci_8086_2920_scsi_host_scsi_host
> pci_8086_2926
> pci_8086_2926_scsi_host
> pci_8086_2926_scsi_host_0
> pci_8086_2930
> pci_8086_2934
> pci_8086_2935
> pci_8086_2936
> pci_8086_2937
> pci_8086_2938
> pci_8086_2939
> pci_8086_293a
> pci_8086_293c
> pci_8086_293e
> pci_8086_29c0
> pci_8086_29c2
> pci_8086_29c4
> pci_8086_29c6
> pci_8086_29c7
> platform_floppy_0_storage_platform_floppy
> storage_model_DVDRW_LH_20A1S
> storage_serial_SATA_ST3320620AS_9QF5E3AP
> usb_device_1d6b_1_0000_00_1a_0
> usb_device_1d6b_1_0000_00_1a_0_if0
> usb_device_1d6b_1_0000_00_1a_1
> usb_device_1d6b_1_0000_00_1a_1_if0
> usb_device_1d6b_1_0000_00_1a_2
> usb_device_1d6b_1_0000_00_1a_2_if0
> usb_device_1d6b_1_0000_00_1d_0
> usb_device_1d6b_1_0000_00_1d_0_if0
> usb_device_1d6b_1_0000_00_1d_1
> usb_device_1d6b_1_0000_00_1d_1_if0
> usb_device_1d6b_1_0000_00_1d_2
> usb_device_1d6b_1_0000_00_1d_2_if0
> usb_device_1d6b_2_0000_00_1a_7
> usb_device_1d6b_2_0000_00_1a_7_if0
> usb_device_1d6b_2_0000_00_1d_7
> usb_device_1d6b_2_0000_00_1d_7_if0
[snip]
> /sys/devices/pci0000:00/0000:00:00.0
> /sys/devices/pci0000:00/0000:00:02.0
> /sys/devices/pci0000:00/0000:00:03.0
> /sys/devices/pci0000:00/0000:00:03.2
> /sys/devices/pci0000:00/0000:00:03.3
> /sys/devices/pci0000:00/0000:00:19.0
> /sys/devices/pci0000:00/0000:00:19.0/net/eth0
> /sys/devices/pci0000:00/0000:00:1a.0/usb3
> /sys/devices/pci0000:00/0000:00:1a.0/usb3/3-0:1.0
> /sys/devices/pci0000:00/0000:00:1a.1/usb4
> /sys/devices/pci0000:00/0000:00:1a.1/usb4/4-0:1.0
> /sys/devices/pci0000:00/0000:00:1a.2/usb5
> /sys/devices/pci0000:00/0000:00:1a.2/usb5/5-0:1.0
> /sys/devices/pci0000:00/0000:00:1a.7/usb1
> /sys/devices/pci0000:00/0000:00:1a.7/usb1/1-0:1.0
> /sys/devices/pci0000:00/0000:00:1b.0
> /sys/devices/pci0000:00/0000:00:1d.0/usb6
> /sys/devices/pci0000:00/0000:00:1d.0/usb6/6-0:1.0
> /sys/devices/pci0000:00/0000:00:1d.1/usb7
> /sys/devices/pci0000:00/0000:00:1d.1/usb7/7-0:1.0
> /sys/devices/pci0000:00/0000:00:1d.2/usb8
> /sys/devices/pci0000:00/0000:00:1d.2/usb8/8-0:1.0
> /sys/devices/pci0000:00/0000:00:1d.7/usb2
> /sys/devices/pci0000:00/0000:00:1d.7/usb2/2-0:1.0
> /sys/devices/pci0000:00/0000:00:1e.0
> /sys/devices/pci0000:00/0000:00:1f.0
> /sys/devices/pci0000:00/0000:00:1f.2/host0
> /sys/devices/pci0000:00/0000:00:1f.2/host0/target0:0:0/0:0:0:0
> /sys/devices/pci0000:00/0000:00:1f.2/host0/target0:0:0/0:0:0:0/block/sda
> /sys/devices/pci0000:00/0000:00:1f.2/host1
> /sys/devices/pci0000:00/0000:00:1f.2/host1/target1:0:0/1:0:0:0
> /sys/devices/pci0000:00/0000:00:1f.2/host1/target1:0:0/1:0:0:0/block/sr0
> /sys/devices/pci0000:00/0000:00:1f.5
> /sys/devices/pci0000:00/0000:00:1f.5/host2
> /sys/devices/pci0000:00/0000:00:1f.5/host3
> /sys/devices/virtual/net/lo
> /sys/devices/virtual/net/ovirtbr0
> /sys/devices/virtual/net/virbr0
> computer
I don't like that we're using the sysfs paths for the name here.
I'd like to see non-path based names for devices. Either by
following the HAL naming, or defining our own naming scheme.
Mostly because if we use paths, then people will come to expect
that this field is a path and if we then change it to not be a path
apps will get confused/broken. If we did want to include a sysfs
path or equivalent, then we ought to have an explicit named XML
element for that.
That's a fair point. I don't like the paths-as-names, either; it's the
udev default, which is why I used it. As much as HAL *should* be
relatively static at this point, it's not guaranteed to be, so I think
trying to follow its naming convention will be a maintenance problem.
Defining our own naming scheme is a little work, definitely less work
than trying to figure out HAL's convention, and not tricky. I'll do so
& add an element for the sysfs path.
Dave