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

1 Jul 2008

"Daniel P. Berrange" <berrange@redhat.com> 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
A virNetworkObjPtr contains a reference to one or two virNetworkDefPtr
objects - the current live config, and potentially a secondary inactive
config which will become live at the next restart.
The structs are defined in network_conf.h, along with a bunch of APIs for
dealing with them. These APIs are the same as similarly named ones from
the current qemu driver, but I'll go over them again for a reminder:
virNetworkObjPtr virNetworkFindByUUID(const virNetworkObjPtr nets,
                                        const unsigned char *uuid);
  virNetworkObjPtr virNetworkFindByName(const virNetworkObjPtr nets,
                                        const char *name);
Allow lookup of a virNetworkObjPtr object based on its name or UUID, as
typically obtained from the public virNetworkPtr object.
The 'nets' parameter to both of thse is a linked list of networks which
are currently known to the driver using this API.
...
Thanks for writing those descriptions.  When you write so much code,
I'm reluctant to say anything about comment density, but I was surprised
to see the same functions defined below with no comment saying what each
does.  It'd be nice to copy each of the above into a function-describing
comment.
...
diff -r a6e5acdd23df src/Makefile.am

--- a/src/Makefile.am	Tue Jun 24 15:07:21 2008 +0100
+++ b/src/Makefile.am	Tue Jun 24 15:07:23 2008 +0100
@@ -52,6 +52,7 @@
      driver.h					\
      proxy_internal.c proxy_internal.h		\
      conf.c conf.h                                   \
+                network_conf.c network_conf.h                   \
leading spaces in Makefile.am is technically ok in this context,
but it's inconsistent with the neighboring lines.
...
xm_internal.c xm_internal.h                     \
      remote_internal.c remote_internal.h		\
      bridge.c bridge.h				\
diff -r a6e5acdd23df src/network_conf.c
...
...
+++ b/src/network_conf.c	Tue Jun 24 15:07:23 2008 +0100
@@ -0,0 +1,467 @@
+/*
+ * network_conf.h: network XML handling
s/\.h/.c/  This is a good reason not to put the name
of the file in a comment inside the file itself.

...
...
+static int
+virNetworkDHCPRangeDefParseXML(virConnectPtr conn,
+                               virNetworkDefPtr def,
+                               xmlNodePtr node) {
+
+    xmlNodePtr cur;
+
+    cur = node->children;
+    while (cur != NULL) {
+        xmlChar *start, *end;
+
+        if (cur->type != XML_ELEMENT_NODE ||
+            !xmlStrEqual(cur->name, BAD_CAST "range")) {
+            cur = cur->next;
+            continue;
+        }
+
+        if (!(start = xmlGetProp(cur, BAD_CAST "start"))) {
+            cur = cur->next;
+            continue;
+        }
+        if (!(end = xmlGetProp(cur, BAD_CAST "end"))) {
+            cur = cur->next;
+            xmlFree(start);
+            continue;
+        }
+
+        if (VIR_REALLOC_N(def->ranges, def->nranges + 1) < 0) {
+            xmlFree(start);
+            xmlFree(end);
+            virNetworkReportError(conn, VIR_ERR_NO_MEMORY, NULL);
+            return -1;
+        }
+        def->ranges[def->nranges].start = (char *)start;
+        def->ranges[def->nranges].end = (char *)end;
+        def->nranges++;
+
+        cur = cur->next;
+    }
+
+    return 0;
+}
With four "cur = cur->next;" statements, I prefer a for-loop.
And rather than all the if-negated-condition-then-continue, I find the
positive-cond tests to be more readable.  Here's an untested alternative
that should be equivalent:

static int
virNetworkDHCPRangeDefParseXML(virConnectPtr conn,
                               virNetworkDefPtr def,
                               xmlNodePtr node)
{
    xmlNodePtr cur;
    for (cur = node->children; cur != NULL; cur = cur->next) {
        xmlChar *start, *end = NULL;

        if (!(cur->type == XML_ELEMENT_NODE &&
              xmlStrEqual(cur->name, BAD_CAST "range"))
            continue;

        if ((start = xmlGetProp(cur, BAD_CAST "start"))
            && (end = xmlGetProp(cur, BAD_CAST "end"))) {
            if (VIR_REALLOC_N(def->ranges, def->nranges + 1) < 0) {
                xmlFree(start);
                xmlFree(end);
                virNetworkReportError(conn, VIR_ERR_NO_MEMORY, NULL);
                return -1;
            }
            def->ranges[def->nranges].start = (char *)start;
            def->ranges[def->nranges].end = (char *)end;
            def->nranges++;
        } else {
            xmlFree(start);
            xmlFree(end);
        }
    }

    return 0;
}
...
+static virNetworkDefPtr
+virNetworkDefParseXML(virConnectPtr conn,
+                      xmlDocPtr xml)
+{
+    xmlNodePtr root = NULL;
Initialization not required.
...
+    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;
+    }
+
+    ctxt = xmlXPathNewContext(xml);
+    if (ctxt == NULL) {
+        virNetworkReportError(conn, VIR_ERR_NO_MEMORY,
+                              "%s", _("failed to allocate space for xmlXPathContext string"));
+        goto error;
+    }
+
+
+    /* Extract network name */
+    def->name = virXPathString("string(/network/name[1])", ctxt);
+    if (!def->name) {
+        virNetworkReportError(conn, VIR_ERR_NO_NAME, NULL);
+        goto error;
+    }
+
+    /* Extract network uuid */
+    tmp = virXPathString("string(/network/uuid[1])", ctxt);
+    if (!tmp) {
+        int err;
+        if ((err = virUUIDGenerate(def->uuid))) {
+            virNetworkReportError(conn, VIR_ERR_INTERNAL_ERROR,
+                             _("Failed to generate UUID: %s"), strerror(err));
+            goto error;
+        }
+    } else {
+        if (virUUIDParse(tmp, def->uuid) < 0) {
+            VIR_FREE(tmp);
+            virNetworkReportError(conn, VIR_ERR_INTERNAL_ERROR,
+                                  "%s", _("malformed uuid element"));
+            goto error;
+        }
+        VIR_FREE(tmp);
+    }
+
+    /* Parse bridge information */
+    def->bridge = virXPathString("string(/network/bridge[1]/@name)", ctxt);
+    tmp = virXPathString("string(/network/bridge[1]/@stp)", ctxt);
+    if (tmp && STREQ(tmp, "off"))
+        def->stp = 0;
+    else
+        def->stp = 1;
I have a slight preference for the one-liner,
but if you prefer to avoid C's ternary operator, ...

    /* Default is "on".  */
    def->stp = (tmp && STREQ(tmp, "off") ? 0 : 1);
