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