[libvirt] PATCH: 2/14: Generic network XML parser/formatter

[Jim Meyering]

Modulo some minor problems, ACK.
Even though I reviewed only the incremental diffs, there was a bit of
new material.  BTW, nice catch, with the new uses of virBufferEscapeString.

> +virNetworkDefPtr virNetworkDefParseNode(virConnectPtr conn,
> +                                        xmlDocPtr xml,
> +                                        xmlNodePtr root)
> +{
> +    xmlXPathContextPtr ctxt = NULL;
> +    virNetworkDefPtr def = NULL;
> +
> +    if (!xmlStrEqual(root->name, BAD_CAST "network")) {
> +        virNetworkReportError(conn, VIR_ERR_INTERNAL_ERROR,
> +                              "%s", _("incorrect root element"));
> +        goto cleanup;
> +    }

No big deal, but the above goto could be "return NULL;".

> +    ctxt = xmlXPathNewContext(xml);
> +    if (ctxt == NULL) {
> +        virNetworkReportError(conn, VIR_ERR_NO_MEMORY, NULL);
> +        goto cleanup;
> +    }
> +
> +    ctxt->node = root;
> +    def = virNetworkDefParseXML(conn, ctxt);
> +
> +cleanup:
> +    xmlXPathFreeContext(ctxt);
> +    return def;
> +}
...
> +int virNetworkSaveConfig(virConnectPtr conn,
> +                         const char *configDir,
> +                         const char *autostartDir,
> +                         virNetworkObjPtr net)
> +{
> +    char *xml;
> +    int fd = -1, ret = -1;
> +    int towrite;

Use size_t, not int.

> +    int err;
> +
> +    if (!net->configFile &&
> +        asprintf(&net->configFile, "%s/%s.xml",
> +                 configDir, net->def->name) < 0) {

Upon failure, set net->autostartFile to NULL.
Otherwise, when the caller frees "net", it will free a potentially
undefined pointer.

> +        virNetworkReportError(conn, VIR_ERR_NO_MEMORY, NULL);
> +        goto cleanup;
> +    }
> +    if (!net->autostartLink &&
> +        asprintf(&net->autostartLink, "%s/%s.xml",
> +                 autostartDir, net->def->name) < 0) {

Likewise for net->autostartLink.

> +        virNetworkReportError(conn, VIR_ERR_NO_MEMORY, NULL);
> +        goto cleanup;
> +    }
> +
> +    if (!(xml = virNetworkDefFormat(conn,
> +                                    net->newDef ? net->newDef : net->def)))
> +        return -1;

No real problem, but each of the above two goto statements could be
"return -1;", or maybe just change this return -1 to "goto cleanup;".
Otherwise, the inconsistency looks suspicious.

> +    if ((err = virFileMakePath(configDir))) {
> +        virNetworkReportError(conn, VIR_ERR_INTERNAL_ERROR,
> +                              _("cannot create config directory %s: %s"),
> +                              configDir, strerror(err));
> +        goto cleanup;
> +    }
> +
> +    if ((err = virFileMakePath(autostartDir))) {
> +        virNetworkReportError(conn, VIR_ERR_INTERNAL_ERROR,
> +                              _("cannot create autostart directory %s: %s"),
> +                              autostartDir, strerror(err));
> +        goto cleanup;
> +    }
> +
> +    if ((fd = open(net->configFile,
> +                   O_WRONLY | O_CREAT | O_TRUNC,
> +                   S_IRUSR | S_IWUSR )) < 0) {
> +        virNetworkReportError(conn, VIR_ERR_INTERNAL_ERROR,
> +                              _("cannot create config file %s: %s"),
> +                              net->configFile, strerror(errno));
> +        goto cleanup;
> +    }
> +
> +    towrite = strlen(xml);
> +    if (safewrite(fd, xml, towrite) < 0) {
> +        virNetworkReportError(conn, VIR_ERR_INTERNAL_ERROR,
> +                              _("cannot write config file %s: %s"),
> +                              net->configFile, strerror(errno));
> +        goto cleanup;
> +    }
> +
> +    if (close(fd) < 0) {
> +        virNetworkReportError(conn, VIR_ERR_INTERNAL_ERROR,
> +                              _("cannot save config file %s: %s"),
> +                              net->configFile, strerror(errno));
> +        goto cleanup;
> +    }
> +
> +    ret = 0;
> +
> + cleanup:
> +    VIR_FREE(xml);
> +    close(fd);

It's better not to close(-1), so as not to
raise flags via tools like valgrind or strace:

    if (fd >= 0)
        close(fd);

> +    return ret;
> +}
> +
> +virNetworkObjPtr virNetworkLoadConfig(virConnectPtr conn,
> +                                      virNetworkObjPtr *nets,
> +                                      const char *configDir,
> +                                      const char *autostartDir,
> +                                      const char *file)
> +{
> +    char *configFile, *autostartLink;
> +    virNetworkDefPtr def = NULL;
> +    virNetworkObjPtr net;
> +    int autostart;
> +
> +    if (asprintf(&configFile, "%s/%s",
> +                 configDir, file) < 0) {

Upon failed asprintf, here, you need to set configFile = NULL.
Otherwise, the VIR_FREE(configFile) below will free a potentially
undefined pointer.

> +        virNetworkReportError(conn, VIR_ERR_NO_MEMORY, NULL);
> +        goto error;
> +    }
> +    if (asprintf(&autostartLink, "%s/%s",
> +                 autostartDir, file) < 0) {

Same for autostartLink.

> +        virNetworkReportError(conn, VIR_ERR_NO_MEMORY, NULL);
> +        goto error;
> +    }
...
> +error:
> +    VIR_FREE(configFile);
> +    VIR_FREE(autostartLink);
> +    virNetworkDefFree(def);
> +    return NULL;
> +}
...
> +int virNetworkDeleteConfig(virConnectPtr conn,
> +                           virNetworkObjPtr net)
> +{
> +    if (!net->configFile || !net->autostartLink) {
> +        virNetworkReportError(conn, VIR_ERR_INTERNAL_ERROR,
> +                              _("no config file for %s"), net->def->name);
> +        return -1;
> +    }
> +
> +    /* Not fatal if this doesn't work */
> +    unlink(net->autostartLink);
> +
> +    if (unlink(net->configFile) < 0) {
> +        virNetworkReportError(conn, VIR_ERR_INTERNAL_ERROR,
> +                              _("cannot remove config for %s"), net->def->name);

Please include strerror(errno), so people know why it fails.

> +        return -1;
> +    }
> +
> +    return 0;
> +}
> diff -r 097ed9d9ae46 src/network_conf.h
...
> +typedef struct _virNetworkDef virNetworkDef;
> +typedef virNetworkDef *virNetworkDefPtr;
> +struct _virNetworkDef {
> +    unsigned char uuid[VIR_UUID_BUFLEN];
> +    char *name;
> +
> +    char *bridge;       /* Name of bridge device */
> +    int stp : 1; /* Spanning tree protocol */
> +    unsigned long delay;   /* Bridge forward delay (ms) */

Not that it matters, but...
Swapping the two preceding lines should decrease the size of this
struct by 8 bytes on a system with 8-byte pointers and longs.

> +
> +    int forwardType;    /* One of virNetworkForwardType constants */
> +    char *forwardDev;   /* Destination device for forwarding */
> +
> +    char *ipAddress;    /* Bridge IP address */
> +    char *netmask;
> +    char *network;
> +
> +    unsigned int nranges;        /* Zero or more dhcp ranges */
> +    virNetworkDHCPRangeDefPtr ranges;
> +};