"Daniel P. Berrange" <berrange@xxxxxxxxxx> 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.
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list