6:46 a.m.
Hi,
This is my first time looking at this, so sorry if I bring up stuff
which has already been discussed at length.
On the whole, this all looks pretty great ...
On Thu, 2008-11-13 at 17:30 +0000, Daniel P. Berrange wrote:
This patch is the public API parts of the node device enumeration
code.
No changes since David's last submission of this, except for some
Makefile tweaks
Daniel
diff -r 78738f80cf4c include/libvirt/libvirt.h
--- a/include/libvirt/libvirt.h Wed Nov 12 21:05:32 2008 +0000
+++ b/include/libvirt/libvirt.h Wed Nov 12 21:11:46 2008 +0000
@@ -995,6 +995,74 @@
unsigned int flags);
/*
+ * Host device enumeration
+ */
+
+/**
+ * virNodeDevice:
+ *
+ * A virNodeDevice contains a node (host) device details.
+ */
+
+typedef struct _virNodeDevice virNodeDevice;
+
+/**
+ * virNodeDevicePtr:
+ *
+ * A virNodeDevicePtr is a pointer to a virNodeDevice structure. Get
+ * one via virNodeDeviceLookupByKey, virNodeDeviceLookupByName, or
+ * virNodeDeviceCreate. Be sure to Call virNodeDeviceFree when done
+ * using a virNodeDevicePtr obtained from any of the above functions to
+ * avoid leaking memory.
+ */
+
+typedef virNodeDevice *virNodeDevicePtr;
+
+
+int virNodeNumOfDevices (virConnectPtr conn,
+ unsigned int flags);
+
+int virNodeListDevices (virConnectPtr conn,
+ char **const names,
+ int maxnames,
+ unsigned int flags);
+
+int virNodeNumOfDevicesByCap (virConnectPtr conn,
+ const char *cap,
+ unsigned int flags);
+
+int virNodeListDevicesByCap (virConnectPtr conn,
+ const char *cap,
+ char **const names,
+ int maxnames,
+ unsigned int flags);
How about combining these two sets of functions and if the capability
type isn't supplied list all devices?
+
+virNodeDevicePtr virNodeDeviceLookupByName (virConnectPtr conn,
+ const char *name);
+
+const char * virNodeDeviceGetName (virNodeDevicePtr dev);
+
How stable are these names? e.g. should we say that no-one should rely
on the format of the name and that the name of a given device could
change across node reboots? Even if HAL guarantees the name to be stable
(does it?), if you switch between HAL and DevKit it could change, right?
+const char * virNodeDeviceGetParent (virNodeDevicePtr
dev);
A GetParent() but no ListChildren() seems a little strange ...
Some of the hierarchy seems a little strange to me, e.g.
# ./virsh node-list-devices --cap net
...
net_00_13_20_f7_4a_06
# ./virsh node-device-dumpxml net_00_13_20_f7_4a_06
<device>
<name>net_00_13_20_f7_4a_06</name>
<parent>pci_8086_10de</parent>
<capability type='net'>
<interface>eth0</interface>
<address>00:13:20:f7:4a:06</address>
<capability type='80203'>
<mac_address>001320f74a06</address>
</capability>
</capability>
</device>
# ./virsh node-device-dumpxml pci_8086_10de
<device>
<name>pci_8086_10de</name>
<parent>computer</parent>
<capability type='pci'>
<domain>0</domain>
<bus>0</bus>
<slot>25</slot>
<function>0</function>
<product id='4318'>82567LM-3 Gigabit Network Connection</product>
<vendor id='32902'>Intel Corporation</vendor>
</capability>
</device>
I see that all this naturally flows from the way hal does things, but
the pci device isn't a parent of the network device in any real
sense ... I guess I would expect to just see one device with both the
'net' and 'pci' capabilities.
+int virNodeDeviceNumOfCaps (virNodeDevicePtr
dev);
+
+int virNodeDeviceListCaps (virNodeDevicePtr dev,
+ char **const names,
+ int maxnames);
I think it would make more sense to me if there was a fixed set of
capability types and for them to be an enum in the API rather than
strings ...
+char * virNodeDeviceGetXMLDesc (virNodeDevicePtr
dev,
+ unsigned int flags);
+
+virNodeDevicePtr virNodeDeviceCreate (virConnectPtr conn,
+ const char *xml,
+ unsigned int flags);
Very strange, and we don't implement it? What's the idea with this?
Perhaps leave it out until we do implement it?
+int virNodeDeviceDestroy (virNodeDevicePtr
dev,
+ unsigned int flags);
Ditto.
diff -r 78738f80cf4c include/libvirt/virterror.h
--- a/include/libvirt/virterror.h Wed Nov 12 21:05:32 2008 +0000
+++ b/include/libvirt/virterror.h Wed Nov 12 21:11:46 2008 +0000
@@ -58,6 +58,7 @@
VIR_FROM_STORAGE, /* Error from storage driver */
VIR_FROM_NETWORK, /* Error from network config */
VIR_FROM_DOMAIN, /* Error from domain config */
+ VIR_FROM_DEVMONITOR,/* Error from node device monitor */
This is the only time any mention of a "device monitor" is made in the
public API, so it's a bit odd.
Perhaps VIR_FROM_NODE ?
diff -r 78738f80cf4c src/libvirt.c
--- a/src/libvirt.c Wed Nov 12 21:05:32 2008 +0000
+++ b/src/libvirt.c Wed Nov 12 21:11:46 2008 +0000
...
+/**
+ * virNodeDeviceGetXMLDesc:
+ * @dev: pointer to the node device
+ * @flags: flags for XML generation (unused, pass 0)
+ *
+ * Fetch an XML document describing all aspects of
+ * the device.
+ *
+ * Return the XML document, or NULL on error
+ */
+char *virNodeDeviceGetXMLDesc(virNodeDevicePtr dev, unsigned int flags)
+{
+ DEBUG("dev=%p, conn=%p", dev, dev ? dev->conn : NULL);
+
+ if (!VIR_IS_CONNECTED_NODE_DEVICE(dev)) {
+ virLibNodeDeviceError(NULL, VIR_ERR_INVALID_NODE_DEVICE, __FUNCTION__);
+ return NULL;
+ }
+
+ if (dev->conn->deviceMonitor &&
dev->conn->deviceMonitor->deviceDumpXML)
+ return dev->conn->deviceMonitor->deviceDumpXML (dev, flags);
+
+ virLibConnError (dev->conn, VIR_ERR_NO_SUPPORT, __FUNCTION__);
+ return NULL;
+}
Missing check for flags == 0
+/**
+ * virNodeDeviceListCaps:
+ * @dev: the device
+ * @names: array to collect the list of capability names
+ * @maxnames: size of @names
+ *
+ * Lists the names of the capabilities supported by the device.
+ *
+ * Returns the number of capability names listed in @names.
+ */
+int virNodeDeviceListCaps(virNodeDevicePtr dev,
+ char **const names,
+ int maxnames)
+{
+ DEBUG("dev=%p, conn=%p, names=%p, maxnames=%d",
+ dev, dev ? dev->conn : NULL, names, maxnames);
+
+ if (!VIR_IS_CONNECTED_NODE_DEVICE(dev)) {
+ virLibNodeDeviceError(NULL, VIR_ERR_INVALID_NODE_DEVICE, __FUNCTION__);
+ return -1;
+ }
+
+ if (dev->conn->deviceMonitor &&
dev->conn->deviceMonitor->deviceListCaps)
+ return dev->conn->deviceMonitor->deviceListCaps (dev, names,
maxnames);
+
+ virLibConnError (dev->conn, VIR_ERR_NO_SUPPORT, __FUNCTION__);
+ return -1;
+}
No checking of names/maxnames
Cheers,
Mark.