Re: [libvirt] node device libudev backend second look

Dave Allan wrote: > Attached is a patch against the current head containing an > implementation of node device enumeration using libudev. It is complete > except for the monitor, but I'm submitting it now as I have a few > questions about the implementation that I'd like advice on. They are > marked XXX in comments in the patch. > > The other thing that's not clear to me is how the code generates the > tree structure for nodedev-list --tree. I'm setting the parent pointer > to what I think is correct, but the tree output is broken. I can dig > through it until I understand it, but if anyone is familiar with the > implementation and would be willing to take a few minutes to walk me > through it, it would save me a bunch of time. > > I think it's also important that people get the code installed on a > variety of systems as soon as possible to shake out the inevitable bugs > that will arise from differing device models and code versions, and I'll > have the final version with the monitor shortly. (very early feedback...I've only read it briefly and tried to compile it so far) <snip> > diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c > index f09f814..2322819 100644 > --- a/src/conf/node_device_conf.c > +++ b/src/conf/node_device_conf.c > @@ -325,6 +325,9 @@ char *virNodeDeviceDefFormat(virConnectPtr conn, > data->usb_if.subclass); > virBufferVSprintf(&buf, " <protocol>%d</protocol>\n", > data->usb_if.protocol); > + if (data->usb_if.interface_name) > + virBufferVSprintf(&buf, " <interface_name>%s</interface_name>\n", > + data->usb_if.interface_name); > if (data->usb_if.description) > virBufferVSprintf(&buf, " <description>%s</description>\n", > data->usb_if.description); > @@ -394,10 +397,19 @@ char *virNodeDeviceDefFormat(virConnectPtr conn, > "</media_available>\n", avl ? 1 : 0); > virBufferVSprintf(&buf, " <media_size>%llu</media_size>\n", > data->storage.removable_media_size); > - virBufferAddLit(&buf, " </capability>\n"); > + virBufferVSprintf(&buf, " <logical_block_size>%llu" > + "</logical_block_size>\n", > + data->storage.logical_block_size); > + virBufferVSprintf(&buf, " <num_blocks>%llu</num_blocks>\n", > + data->storage.num_blocks); This code is a bit difficult to read, but I think you still need the closing </capability> tag here. There's a high-level <capability> tag, but then this is sort of a sub-capability, and I think you need to close it off. > } else { > virBufferVSprintf(&buf, " <size>%llu</size>\n", > data->storage.size); > + virBufferVSprintf(&buf, " <logical_block_size>%llu" > + "</logical_block_size>\n", > + data->storage.logical_block_size); > + virBufferVSprintf(&buf, " <num_blocks>%llu</num_blocks>\n", > + data->storage.num_blocks); > } > if (data->storage.flags & VIR_NODE_DEV_CAP_STORAGE_HOTPLUGGABLE) > virBufferAddLit(&buf, > @@ -1239,6 +1251,7 @@ void virNodeDevCapsDefFree(virNodeDevCapsDefPtr caps) > VIR_FREE(data->usb_dev.vendor_name); > break; > case VIR_NODE_DEV_CAP_USB_INTERFACE: > + VIR_FREE(data->usb_if.interface_name); > VIR_FREE(data->usb_if.description); > break; > case VIR_NODE_DEV_CAP_NET: > diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h > index 29a4d43..95910c5 100644 > --- a/src/conf/node_device_conf.h > +++ b/src/conf/node_device_conf.h > @@ -101,6 +101,7 @@ struct _virNodeDevCapsDef { > unsigned function; > unsigned product; > unsigned vendor; > + unsigned class; > char *product_name; > char *vendor_name; > } pci_dev; > @@ -117,10 +118,12 @@ struct _virNodeDevCapsDef { > unsigned _class; /* "class" is reserved in C */ > unsigned subclass; > unsigned protocol; > + char *interface_name; > char *description; > } usb_if; > struct { > char *address; > + unsigned address_len; > char *ifname; > enum virNodeDevNetCapType subtype; /* LAST -> no subtype */ > } net; > @@ -139,6 +142,8 @@ struct _virNodeDevCapsDef { > } scsi; > struct { > unsigned long long size; > + unsigned long long num_blocks; > + unsigned long long logical_block_size; > unsigned long long removable_media_size; > char *block; > char *bus; > diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c > index 14b3098..f3bd45d 100644 > --- a/src/node_device/node_device_driver.c > +++ b/src/node_device/node_device_driver.c > @@ -70,9 +70,12 @@ static int update_caps(virNodeDeviceObjPtr dev) > } > > > -#ifdef __linux__ > -static int update_driver_name(virConnectPtr conn, > - virNodeDeviceObjPtr dev) > +#if defined (__linux__) && defined (HAVE_HAL) > +/* XXX Why does this function exist? Are there devices that change > + * their drivers while running? Under libudev, most devices seem to > + * provide their driver name as a property "DRIVER" */ > +static int update_driver_name_hal_linux(virConnectPtr conn, > + virNodeDeviceObjPtr dev) Oops, renaming this causes the build to fail. I've switched it back to "update_driver_name" for the time being. I'm also getting: CCLD libvirt_driver_network.la CCLD libvirt_driver_storage.la CC libvirt_driver_nodedev_la-node_device_driver.lo make[3]: *** No rule to make target `node_device/node_device_linux_sysfs.c', needed by `libvirt_driver_nodedev_la-node_device_linux_sysfs.lo'. Stop. make[3]: Leaving directory `/root/libvirt/src' While trying to compile. Any thoughts?

From ea9c2464be632ca6da80b8d35599b739e66bf1c8 Mon Sep 17 00:00:00 2001