[libvirt] node device libudev backend second look

[Chris Lalancette]

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