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