On Fri, Oct 17, 2008 at 03:58:05PM +0200, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote: ...
+int +openvzWriteConfigParam(int vpsid, const char *param, const char *value) +{ + char conf_file[PATH_MAX]; + char temp_file[PATH_MAX]; + char line[PATH_MAX] ; + int fd, temp_fd; + + if (openvzLocateConfFile(vpsid, conf_file, PATH_MAX, "conf")<0) + return -1; + if (openvzLocateConfFile(vpsid, temp_file, PATH_MAX, "tmp")<0) + return -1; + + fd = open(conf_file, O_RDONLY); + if (fd == -1) + return -1; + temp_fd = open(temp_file, O_WRONLY | O_CREAT | O_TRUNC, 0644); + if (temp_fd == -1) { + close(fd); + return -1; + } + + while(1) { + if (openvz_readline(fd, line, sizeof(line)) <= 0) + break; + + if (!STRPREFIX(line, param)) { + if (safewrite(temp_fd, line, strlen(line)) != + strlen(line)) + goto error; + } + } + + if (safewrite(temp_fd, param, strlen(param)) != + strlen(param)) + goto error; + if (safewrite(temp_fd, "=\"", 2) != 2) + goto error; + if (safewrite(temp_fd, value, strlen(value)) != + strlen(value)) + goto error; + if (safewrite(temp_fd, "\"\n", 2) != 2) + goto error;
How about this instead, so the reader doesn't have to manually count string lengths and verify that the "VAR" in strlen(VAR) on the RHS matches the one on the LHS:
if (safewrite(temp_fd, param, strlen(param)) < 0 || safewrite(temp_fd, "=\"", 2) < 0 || safewrite(temp_fd, value, strlen(value)) < 0 || safewrite(temp_fd, "\"\n", 2) < 0) goto error;
Yeah, that's good.
+ close(fd); + close(temp_fd);
Officially, you should always check for failure when you've opened the file descriptor for writing.
Ok, will fix.
+ fd = temp_fd = -1; + + if (rename(temp_file, conf_file) < 0) + goto error; + + return 0; + +error: + fprintf(stderr, "damn %s\n", strerror(errno));
How about mentioning the file name, too:
fprintf(stderr, "failed to write %s: %s\n", conf_file, strerror(errno));
That is debugging code I should have removed - we should raise a proper libvirt error if applicable.
+ +static int +openvzDomainSetNetworkConfig(virConnectPtr conn, + virDomainDefPtr def) +{ + unsigned int i; + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *param; + struct openvz_driver *driver = (struct openvz_driver *) conn->privateData; + + for (i = 0 ; i < def->nnets ; i++) { + if (driver->version < VZCTL_BRIDGE_MIN_VERSION && i > 0) + virBufferAddLit(&buf, ";"); + + if (openvzDomainSetNetwork(conn, def->name, def->nets[i], &buf) < 0) { + openvzError(conn, VIR_ERR_INTERNAL_ERROR, + "%s", _("Could not configure network")); + goto exit; + } + } + + param = virBufferContentAndReset(&buf); + if (driver->version < VZCTL_BRIDGE_MIN_VERSION && def->nnets) { + if (openvzWriteConfigParam(strtoI(def->name), "NETIF", param) < 0) { + VIR_FREE(param); + openvzError(conn, VIR_ERR_INTERNAL_ERROR, + "%s", _("cannot replace NETIF config")); + return -1; + } + } + + VIR_FREE(param); + return 0;
param can be NULL and then dereferenced via openvzWriteConfigParam. How about this instead:
if (driver->version < VZCTL_BRIDGE_MIN_VERSION && def->nnets) { char *param = virBufferContentAndReset(&buf); if (openvzWriteConfigParam(strtoI(def->name), "NETIF", param) < 0) { VIR_FREE(param); openvzError(conn, VIR_ERR_INTERNAL_ERROR, "%s", _("cannot replace NETIF config")); return -1; } VIR_FREE(param); }
return 0;
+exit: + param = virBufferContentAndReset(&buf); + VIR_FREE(param);
Then free the above directly.
Yep, good idea. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|