"Daniel P. Berrange" <berrange@xxxxxxxxxx> 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; > +} > + -- Libvir-list mailing list Libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list