...
+    VIR_FREE(tmp);
+
+    if (virXPathLong("string(/network/bridge[1]/@delay)", ctxt, &def->delay) < 0)
+        def->delay = 0;
+
+    def->ipAddress = virXPathString("string(/network/ip[1]/@address)", ctxt);
+    def->netmask = virXPathString("string(/network/ip[1]/@netmask)", ctxt);
+    if (def->ipAddress &&
+        def->netmask) {
+        /* XXX someday we want IPv6 too, so inet_aton won't work there */
+        struct in_addr inaddress, innetmask;
+        char *netaddr;
+        int netlen;
+        xmlNodePtr dhcp;
+
+        if (!inet_aton(def->ipAddress, &inaddress)) {
+            virNetworkReportError(conn, VIR_ERR_INTERNAL_ERROR,
+                                  _("cannot parse IP address '%s'"),
+                                  def->ipAddress);
+            goto error;
+        }
+        if (!inet_aton(def->netmask, &innetmask)) {
+            virNetworkReportError(conn, VIR_ERR_INTERNAL_ERROR,
+                                  _("cannot parse netmask '%s'"),
+                                  def->netmask);
+            goto error;
+        }
+
+        inaddress.s_addr &= innetmask.s_addr;
+        netaddr = inet_ntoa(inaddress);
+
+        netlen = strlen(netaddr) + 1 + strlen(def->netmask) + 1;
+        if (VIR_ALLOC_N(def->network, netlen) < 0) {
+            virNetworkReportError(conn, VIR_ERR_NO_MEMORY, NULL);
+            goto error;
+        }
+        strcpy(def->network, netaddr);
+        strcat(def->network, "/");
+        strcat(def->network, def->netmask);
I prefer to use asprintf, and then remove 4 of the 8 lines above,
as well as the netlen declaration.
...
+        if ((dhcp = virXPathNode("/network/ip[1]/dhcp[1]", ctxt)) &&
+            virNetworkDHCPRangeDefParseXML(conn, def, dhcp) < 0)
+            goto error;
+    }
+
+
+    /* IPv4 forwarding setup */
+    if (virXPathBoolean("count(/network/forward) > 0", ctxt)) {
+        if (!def->ipAddress ||
+            !def->netmask) {
+            virNetworkReportError(conn, VIR_ERR_INTERNAL_ERROR,
+                                  "%s", _("Forwarding requested, but no IPv4 address/netmask provided"));
+            goto error;
+        }
+
+        tmp = virXPathString("string(/network/forward[1]/@mode)", ctxt);
+        if (tmp) {
+            if ((def->forwardType = virNetworkForwardTypeFromString(tmp)) < 0) {
+                virNetworkReportError(conn, VIR_ERR_INTERNAL_ERROR,
+                                      _("unknown forwarding type '%s'"), tmp);
+                VIR_FREE(tmp);
missing goto
...
+            }
+            VIR_FREE(tmp);
Recently, I've begun using a slightly different paradigm, in order to remove
the need for a separate free call in the error path, since it's so easy
to omit.  i.e, here, you can do this:

        if (tmp) {
            def->forwardType = virNetworkForwardTypeFromString(tmp);
            VIR_FREE(tmp);
            if (def->forwardType < 0) {
                virNetworkReportError(conn, VIR_ERR_INTERNAL_ERROR,
                                      _("unknown forwarding type '%s'"), tmp);
                goto error;
            }
        } ...

Here, it's not ideal, since this separation duplicates the relatively
long LHS, "def->forwardType", but if you don't like that, use a temporary:

        if (tmp) {
            int fail = ((def->forwardType
                         = virNetworkForwardTypeFromString(tmp)) < 0);
            VIR_FREE(tmp);
            if (fail) {
                virNetworkReportError(conn, VIR_ERR_INTERNAL_ERROR,
                                      _("unknown forwarding type '%s'"), tmp);
                goto error;
            }
        } ...
...
+        } else {
+            def->forwardType = VIR_NETWORK_FORWARD_NAT;
+        }
+
+
+        def->forwardDev = virXPathString("string(/network/forward[1]/@dev)", ctxt);
+    } else {
+        def->forwardType = VIR_NETWORK_FORWARD_NONE;
+    }
+
+    xmlXPathFreeContext(ctxt);
+
+    return def;
+
+ error:
+    xmlXPathFreeContext(ctxt);
+    virNetworkDefFree(def);
+    return NULL;
+}
Here's a patch with the suggested changes to that
single function:

