[libvirt] [PATCH v2 REPOST 1/8] Add namespace callback hooks to domain_conf.

[Chris Lalancette]

On 07/01/10 - 03:23:07PM, Eric Blake wrote:
> On 07/01/2010 02:59 PM, Chris Lalancette wrote:
> > This patch adds namespace XML parsers to be hooked into
> > the main domain parser.  This allows for individual hypervisor
> > drivers to add per-namespace XML into the main domain XML.
> > 
> > Changes since v1:
> >  - Use a statically declared table for caps->ns, removing the need to
> >    allocate/free it.
> > 
> > Signed-off-by: Chris Lalancette <clalance at redhat.com>
> 
> > +typedef int (*virDomainDefNamespaceParse)(xmlDocPtr, xmlNodePtr,
> > +                                          xmlXPathContextPtr, void **);
> > +typedef void (*virDomainDefNamespaceFree)(void *);
> > +typedef int (*virDomainDefNamespaceXMLFormat)(virBufferPtr, void *);
> > +typedef const char *(*virDomainDefNamespaceHref)(void);
> 
> Should virDomainDefNamespaceHref take a void* argument...
> 
> > +    if (def->namespaceData && def->ns.href)
> > +        virBufferVSprintf(&buf, " %s", (def->ns.href)());
> 
> and pass def->namespaceData through to that function?  I'm a little bit
> nervous about callback functions that cannot be extended, and passing a
> (unused, for now) void* pointer gives us some room for growth without
> changing the API, if need be.

We could.  It's an internal API, though, so we are free to change it at
will later.

> 
> > +++ b/src/conf/domain_conf.c
> > @@ -738,6 +738,9 @@ void virDomainDefFree(virDomainDefPtr def)
> >  
> >      virCPUDefFree(def->cpu);
> >  
> > +    if (def->namespaceData && def->ns.free)
> > +        (def->ns.free)(def->namespaceData);
> 
> Style nit - you can omit the first set of parenthesis:
> 
> def->ns.free(def->namespaceData)
> 
> I don't know which style is more prevalent in existing code though, but
> if you choose to elide the parenthesis, there are several places in this
> patch that are affected.

Yeah, true.  I was actually following the style of other callbacks.  I don't
really care either way.

> 
> >  static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps,
> > -                                            xmlXPathContextPtr ctxt, int flags)
> > +                                            xmlDocPtr xml,
> > +                                            xmlNodePtr root,
> > +                                            xmlXPathContextPtr ctxt,
> > +                                            int flags)
> 
> As long as we are changing this API, should we change flags to unsigned
> int?  And should we add a virCheckFlags(VIR_DOMAIN_XML_INACTIVE..., NULL)?

Again, an internal API, so I guess I could change the flags.  Minor change,
though.  As far as virCheckFlags() is concerned, my take on it (and it's usage
in the tree up to this point) is always at driver entry points, not
internal API's.  While we could add it here, passing bogus flags here is a
programming error, not a user error.

--
Chris Lalancette