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.