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

Re: [libvirt] PATCH: 7/12: Public API for node devices



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.


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