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; +};