5:49 a.m.
On Fri, Oct 17, 2008 at 03:58:05PM +0200, Jim Meyering wrote:
"Daniel P. Berrange" <berrange(a)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 :|