[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] node device libudev backend second look



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

>From ea9c2464be632ca6da80b8d35599b739e66bf1c8 Mon Sep 17 00:00:00 2001
From: David Allan <dallan redhat com>
Date: Fri, 16 Oct 2009 09:34:35 -0400
Subject: [PATCH 1/1] Fixes per Chris Lalancette, thanks for the feedback

* Added closing </capability> tag for removable devices
* Fixed HAL build
---
 src/conf/node_device_conf.c          |    1 +
 src/node_device/node_device_driver.c |    4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
index 2322819..8a93626 100644
--- a/src/conf/node_device_conf.c
+++ b/src/conf/node_device_conf.c
@@ -402,6 +402,7 @@ char *virNodeDeviceDefFormat(virConnectPtr conn,
                                   data->storage.logical_block_size);
                 virBufferVSprintf(&buf, "      <num_blocks>%llu</num_blocks>\n",
                                   data->storage.num_blocks);
+                virBufferAddLit(&buf, "    </capability>\n");
             } else {
                 virBufferVSprintf(&buf, "    <size>%llu</size>\n",
                                   data->storage.size);
diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
index f3bd45d..f3fbc38 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -74,8 +74,8 @@ static int update_caps(virNodeDeviceObjPtr dev)
 /* 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)
+static int update_driver_name(virConnectPtr conn,
+                              virNodeDeviceObjPtr dev)
 {
     char *driver_link = NULL;
     char devpath[PATH_MAX];
-- 
1.6.4.4


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]