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

[Daniel Veillard]

On Tue, Jun 24, 2008 at 04:17:18PM +0100, Daniel P. Berrange wrote:
> We currently have two drivers which handle the networking XML containing
> duplicated parsers and formatters for the XML, and very similar structs.
> This patch introduces a new general purpose internal API for parsing and
> formatting network XML, and representing it as a series of structs.
> 
> This code is derived from the current equivalent code in the QEMU driver
> for networks.
> 
> The naming conventions I'm adopting in this patch follow those  in the
> storage driver:
> 
>  - virNetworkPtr - the public opaque object in libvirt.h
>  - virNetworkObjPtr - the primary internal object for network state
>  - virNetworkDefPtr - the configuration data for a network
[...]
> As a mentioned earlier, the impl of these APIs is just copied from the QEMU
> driver, but instead of using pre-declared char[PATH_MAX] fields, we allocate
> memory for strings as required.

  yeah, this all makes perfect sense, thanks for cleaning this up

[...]
> +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.

[...]
> +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

> +    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.
  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.

> diff -r a6e5acdd23df src/network_conf.h
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/src/network_conf.h	Tue Jun 24 15:07:23 2008 +0100
> @@ -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 ?

> + * 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

  ....

> +#ifndef __NETWORK_CONF_H__
> +#define __NETWORK_CONF_H__

  many type definitions looks good. Ultimately we may export some of them 
to provide non-XML interfaces once we get ready for for an 1.0 release.

  Looks good to me, +1, thanks a lot !

Daniel

-- 
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard      | virtualization library  http://libvirt.org/
veillard at redhat.com  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine  http://rpmfind.net/