9:34 a.m.
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;
+};