Re: [libvirt] anyone implementing host device enumeration?
Hi Daniel -
I'm implementing virNodeDeviceDef, as suggested, but I just noticed
something that really bothers me. In a nutshell, it's that the current
implementation of this scheme unnecessarily increases the (time)
complexity of O(1) operations to O(n).
For example, take virDomainGetXMLDesc(). One would think dumping the
XML descriptor for a domain object we have in hand would be O(1), but in
fact, it's O(n) (where n is the number of domains with the same driver),
because qemudDomainDumpXML must find the DomainObjPtr (from which it can
get the def). I suppose this lookup could be made O(log n) if the
domains were sorted by UUID on the list, but the fact remains that
lookup could be eliminated entirely.
Perhaps it's just a matter of allowing the "public" objects
(virDomain, virNetwork, etc.) to hold a pointer to their optional (never
used by remote driver, for example) private state objects. So I guess
I'm suggesting adding a void * to each of the public objects, which
drivers can use for this purpose. I'll go ahead and do this for
NodeDevices in the next incarnation of this patch to show you what I
mean. (It won't be hard go to the conventional lookup impl if this
turns out to be a bad idea.)
Dave
On Fri, 2008-10-03 at 16:23 +0100, Daniel P. Berrange wrote:
On Tue, Sep 30, 2008 at 12:17:27AM -0400, David Lively wrote:
> diff --git a/src/internal.h b/src/internal.h
> index d96504d..9a94f6d 100644
> --- a/src/internal.h
> +++ b/src/internal.h
> @@ -292,6 +307,25 @@ struct _virStorageVol {
> };
>
>
> +/**
> + * _virNodeDevice:
> + *
> + * Internal structure associated with a node device
> + */
> +struct _virNodeDevice {
> + unsigned int magic; /* specific value to check */
> + int refs; /* reference count */
> + virConnectPtr conn; /* pointer back to the connection */
> + char *key; /* device key */
> + char *name; /* device name */
> + char *parent_key; /* parent device */
> + char *bus_name; /* (optional) bus name */
> + xmlNodePtr bus; /* (optional) bus descriptor */
> + char **cap_names; /* capability names (null-term) */
> + xmlNodePtr *caps; /* capability descs (null-term) */
> +};
This is not the right way to maintain the internal representation of
devices.
The model we follow is to have 2, sometimes 3 structs to represent
an object. I'll summarize in terms of domain objects
- virDomainPtr - the public opaque struct representing a domain
the internal struct is defined in internal.h
and merely contains reference counting, connection
pointer, and any unique keys (ID, name, UUID).
- virDomainObjPtr - the internal struct representing the state of
a domain object. Not all drivers use us - only
stateful drivers do. It is used to track state
such as guest PID, logfile, running state,
and a pointer to the live and inactive configs
This is done in domain_conf.h/.c
- virDomainDefPtr - the internal struct representing the canonical
configuration of a domain. This is a direct
mapping of the XML into a series of structs.
This is done in domain_conf.h/.c
So, in the context for host devices we need a few changes. In the
internal.h file, you should only store the key/name attributes.
There should then be a separate file handling configuration parsing
and formatting, node_device_conf.h/.c. There is probably no need
for a state object, so skip virNodeDeviceObjPtr, and just create a
virNodeDeviceDefPtr representing the XML format as a series of
structs/unions, etc. See virDomainDefPtr for a good example. This
should not store any xmlNodePtr instances - it should be all done
as explicit struct/enum fields
The node_device_conf.c file should at mimium have 2 methods, one
for converting an XML document into a virNodeDeviceDefPtr object,
and one for the converting a virNodeDeviceObjPtr back into an XML
document. Follow the existing API naming / method signatures that
are seen in domain_conf.h / network_conf.h for current 'best practice'
Daniel