--- parse.c	2008-07-01 11:27:58.000000000 +0200
+++ parse.c	2008-07-01 13:41:18.000000000 +0200
@@ -45,22 +45,20 @@
             goto error;
         }
     } else {
-        if (virUUIDParse(tmp, def->uuid) < 0) {
-            VIR_FREE(tmp);
+        int fail = (virUUIDParse(tmp, def->uuid) < 0);
+        VIR_FREE(tmp);
+        if (fail) {
             virNetworkReportError(conn, VIR_ERR_INTERNAL_ERROR,
                                   "%s", _("malformed uuid element"));
             goto error;
         }
-        VIR_FREE(tmp);
     }
 
     /* Parse bridge information */
     def->bridge = virXPathString("string(/network/bridge[1]/@name)", ctxt);
     tmp = virXPathString("string(/network/bridge[1]/@stp)", ctxt);
-    if (tmp && STREQ(tmp, "off"))
-        def->stp = 0;
-    else
-        def->stp = 1;
+    /* Default is "on".  */
+    def->stp = (tmp && STREQ(tmp, "off") ? 0 : 1);
     VIR_FREE(tmp);
 
     if (virXPathLong("string(/network/bridge[1]/@delay)", ctxt, &def->delay) < 0)
@@ -73,7 +71,6 @@
         /* XXX someday we want IPv6 too, so inet_aton won't work there */
         struct in_addr inaddress, innetmask;
         char *netaddr;
-        int netlen;
         xmlNodePtr dhcp;
 
         if (!inet_aton(def->ipAddress, &inaddress)) {
@@ -92,15 +89,11 @@
         inaddress.s_addr &= innetmask.s_addr;
         netaddr = inet_ntoa(inaddress);
 
-        netlen = strlen(netaddr) + 1 + strlen(def->netmask) + 1;
-        if (VIR_ALLOC_N(def->network, netlen) < 0) {
+        if (asprintf(&def->network, "%s/%s", netaddr, def->netmask) < 0) {
+            def->network = NULL;
             virNetworkReportError(conn, VIR_ERR_NO_MEMORY, NULL);
             goto error;
         }
-        strcpy(def->network, netaddr);
-        strcat(def->network, "/");
-        strcat(def->network, def->netmask);
-
 
         if ((dhcp = virXPathNode("/network/ip[1]/dhcp[1]", ctxt)) &&
             virNetworkDHCPRangeDefParseXML(conn, def, dhcp) < 0)
@@ -119,12 +112,13 @@
 
         tmp = virXPathString("string(/network/forward[1]/@mode)", ctxt);
         if (tmp) {
-            if ((def->forwardType = virNetworkForwardTypeFromString(tmp)) < 0) {
+            def->forwardType = virNetworkForwardTypeFromString(tmp);
+            VIR_FREE(tmp);
+            if (def->forwardType < 0) {
                 virNetworkReportError(conn, VIR_ERR_INTERNAL_ERROR,
                                       _("unknown forwarding type '%s'"), tmp);
-                VIR_FREE(tmp);
+                goto error;
             }
-            VIR_FREE(tmp);
         } else {
             def->forwardType = VIR_NETWORK_FORWARD_NAT;
         }
...
+virNetworkDefPtr virNetworkDefParse(virConnectPtr conn,
I'll look at the rest later.

    

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

Jim Meyering