On Fri, Oct 17, 2008 at 03:58:05PM +0200, Jim Meyering wrote:
> "Daniel P. Berrange" <berrange@xxxxxxxxxx> 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 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list