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