[libvirt] PATCH: Generic internal API for network XML parser/formatter

[Daniel P. Berrange]

On Thu, Jun 26, 2008 at 08:36:46AM -0400, Daniel Veillard wrote:
> [...]
> > +static virNetworkDefPtr
> > +virNetworkDefParseXML(virConnectPtr conn,
> > +                      xmlDocPtr xml)
> > +{
> > +    xmlNodePtr root = NULL;
> > +    xmlXPathContextPtr ctxt = NULL;
> > +    virNetworkDefPtr def;
> > +    char *tmp;
> > +
> > +    if (VIR_ALLOC(def) < 0) {
> > +        virNetworkReportError(conn, VIR_ERR_NO_MEMORY, NULL);
> > +        return NULL;
> > +    }
> > +
> > +    /* Prepare parser / xpath context */
> > +    root = xmlDocGetRootElement(xml);
> > +    if ((root == NULL) || (!xmlStrEqual(root->name, BAD_CAST "network"))) {
> > +        virNetworkReportError(conn, VIR_ERR_INTERNAL_ERROR,
> > +                              "%s", _("incorrect root element"));
> > +        goto error;
> > +    }
> 
>   One thing i'm wondering is if it would not make a bit more sense to cut
> the API at a different level, getting the root element in the caller and
> have the ParseXML() routines operates from an xmlNodePtr. That would allow
> easy refactoring if we want to pack multiple XML definitions in a single
> instance and just call the ParseXML() on each network (or other) subtree.

Hmm, i think I'd probably go a ittle further still - this line you quote
is the only place which uses the 'root' element directly - the rest is all
just XPath queries. So if we changed that to just query '/network' or 
similar, we can just pass an xmlXPathContextPtr object into this method
instead.

> > +virNetworkDefPtr virNetworkDefParse(virConnectPtr conn,
> > +                                    const char *xmlStr,
> > +                                    const char *displayName)
> > +{
> > +    xmlDocPtr xml;
> > +    virNetworkDefPtr def;
> > +
> > +    if (!(xml = xmlReadDoc(BAD_CAST xmlStr, displayName ? displayName : "network.xml", NULL,
> > +                           XML_PARSE_NOENT | XML_PARSE_NONET |
> > +                           XML_PARSE_NOERROR | XML_PARSE_NOWARNING))) {
> > +        virNetworkReportError(conn, VIR_ERR_XML_ERROR, NULL);
> > +        return NULL;
> > +    }
> > +
> > +    def = virNetworkDefParseXML(conn, xml);
> 
>   Do the Root lookup here and pass it down

Or

    ctxt = xmlXPathNewContext(xml);

And just pass 'ctxt' in.

> 
> > +    xmlFreeDoc(xml);
> > +
> > +    return def;
> > +}
> > +
> > +char *virNetworkDefFormat(virConnectPtr conn,
> > +                          const virNetworkDefPtr def)
> > +{
> > +    virBuffer buf = VIR_BUFFER_INITIALIZER;
> > +    unsigned char *uuid;
> > +    char *tmp;
> > +    char uuidstr[VIR_UUID_STRING_BUFLEN];
> 
>   A bit of the same thing but in reverse, basically allocate the buffer 
> in a top routine and call with (conn, buf, def) that way it gets trivial
> to serialize all network definitions in a single XML file for example.
> It's just code refactoring around boundaries for function work, but while we
> are at code refactoring maybe it's easier to do this now.

Sure that sounds reasonable to me.

>   The only drawback one can see is that if you don't get indentation in the
> serialization routine but honnestly I don't think it's a big deal.

Yeah, no big deal - anyone who cares can easily run it through xmllint

> > @@ -0,0 +1,114 @@
> > +/*
> > + * network_conf.h: network XML handling
> > + *
> > + * Copyright (C) 2006-2008 Red Hat, Inc.
> > + * Copyright (C) 2006-2008 Daniel P. Berrange
> 
>   Hum, what about just pointing to the COPYING.LIB file instead of
> including the text below ?

I prefer to keep the full boiler-plate text since that's what's
recommended by the COPYING.LIB file itself. It also means that if
someone takes libvirt code and merges it into another library the
copying terms are clearly shown.

> 
> > + * This library is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Lesser General Public
> > + * License as published by the Free Software Foundation; either
> > + * version 2.1 of the License, or (at your option) any later version.
> > + *
> > + * This library is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > + * Lesser General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public
> > + * License along with this library; if not, write to the Free Software
> > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307  USA

Regards,
Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|