On Tue, Sep 30, 2008 at 12:17:27AM -0400, David Lively wrote:
> Hi Daniel -
>
> I'm not really ready to submit this yet (hence no [PATCH] in the
> subject), but it is functional enough to play with, mostly with the
> HAL-based driver, though the devkit-based one works too. In particular,
> I'm not yet happy with the code to gather capabilities and bus info
> (though gather_capabilities in node_device_hal.c is close to what I'm
> going for). Also, many of the details (which properties do we care
> about for which buses and capabilities?) that make this useful are
> missing, though I've provided a few guesses to get started. And
> finally, it's had very little testing at this point.
Okay, I think giving a bit of feedback at least on the API entry
points makes sense even if not complete.
> Configure --with-hal or --with-devkit to support one or both (if both
> are configured, HAL is preferred, provided we can talk to hald on dbus).
> There are quite a few TODO's in the patch (mostly little things).
The set of dependancies for libvirt is becoming incredibly large
but that's fine :-)
> * figure out how devkit and HAL correlate, so we report device info
> consistently
that has the potential of being nasty like seeing devices listed
twice
> * register for devkit events to keep device info up-to-date
okay it looks like there is still some work, but thanks a lot for
sharing that first version, very appreciated !
Good to see that you seems to have the python bindings ready too !
> +/*
> + * 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.
> + */
[...]
> +const char * virNodeDeviceGetKey (virNodeDevicePtr dev);
> +
> +const char * virNodeDeviceGetName (virNodeDevicePtr dev);
> +
> +const char * virNodeDeviceGetParentKey(virNodeDevicePtr dev);
> +
> +const char * virNodeDeviceGetBus (virNodeDevicePtr dev);
> +
> +int virNodeDeviceNumOfCaps (virNodeDevicePtr dev);
> +
> +int virNodeDeviceListCaps (virNodeDevicePtr dev,
> + char **const names,
> + int maxnames);
> +
> +char * virNodeDeviceGetXMLDesc (virNodeDevicePtr dev,
> + unsigned int flags);
> +
Opaque type with accessors, looks fine to me !
> +typedef virNodeDevice *virNodeDevicePtr;
> +
> +
> +int virNodeNumOfDevices (virConnectPtr conn);
> +
> +int virNodeListDevices (virConnectPtr conn,
> + char **const names,
> + int maxnames);
> +
> +int virNodeNumOfDevicesByCap (virConnectPtr conn,
> + const char *cap);
> +
> +int virNodeListDevicesByCap (virConnectPtr conn,
> + const char *cap,
> + char **const names,
> + int maxnames);
> +
> +int virNodeNumOfDevicesByBus (virConnectPtr conn,
> + const char *bus);
> +
> +int virNodeListDevicesByBus (virConnectPtr conn,
> + const char *bus,
> + char **const names,
> + int maxnames);
> +
> +virNodeDevicePtr virNodeDeviceLookupByName (virConnectPtr conn,
> + const char *name);
> +
> +virNodeDevicePtr virNodeDeviceLookupByKey (virConnectPtr conn,
> + const char *key);
> +
> +virNodeDevicePtr virNodeDeviceCreate (virConnectPtr conn,
> + const char *xml);
I wonder how many of them should be future-proofed by adding them
a final flags argument too ... For example it may be useful to only
lookup devices which are local to the machine, or the opposite only
remote devices. We don't have to specify now flags values, but having
the APIs ready is sufficient.
The virNodeNum/virNodeListDevices devices can probably all share
the same flags definitions when needed.
The LookupByName/LookupByKey may be able to use the same set. I wonder
a bit about the key argument though, I assume it's something actually
defined by the lower APIs (hal/devkit).
For virNodeDeviceCreate maybe a flags may be needed too, though the
flexibility of the API is provided by the XML.
> +int virNodeDeviceDestroy (virNodeDevicePtr dev);
> +
> +int virNodeDeviceFree (virNodeDevicePtr dev);
Maybe Destroy need flags too, for example to force (or not)
destruction of devices which may be in use.
>
> @@ -332,6 +340,12 @@ skip_function = (
> 'virCopyLastError', # Python API is called virGetLastError instead
> 'virConnectOpenAuth', # Python C code is manually written
> 'virDefaultErrorFunc', # Python virErrorFuncHandler impl calls this from C
> + 'virNodeDeviceGetKey',
> + 'virNodeDeviceGetName',
> + 'virNodeDeviceGetParentKey',
> + 'virNodeDeviceGetBus',
> + 'virNodeDeviceNumOfCaps',
> + 'virNodeDeviceListCaps',
> )
Strange how are the accessors supposed to work from python then ?
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 9845332..10fdd7d 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -1,5 +1,18 @@
> ## Process this file with automake to produce Makefile.in
>
> +NODE_DEVICE_SOURCES = node_device.c node_device.h
> +if HAVE_HAL
> +NODE_DEVICE_CFLAGS = $(HAL_CFLAGS)
> +NODE_DEVICE_LIBS = $(HAL_LIBS)
> +NODE_DEVICE_SOURCES += node_device_hal.c
> +else
> +if HAVE_DEVKIT
> +NODE_DEVICE_CFLAGS = $(DEVKIT_CFLAGS)
> +NODE_DEVICE_LIBS = $(DEVKIT_LIBS)
> +NODE_DEVICE_SOURCES += node_device_devkit.c
> +endif
> +endif
Splitting into modules looks nice,
[...]
> $(GENERIC_LIB_SOURCES) \
> $(DOMAIN_CONF_SOURCES)
> libvirt_lxc_LDFLAGS = $(WARN_CFLAGS) $(COVERAGE_LDCFLAGS)
> -libvirt_lxc_LDADD = $(LIBXML_LIBS) ../gnulib/lib/libgnu.la
> +libvirt_lxc_LDADD = $(LIBXML_LIBS) ../gnulib/lib/libgnu.la $(NODE_DEVICE_LIBS)
> libvirt_lxc_CFLAGS = $(LIBPARTED_CFLAGS)
Hum, the libvirt_lxc really need to link to the device discovery libs ?
> index 655cd05..6a2ef82 100644
> --- a/src/driver.h
> +++ b/src/driver.h
> @@ -601,6 +601,72 @@ struct _virStateDriver {
> };
> #endif
>
> +
> +typedef struct _virNodeDriver virNodeDriver;
> +typedef virNodeDriver *virNodeDriverPtr;
> +
> +typedef int (*virDrvNodeNumOfDevices)(virConnectPtr conn);
> +
> +typedef int (*virDrvNodeListDevices)(virConnectPtr conn,
> + char **const names,
> + int maxnames);
> +
> +typedef int (*virDrvNodeNumOfDevicesByCap)(virConnectPtr conn,
> + const char *cap);
> +
> +typedef int (*virDrvNodeListDevicesByCap)(virConnectPtr conn,
> + const char *cap,
> + char **const names,
> + int maxnames);
> +
> +typedef int (*virDrvNodeNumOfDevicesByBus)(virConnectPtr conn,
> + const char *bus);
> +
> +typedef int (*virDrvNodeListDevicesByBus)(virConnectPtr conn,
> + const char *bus,
> + char **const names,
> + int maxnames);
> +
> +typedef virNodeDevicePtr (*virDrvNodeDeviceLookupByName)(virConnectPtr conn,
> + const char *name);
> +
> +typedef virNodeDevicePtr (*virDrvNodeDeviceLookupByKey)(virConnectPtr conn,
> + const char *key);
> +
> +typedef int (*virDrvNodeDeviceFree)(virNodeDevicePtr dev);
> +
> +typedef char * (*virDrvNodeDeviceDumpXML)(virNodeDevicePtr dev,
> + unsigned int flags);
> +
> +typedef virNodeDevicePtr (*virDrvNodeDeviceCreate)(virConnectPtr conn,
> + const char *xml);
> +
> +typedef int (*virDrvNodeDeviceDestroy)(virNodeDevicePtr dev);
Fine modulo the flags to be added ...
[...]
> +/**
> + * VIR_NODE_DEVICE_MAGIC:
> + *
> + * magic value used to protect the API when pointers to storage vol structures
> + * are passed down by the users.
> + */
> +#define VIR_NODE_DEVICE_MAGIC 0xDEAD5679
> +#define VIR_IS_NODE_DEVICE(obj) ((obj) && (obj)->magic==VIR_NODE_DEVICE_MAGIC)
> +#define VIR_IS_CONNECTED_NODE_DEVICE(obj) (VIR_IS_NODE_DEVICE(obj) && VIR_IS_CONNECT((obj)->conn))
> +
Ah cool :-)
> @@ -315,7 +325,7 @@ virInitialize(void)
> *
> * Handle an error at the connection level
> */
> -static void
> +void
> virLibConnError(virConnectPtr conn, virErrorNumber error, const char *info)
> {
> const char *errmsg;
> @@ -336,7 +346,7 @@ virLibConnError(virConnectPtr conn, virErrorNumber error, const char *info)
> *
> * Handle an error at the connection level
> */
> -static void
> +void
> virLibConnWarning(virConnectPtr conn, virErrorNumber error, const char *info)
> {
> const char *errmsg;
> @@ -454,6 +464,32 @@ virLibStorageVolError(virStorageVolPtr vol, virErrorNumber error,
> }
you really need to export them ?
[...]
for the libvirt.c entry points, looks fine but again flags should be
added. In the
mark them as "int flags ATTRIBUTE_UNUSED" in node_device_* since the code
won't reference them ... yet.
Hum ... we need the comment on the accessors, so they get extracted
as part of the API when doing 'make rebuild' in docs/ added to the
resulting XML, which then will provide the python accessors
automatically I think.
> +
> +
> +const char *virNodeDeviceGetKey(virNodeDevicePtr dev)
> +{
> + DEBUG("dev=%p, conn=%p", dev, dev ? dev->conn : NULL);
> + return dev->key;
> +}
> +
> +const char *virNodeDeviceGetName(virNodeDevicePtr dev)
> +{
> + DEBUG("dev=%p, conn=%p", dev, dev ? dev->conn : NULL);
> + return dev->name;
> +}
> +
> +const char *virNodeDeviceGetParentKey(virNodeDevicePtr dev)
> +{
> + DEBUG("dev=%p, conn=%p", dev, dev ? dev->conn : NULL);
> + return dev->parent_key;
> +}
> +
> +const char *virNodeDeviceGetBus(virNodeDevicePtr dev)
> +{
> + DEBUG("dev=%p, conn=%p", dev, dev ? dev->conn : NULL);
> + return dev->bus_name;
> +}
> +
> +int virNodeDeviceNumOfCaps(virNodeDevicePtr dev)
> +{
> + int ncaps = 0;
> +
> + DEBUG("dev=%p, conn=%p", dev, dev ? dev->conn : NULL);
> + while (dev->cap_names && dev->cap_names[ncaps] != NULL)
> + ++ncaps;
> + return ncaps;
> +}
> +
> +int virNodeDeviceListCaps(virNodeDevicePtr dev,
> + char **const names,
> + int maxnames)
> +{
> + int n = 0;
> +
> + DEBUG("dev=%p, conn=%p", dev, dev ? dev->conn : NULL);
> + while (dev->cap_names && dev->cap_names[n] && n < maxnames) {
> + names[n] = dev->cap_names[n];
> + n++;
> + }
> + return n;
> +}
> diff --git a/src/libvirt_sym.version b/src/libvirt_sym.version
> index b8c470c..e214560 100644
> --- a/src/libvirt_sym.version
> +++ b/src/libvirt_sym.version
> @@ -146,6 +146,19 @@
> virStorageVolGetXMLDesc;
> virStorageVolGetPath;
>
> + virNodeNumOfDevices;
> + virNodeListDevices;
> + virNodeNumOfDevicesByCap;
> + virNodeListDevicesByCap;
> + virNodeNumOfDevicesByBus;
> + virNodeListDevicesByBus;
> + virNodeDeviceLookupByName;
> + virNodeDeviceLookupByKey;
> + virNodeDeviceFree;
> + virNodeDeviceGetXMLDesc;
> + virNodeDeviceCreate;
> + virNodeDeviceDestroy;
> +
> /* Symbols with __ are private only
> for use by the libvirtd daemon.
> They are not part of stable ABI
> @@ -165,6 +178,7 @@
> __virGetNetwork;
> __virGetStoragePool;
> __virGetStorageVol;
> + __virGetNodeDevice;
>
> __virEventRegisterImpl;
looks fine :-)
Okay I didn't yet finished the review for the src/node_* new modules.
I will try to get back to it tomorrow.
So far that looks good to me, at least from a high level and API
perspective !
thanks a lot !
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list