[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]