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

17 Oct 2008

"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_...
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;
+}
+

    

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

Jim Meyering