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?
--
Chris Lalancette
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
