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;
> +};
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list