[libvirt] PATCH 3/4: SUpport bridge config for openvz

[Daniel P. Berrange]

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