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

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

> @@ -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