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