[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



"Daniel P. Berrange" <berrange redhat com> wrote:
> This implements support for bridge configs in openvz following the rules
> set out in
>
> http://wiki.openvz.org/Virtual_Ethernet_device#Making_a_bridged_veth-device_persistent
>
> This simply requires that the admin has created /etc/vz/vznetctl.conf
> containing
>
>   #!/bin/bash
>   EXTERNAL_SCRIPT="/usr/sbin/vznetaddbr"
>
>
> For openvz <= 3.0.22, we have to manually re-write the NETIF line to
> add the bridge config parameter. For newer openvz we can simply pass
> the bridge name on the commnand line to --netif_add.
>
> Older openvz also requires that the admin install /usr/sbin/vznetaddbr
> since it is not available out of the box

Hi Dan,
Looks fine on principle.
A couple suggestions below.

...
> +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;

That works because safewrite always returns negative upon failure.
By the way, in ensuring that, I noticed that both safewrite and
saferead have this dead code:

                if (r == 0)
                        return nread;

                if (r == 0)
                        return nwritten;

that's because read and write never return 0
when they've been told to read or write N>0 bytes.


> +    close(fd);
> +    close(temp_fd);

Officially, you should always check for failure when
you've opened the file descriptor for writing.

> +    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));

> +    if (fd != -1)
> +        close(fd);
> +    if (temp_fd != -1)
> +        close(temp_fd);
> +    unlink(temp_file);
> +    return -1;
> +}
...
> diff -r 2e218ae09a5d src/openvz_driver.c
> --- a/src/openvz_driver.c	Tue Oct 14 15:14:35 2008 +0100
> +++ b/src/openvz_driver.c	Tue Oct 14 15:43:36 2008 +0100
> @@ -55,6 +55,7 @@
>  #include "openvz_conf.h"
>  #include "nodeinfo.h"
>  #include "memory.h"
> +#include "bridge.h"
>
>  #define OPENVZ_MAX_ARG 28
>  #define CMDBUF_LEN 1488
> @@ -329,13 +330,55 @@ static int openvzDomainReboot(virDomainP
>      return 0;
>  }
>
> +static char *
> +openvzGenerateVethName(int veid, char *dev_name_ve)
> +{
> +    char    dev_name[32];
> +    int     ifNo = 0;
> +
> +    if (sscanf(dev_name_ve, "%*[^0-9]%d", &ifNo) != 1)
> +        return NULL;
> +    if (snprintf(dev_name, sizeof(dev_name), "veth%d.%d", veid, ifNo) < 7)
> +        return NULL;
> +    return strdup(dev_name);
> +}
> +
> +static char *
> +openvzGenerateContainerVethName(int veid)
> +{
> +    int     ret;
> +    char    temp[1024];
> +
> +    /* try to get line "^NETIF=..." from config */
> +    if ( (ret = openvzReadConfigParam(veid, "NETIF", temp, sizeof(temp))) <= 0) {
> +        snprintf(temp, sizeof(temp), "eth0");
> +    } else {
> +        char   *s;
> +        int     max = 0;
> +
> +        /* get maximum interface number (actually, it is the last one) */
> +        for (s=strtok(temp, ";"); s; s=strtok(NULL, ";")) {
> +            int x;
> +
> +            if (sscanf(s, "ifname=eth%d", &x) != 1) return NULL;
> +            if (x > max) max = x;

More consistent and imho, readable to put newline after each ")":

               if (sscanf(s, "ifname=eth%d", &x) != 1)
                   return NULL;
               if (x > max)
                   max = x;

> +        }
> +
> +        /* set new name */
> +        snprintf(temp, sizeof(temp), "eth%d", max+1);
> +    }
> +    return strdup(temp);
> +}
> +
>  static int
>  openvzDomainSetNetwork(virConnectPtr conn, const char *vpsid,
> -                        virDomainNetDefPtr net)
> +                       virDomainNetDefPtr net,
> +                       virBufferPtr configBuf)
>  {
>      int rc = 0, narg;
>      const char *prog[OPENVZ_MAX_ARG];
> -    char *mac = NULL;
> +    char macaddr[VIR_MAC_STRING_BUFLEN];
> +    struct openvz_driver *driver = (struct openvz_driver *) conn->privateData;
>
>  #define ADD_ARG_LIT(thisarg)                                            \
>      do {                                                                \
> @@ -367,21 +410,61 @@ openvzDomainSetNetwork(virConnectPtr con
>          ADD_ARG_LIT(vpsid);
>      }
>
> -    if (openvzCheckEmptyMac(net->mac) > 0)
> -          mac = openvzMacToString(net->mac);
> -
> -    if (net->type == VIR_DOMAIN_NET_TYPE_BRIDGE &&
> -           net->data.bridge.brname != NULL) {
> -        char opt[1024];
> +    virFormatMacAddr(net->mac, macaddr);
> +
> +    if (net->type == VIR_DOMAIN_NET_TYPE_BRIDGE) {
> +        virBuffer buf = VIR_BUFFER_INITIALIZER;
> +        char *opt;
> +        char *dev_name_ve;
> +        int veid = strtoI(vpsid);
> +
>          //--netif_add ifname[,mac,host_ifname,host_mac]
>          ADD_ARG_LIT("--netif_add") ;
> -        strncpy(opt, net->data.bridge.brname, 256);
> -        if (mac != NULL) {
> -            strcat(opt, ",");
> -            strcat(opt, mac);
> -        }
> +
> +        /* generate interface name in ve and copy it to options */
> +        dev_name_ve = openvzGenerateContainerVethName(veid);
> +        if (dev_name_ve == NULL) {
> +           openvzError(conn, VIR_ERR_INTERNAL_ERROR,
> +                    _("Could not generate eth name for container"));
> +           rc = -1;
> +           goto exit;
> +        }
> +
> +        /* if user doesn't specified host interface name,
> +         * than we need to generate it */
> +        if (net->ifname == NULL) {
> +            net->ifname = openvzGenerateVethName(veid, dev_name_ve);
> +            if (net->ifname == NULL) {
> +               openvzError(conn, VIR_ERR_INTERNAL_ERROR,
> +                        _("Could not generate veth name"));
> +               rc = -1;
> +               VIR_FREE(dev_name_ve);
> +               goto exit;
> +            }
> +        }
> +
> +        virBufferAdd(&buf, dev_name_ve, -1); /* Guest dev */
> +        virBufferVSprintf(&buf, ",%s", macaddr); /* Guest dev mac */
> +        virBufferVSprintf(&buf, ",%s", net->ifname); /* Host dev */
> +        virBufferVSprintf(&buf, ",%s", macaddr); /* Host dev mac */
> +
> +        if (driver->version >= VZCTL_BRIDGE_MIN_VERSION) {
> +            virBufferVSprintf(&buf, ",%s", net->data.bridge.brname); /* Host bridge */
> +        } else {
> +            virBufferVSprintf(configBuf, "ifname=%s", dev_name_ve);
> +            virBufferVSprintf(configBuf, ",mac=%s", macaddr); /* Guest dev mac */
> +            virBufferVSprintf(configBuf, ",host_ifname=%s", net->ifname); /* Host dev */
> +            virBufferVSprintf(configBuf, ",host_mac=%s", macaddr); /* Host dev mac */
> +            virBufferVSprintf(configBuf, ",bridge=%s", net->data.bridge.brname); /* Host bridge */
> +        }
> +
> +        VIR_FREE(dev_name_ve);
> +
> +        if (!(opt = virBufferContentAndReset(&buf)))
> +            goto no_memory;
> +
>          ADD_ARG_LIT(opt) ;
> -    }else if (net->type == VIR_DOMAIN_NET_TYPE_ETHERNET &&
> +    } else if (net->type == VIR_DOMAIN_NET_TYPE_ETHERNET &&
>                net->data.ethernet.ipaddr != NULL) {
>          //--ipadd ip
>          ADD_ARG_LIT("--ipadd") ;
> @@ -402,18 +485,57 @@ openvzDomainSetNetwork(virConnectPtr con
>
>   exit:
>      cmdExecFree(prog);
> -    VIR_FREE(mac);
>      return rc;
>
>   no_memory:
>      openvzError(conn, VIR_ERR_INTERNAL_ERROR,
>                  _("Could not put argument to %s"), VZCTL);
>      cmdExecFree(prog);
> -    VIR_FREE(mac);
>      return -1;
>
>  #undef ADD_ARG_LIT
>  }
> +
> +
> +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.

> +    return -1;
> +}
> +


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]