[libvirt] node device libudev backend second look
Dave Allan
dallan at redhat.com
Fri Oct 16 13:41:23 UTC 2009
Chris Lalancette wrote:
> 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?
>
Hi Chris,
Thanks for the catching those problems--here is a patch that fixes both
the missing </capability> tag for removable devices and the HAL build
failure.
To build the udev backend, use --without-hal --with-udev when configuring.
Dave
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0001-Fixes-per-Chris-Lalancette-thanks-for-the-feedback.patch
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20091016/50b8d39a/attachment-0001.ksh>
More information about the libvir-list
mailing list