"Daniel P. Berrange" <berrange(a)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-devi...
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;
+}
+