[libvirt] PATCH 3/4: SUpport bridge config for openvz
Jim Meyering
jim at meyering.net
Fri Oct 17 13:58:05 UTC 2008
"Daniel P. Berrange" <berrange at 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;
> +}
> +
More information about the libvir-list
mailing list