10:59 a.m.
This set is actually authored by Dave Leskovec. He is moving on to some other
work and I will be working this into the tree. I have made a few changes
to this, mostly to allow this code to compile out if the proper kernel
and userspace support is not present.
I am submitting it as an RFC because the required kernel patches are not yet
upstream (even in -mm) and there are some missing userspace patches to
iproute2 as well. I expect there to be quite a few comments and suggestions
about the veth.c patch, so I wanted to go ahead and get that started while
we wait for upstream to catch up.
Comments appreciated!
10:59 a.m.
New subject: [libvirt] [PATCH 1 of 3] [LXC] Add functions to manage veth device pairs
# HG changeset patch
# User Dave Leskovec <dlesko(a)linux.vnet.ibm.com>
# Date 1213891164 25200
# Node ID 386c067de8995028dd11f70602081c31682dd293
# Parent 8d2afc533c91c4796512e1e71c8283e86eafd18a
[LXC] Add functions to manage veth device pairs
This gives us the ability to create a veth pair so that we can move one
into the network namespace of an LXC container.
diff -r 8d2afc533c91 -r 386c067de899 configure.in
--- a/configure.in Tue Jun 17 15:55:03 2008 +0000
+++ b/configure.in Thu Jun 19 08:59:24 2008 -0700
@@ -301,6 +301,20 @@
if test "$with_qemu" = "yes" ; then
AC_CHECK_HEADERS([linux/param.h linux/sockios.h linux/if_bridge.h linux/if_tun.h],,
AC_MSG_ERROR([You must install kernel-headers in order to compile
libvirt]))
+fi
+
+dnl
+dnl check for patched iproute2 for lxc network support
+dnl
+if test "$with_lxc" = "yes" ; then
+ AC_MSG_CHECKING([for NETNS support])
+ if ip link help 2>&1 | grep -q netns; then
+ with_lxc_netns="yes"
+ AC_DEFINE([HAVE_NETNS], [], [Kernel has NETNS support])
+ else
+ with_lxc_netns="no"
+ fi
+ AC_MSG_RESULT($with_lxc_netns)
fi
dnl Need to test if pkg-config exists
diff -r 8d2afc533c91 -r 386c067de899 src/Makefile.am
--- a/src/Makefile.am Tue Jun 17 15:55:03 2008 +0000
+++ b/src/Makefile.am Thu Jun 19 08:59:24 2008 -0700
@@ -64,6 +64,7 @@
lxc_driver.c lxc_driver.h \
lxc_conf.c lxc_conf.h \
lxc_container.c lxc_container.h \
+ veth.c veth.h \
nodeinfo.h nodeinfo.c \
util.c util.h
diff -r 8d2afc533c91 -r 386c067de899 src/veth.c
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/src/veth.c Thu Jun 19 08:59:24 2008 -0700
@@ -0,0 +1,247 @@
+/*
+ * Copyright IBM Corp. 2008
+ *
+ * veth.c: file description
+ *
+ * Authors:
+ * David L. Leskovec <dlesko at linux.vnet.ibm.com>
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+
+#include <config.h>
+
+#ifdef HAVE_NETNS
+
+#include <string.h>
+
+#include "veth.h"
+#include "internal.h"
+#include "memory.h"
+#include "util.h"
+
+#define DEBUG(fmt,...) VIR_DEBUG(__FILE__, fmt, __VA_ARGS__)
+#define DEBUG0(msg) VIR_DEBUG(__FILE__, "%s", msg)
+
+/* Functions */
+/**
+ * getFreeVethName:
+ * @veth: name for veth device (NULL to find first open)
+ * @maxLen: max length of veth name
+ * @startDev: device number to start at (x in vethx)
+ *
+ * Looks in /sys/class/net/ to find the first available veth device
+ * name.
+ *
+ * Returns 0 on success or -1 in case of error
+ */
+static int getFreeVethName(char *veth, int maxLen, int startDev)
+{
+ int rc = -1;
+ int devNum = startDev;
+ char path[PATH_MAX];
+
+ snprintf(path, PATH_MAX, "/sys/class/net/veth%d/", devNum);
+ while (virFileExists(path)) {
+ ++devNum;
+ sprintf(path, "/sys/class/net/veth%d/", devNum);
+ }
+
+ snprintf(veth, maxLen, "veth%d", devNum);
+
+ rc = devNum;
+
+ return rc;
+}
+
+/**
+ * vethCreate:
+ * @veth1: name for one end of veth pair
+ * @veth1MaxLen: max length of veth1 name
+ * @veth2: name for one end of veth pair
+ * @veth2MaxLen: max length of veth1 name
+ *
+ * Creates a veth device pair using the ip command:
+ * ip link add veth1 type veth peer name veth2
+ * NOTE: If veth1 and veth2 names are not specified, ip will auto assign
+ * names. There seems to be two problems here -
+ * 1) There doesn't seem to be a way to determine the names of the
+ * devices that it creates. They show up in ip link show and
+ * under /sys/class/net/ however there is no guarantee that they
+ * are the devices that this process just created.
+ * 2) Once one of the veth devices is moved to another namespace, it
+ * is no longer visible in the parent namespace. This seems to
+ * confuse the name assignment causing it to fail with File exists.
+ * Because of these issues, this function currently forces the caller
+ * to fully specify the veth device names.
+ *
+ * Returns 0 on success or -1 in case of error
+ */
+int vethCreate(char* veth1, int veth1MaxLen,
+ char* veth2, int veth2MaxLen)
+{
+ int rc = -1;
+ const char *argv[] = {
+ "ip", "link", "add", veth1, "type",
"veth", "peer", "name", veth2, NULL
+ };
+ int cmdResult;
+ int vethDev = 0;
+
+ if ((NULL == veth1) || (NULL == veth2)) {
+ goto error_out;
+ }
+
+ DEBUG("veth1: %s veth2: %s", veth1, veth2);
+
+ if (1 > strlen(veth1)) {
+ vethDev = getFreeVethName(veth1, veth1MaxLen, 0);
+ ++vethDev;
+ DEBUG("assigned veth1: %s", veth1);
+ }
+
+ if (1 > strlen(veth2)) {
+ vethDev = getFreeVethName(veth2, veth2MaxLen, vethDev);
+ DEBUG("assigned veth2: %s", veth2);
+ }
+
+ rc = virRun(NULL, (char**)argv, &cmdResult);
+
+ if (0 == rc) {
+ rc = cmdResult;
+ }
+
+error_out:
+ return rc;
+}
+
+/**
+ * vethDelete:
+ * @veth: name for one end of veth pair
+ *
+ * This will delete both veth devices in a pair. Only one end needs to
+ * be specified. The ip command will identify and delete the other veth
+ * device as well.
+ * ip link del veth
+ *
+ * Returns 0 on success or -1 in case of error
+ */
+int vethDelete(const char *veth)
+{
+ int rc = -1;
+ const char *argv[] = {"ip", "link", "del", veth,
NULL};
+ int cmdResult;
+
+ if (NULL == veth) {
+ goto error_out;
+ }
+
+ DEBUG("veth: %s", veth);
+
+ rc = virRun(NULL, (char**)argv, &cmdResult);
+
+ if (0 == rc) {
+ rc = cmdResult;
+ }
+
+error_out:
+ return rc;
+}
+
+/**
+ * vethInterfaceUpOrDown:
+ * @veth: name of veth device
+ * @upOrDown: 0 => down, 1 => up
+ *
+ * Enables a veth device using the ifconfig command. A NULL inetAddress
+ * will cause it to be left off the command line.
+ *
+ * Returns 0 on success or -1 in case of error
+ */
+int vethInterfaceUpOrDown(const char* veth, int upOrDown)
+{
+ int rc = -1;
+ char upOrDownString[8];
+ const char *argv[] = {"ifconfig", veth, upOrDownString, NULL};
+ int cmdResult;
+
+ if (NULL == veth) {
+ goto error_out;
+ }
+
+ if (0 == upOrDown) {
+ strcpy(upOrDownString, "down");
+ } else {
+ strcpy(upOrDownString, "up");
+ }
+
+ rc = virRun(NULL, (char**)argv, &cmdResult);
+
+ if (0 == rc) {
+ rc = cmdResult;
+ }
+
+error_out:
+ return rc;
+}
+
+/**
+ * moveInterfaceToNetNs:
+ * @interface: name of device
+ * @pidInNs: PID of process in target net namespace
+ *
+ * Moves the given device into the target net namespace specified by the given
+ * pid using this command:
+ * ip link set interface netns pidInNs
+ *
+ * Returns 0 on success or -1 in case of error
+ */
+int moveInterfaceToNetNs(const char* interface, int pidInNs)
+{
+ int rc;
+ /* offset of the pid field in the following args */
+ const int pidArgvOffset = 5;
+ const char *argv[] = {
+ "ip", "link", "set", interface, "netns",
NULL, NULL
+ };
+ int cmdResult;
+ int len;
+
+ if (NULL == interface) {
+ goto error_out;
+ }
+
+ if (0 != VIR_ALLOC_N(argv[pidArgvOffset], (sizeof(int) * 3) + 1)) {
+ goto error_out;
+ }
+ len = snprintf(argv[pidArgvOffset], (sizeof(int) * 3) + 1, "%d", pidInNs);
+ if (len >= (sizeof(int) * 3) + 1) {
+ goto cleanup;
+ }
+
+ rc = virRun(NULL, (char**)argv, &cmdResult);
+
+ if (0 == rc) {
+ rc = cmdResult;
+ }
+
+cleanup:
+ VIR_FREE(argv[pidArgvOffset]);
+
+error_out:
+ return rc;
+}
+
+#endif /* HAVE_NETNS */
+
diff -r 8d2afc533c91 -r 386c067de899 src/veth.h
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/src/veth.h Thu Jun 19 08:59:24 2008 -0700
@@ -0,0 +1,39 @@
+/*
+ * Copyright IBM Corp. 2008
+ *
+ * veth.h: file description
+ *
+ * Authors:
+ * David L. Leskovec <dlesko at linux.vnet.ibm.com>
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+
+#ifndef VETH_H
+#define VETH_H
+
+#include <config.h>
+
+#ifdef HAVE_NETNS
+
+/* Function declarations */
+int vethCreate(char* veth1, int veth1MaxLen, char* veth2,
+ int veth2MaxLen);
+int vethDelete(const char* veth);
+int vethInterfaceUpOrDown(const char* veth, int upOrDown);
+int moveInterfaceToNetNs(const char *interface, int pidInNs);
+
+#endif /* HAVE_NETNS */
+#endif /* VETH_H */
11:43 a.m.
New subject: [libvirt] [PATCH 1 of 3] [LXC] Add functions to manage veth device pairs
On Thu, Jun 19, 2008 at 08:59:52AM -0700, Dan Smith wrote:
# HG changeset patch
# User Dave Leskovec <dlesko(a)linux.vnet.ibm.com>
# Date 1213891164 25200
# Node ID 386c067de8995028dd11f70602081c31682dd293
# Parent 8d2afc533c91c4796512e1e71c8283e86eafd18a
[LXC] Add functions to manage veth device pairs
This gives us the ability to create a veth pair so that we can move one
into the network namespace of an LXC container.
Basically it's executing ip commands, so the real code changes are outside
of libvirt.
diff -r 8d2afc533c91 -r 386c067de899 configure.in
--- a/configure.in Tue Jun 17 15:55:03 2008 +0000
+++ b/configure.in Thu Jun 19 08:59:24 2008 -0700
@@ -301,6 +301,20 @@
if test "$with_qemu" = "yes" ; then
AC_CHECK_HEADERS([linux/param.h linux/sockios.h linux/if_bridge.h linux/if_tun.h],,
AC_MSG_ERROR([You must install kernel-headers in order to compile
libvirt]))
+fi
+
+dnl
+dnl check for patched iproute2 for lxc network support
+dnl
+if test "$with_lxc" = "yes" ; then
+ AC_MSG_CHECKING([for NETNS support])
+ if ip link help 2>&1 | grep -q netns; then
+ with_lxc_netns="yes"
+ AC_DEFINE([HAVE_NETNS], [], [Kernel has NETNS support])
+ else
+ with_lxc_netns="no"
+ fi
+ AC_MSG_RESULT($with_lxc_netns)
fi
Hum, it's not only a kernel feature, it looks more like a dependancy on
iproute, maybe that can be checked at runtime when initializing the lxc
module, no ?
[...]
+static int getFreeVethName(char *veth, int maxLen, int startDev)
+{
+ int rc = -1;
+ int devNum = startDev;
+ char path[PATH_MAX];
+
+ snprintf(path, PATH_MAX, "/sys/class/net/veth%d/", devNum);
+ while (virFileExists(path)) {
+ ++devNum;
+ sprintf(path, "/sys/class/net/veth%d/", devNum);
use snprintf there too
+ }
+
+ snprintf(veth, maxLen, "veth%d", devNum);
+
+ rc = devNum;
+
+ return rc;
+}
+
+/**
+ * vethCreate:
+ * @veth1: name for one end of veth pair
+ * @veth1MaxLen: max length of veth1 name
+ * @veth2: name for one end of veth pair
+ * @veth2MaxLen: max length of veth1 name
+ *
+ * Creates a veth device pair using the ip command:
+ * ip link add veth1 type veth peer name veth2
+ * NOTE: If veth1 and veth2 names are not specified, ip will auto assign
+ * names. There seems to be two problems here -
+ * 1) There doesn't seem to be a way to determine the names of the
+ * devices that it creates. They show up in ip link show and
+ * under /sys/class/net/ however there is no guarantee that they
+ * are the devices that this process just created.
+ * 2) Once one of the veth devices is moved to another namespace, it
+ * is no longer visible in the parent namespace. This seems to
+ * confuse the name assignment causing it to fail with File exists.
+ * Because of these issues, this function currently forces the caller
+ * to fully specify the veth device names.
+ *
+ * Returns 0 on success or -1 in case of error
+ */
+int vethCreate(char* veth1, int veth1MaxLen,
+ char* veth2, int veth2MaxLen)
+{
+ int rc = -1;
+ const char *argv[] = {
I think Jim insists on having const char const * [], but I bet he will explain
this better than me :-)
+ "ip", "link", "add", veth1,
"type", "veth", "peer", "name", veth2, NULL
+ };
[...]
+/**
+ * moveInterfaceToNetNs:
+ * @interface: name of device
+ * @pidInNs: PID of process in target net namespace
+ *
+ * Moves the given device into the target net namespace specified by the given
+ * pid using this command:
+ * ip link set interface netns pidInNs
+ *
+ * Returns 0 on success or -1 in case of error
+ */
+int moveInterfaceToNetNs(const char* interface, int pidInNs)
+{
+ int rc;
+ /* offset of the pid field in the following args */
+ const int pidArgvOffset = 5;
+ const char *argv[] = {
+ "ip", "link", "set", interface, "netns",
NULL, NULL
+ };
+ int cmdResult;
+ int len;
+
+ if (NULL == interface) {
+ goto error_out;
+ }
+
+ if (0 != VIR_ALLOC_N(argv[pidArgvOffset], (sizeof(int) * 3) + 1)) {
+ goto error_out;
+ }
Hum, here i don't understand, if argv is defined as local stack data,
I find a bit confusing to use it in the argument for VIR_ALLOC_N. I would
use an intermediate local variable to make this cleaner/ easier to read.
+ len = snprintf(argv[pidArgvOffset], (sizeof(int) * 3) + 1,
"%d", pidInNs);
yeah I'm getting very confused by that code :-)
+ if (len >= (sizeof(int) * 3) + 1) {
+ goto cleanup;
+ }
+
+ rc = virRun(NULL, (char**)argv, &cmdResult);
+
+ if (0 == rc) {
+ rc = cmdResult;
+ }
+
+cleanup:
+ VIR_FREE(argv[pidArgvOffset]);
+
+error_out:
+ return rc;
+}
+
+#endif /* HAVE_NETNS */
+
diff -r 8d2afc533c91 -r 386c067de899 src/veth.h
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/src/veth.h Thu Jun 19 08:59:24 2008 -0700
@@ -0,0 +1,39 @@
+/*
+ * Copyright IBM Corp. 2008
+ *
+ * veth.h: file description
+ *
+ * Authors:
+ * David L. Leskovec <dlesko at linux.vnet.ibm.com>
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
Oh can you use
/*
* veth.h: .... a proper file description :-) ....
*
* Copyright IBM Corp. 2008
*
* See COPYING.LIB for the License of this software
*
* Authors:
* David L. Leskovec <dlesko at linux.vnet.ibm.com>
*/
as the header template for the two new files to match the other files.
COPYING.LIB is of course LGPL 2.1
thanks !
Daniel
--
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard | virtualization library http://libvirt.org/
veillard(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
12:46 p.m.
New subject: [libvirt] [PATCH 1 of 3] [LXC] Add functions to manage veth device pairs
DV> Hum, it's not only a kernel feature, it looks more like a
DV> dependancy on iproute, maybe that can be checked at runtime when
DV> initializing the lxc module, no ?
I suppose I could actually let it fail when it tries to do the actual
move. Would you rather a specific test during the define stage to
reject the <interface> if the non-netns 'ip' command is found?
> + if (0 != VIR_ALLOC_N(argv[pidArgvOffset], (sizeof(int) * 3)
+ 1)) {
> + goto error_out;
> + }
DV> Hum, here i don't understand, if argv is defined as local stack data,
DV> I find a bit confusing to use it in the argument for VIR_ALLOC_N. I would
DV> use an intermediate local variable to make this cleaner/ easier to read.
> + len = snprintf(argv[pidArgvOffset], (sizeof(int) * 3) + 1,
"%d", pidInNs);
DV> yeah I'm getting very confused by that code :-)
Agreed, I'll fix it up :)
DV> Oh can you use
DV> /*
DV> * veth.h: .... a proper file description :-) ....
DV> *
DV> * Copyright IBM Corp. 2008
DV> *
DV> * See COPYING.LIB for the License of this software
DV> *
DV> * Authors:
DV> * David L. Leskovec <dlesko at linux.vnet.ibm.com>
DV> */
DV> as the header template for the two new files to match the other files.
DV> COPYING.LIB is of course LGPL 2.1
Okay, I'll change it.
Thanks!
--
Dan Smith
IBM Linux Technology Center
Open Hypervisor Team
email: danms(a)us.ibm.com
7:30 a.m.
New subject: [libvirt] [PATCH 1 of 3] [LXC] Add functions to manage veth device pairs
On Thu, Jun 19, 2008 at 10:46:40AM -0700, Dan Smith wrote:
DV> Hum, it's not only a kernel feature, it looks more like a
DV> dependancy on iproute, maybe that can be checked at runtime when
DV> initializing the lxc module, no ?
I suppose I could actually let it fail when it tries to do the actual
move. Would you rather a specific test during the define stage to
reject the <interface> if the non-netns 'ip' command is found?
Well i was wondering if you could not try to detect if the feature
is available for example in lxcProbe() then keep the status.
DV> yeah I'm getting very confused by that code :-)
Agreed, I'll fix it up :)
okay :-)
DV> as the header template for the two new files to match the
other files.
DV> COPYING.LIB is of course LGPL 2.1
Okay, I'll change it.
Cool, thanks !
Daniel
--
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard | virtualization library http://libvirt.org/
veillard(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
4:18 a.m.
New subject: [libvirt] [PATCH 1 of 3] [LXC] Add functions to manage veth device pairs
Daniel Veillard <veillard(a)redhat.com> wrote:
...
> +int vethCreate(char* veth1, int veth1MaxLen,
> + char* veth2, int veth2MaxLen)
> +{
> + int rc = -1;
> + const char *argv[] = {
I think Jim insists on having const char const * [], but I bet he will explain
this better than me :-)
> + "ip", "link", "add", veth1, "type",
"veth", "peer", "name", veth2, NULL
You're right that I encourage the "const correct" approach ;-)
To tell the compiler (and more importantly, the reader) that you
have a const array of const strings, you'd declare it like this:
const char *const argv[] = {
or, equivalently, the const can go after the type name of "char":
char const *const argv[] = {
However, argv is one place where it pays to relax const-correctness
guidelines, at least in C, because so many interfaces require non-const
"char **argv" pointers. IMHO, it's better to avoid const altogether in
this limited case than to be forced to litter the code with ugly and
dangerous const-adjusting casts to accommodate "correctness".
1:11 p.m.
New subject: [libvirt] [PATCH 1 of 3] [LXC] Add functions to manage veth device pairs
JM> However, argv is one place where it pays to relax
JM> const-correctness guidelines, at least in C, because so many
JM> interfaces require non-const "char **argv" pointers. IMHO, it's
JM> better to avoid const altogether in this limited case than to be
JM> forced to litter the code with ugly and dangerous const-adjusting
JM> casts to accommodate "correctness".
I agree with the last statement, but in this case, leaving the const
off the declaration results in a compiler warning about discarding the
inherent const qualifier from a literal string.
A *very* quick look in src/*.c doesn't reveal any other examples of
all-literal argv[] lists like this. I'm inclined to leave the const
and the cast, but if you have a better suggestion, I'll be glad to
make the change.
Thanks!
--
Dan Smith
IBM Linux Technology Center
Open Hypervisor Team
email: danms(a)us.ibm.com
1:05 p.m.
New subject: [libvirt] [PATCH 1 of 3] [LXC] Add functions to manage veth device pairs
DS> # HG changeset patch
DS> # User Dave Leskovec <dlesko(a)linux.vnet.ibm.com>
DS> # Date 1213891164 25200
DS> # Node ID 386c067de8995028dd11f70602081c31682dd293
DS> # Parent 8d2afc533c91c4796512e1e71c8283e86eafd18a
DS> [LXC] Add functions to manage veth device pairs
My apologies for the HG headers. I forgot the --plain when I sent it
out...
--
Dan Smith
IBM Linux Technology Center
Open Hypervisor Team
email: danms(a)us.ibm.com
10:37 a.m.
New subject: [libvirt] [PATCH 1 of 3] [LXC] Add functions to manage veth device pairs
Dan Smith wrote:
# HG changeset patch
# User Dave Leskovec <dlesko(a)linux.vnet.ibm.com>
# Date 1213891164 25200
# Node ID 386c067de8995028dd11f70602081c31682dd293
# Parent 8d2afc533c91c4796512e1e71c8283e86eafd18a
[LXC] Add functions to manage veth device pairs
This gives us the ability to create a veth pair so that we can move one
into the network namespace of an LXC container.
diff -r 8d2afc533c91 -r 386c067de899 configure.in
--- a/configure.in Tue Jun 17 15:55:03 2008 +0000
+++ b/configure.in Thu Jun 19 08:59:24 2008 -0700
@@ -301,6 +301,20 @@
if test "$with_qemu" = "yes" ; then
AC_CHECK_HEADERS([linux/param.h linux/sockios.h linux/if_bridge.h linux/if_tun.h],,
AC_MSG_ERROR([You must install kernel-headers in order to compile
libvirt]))
+fi
+
+dnl
+dnl check for patched iproute2 for lxc network support
+dnl
+if test "$with_lxc" = "yes" ; then
+ AC_MSG_CHECKING([for NETNS support])
+ if ip link help 2>&1 | grep -q netns; then
+ with_lxc_netns="yes"
+ AC_DEFINE([HAVE_NETNS], [], [Kernel has NETNS support])
+ else
+ with_lxc_netns="no"
+ fi
+ AC_MSG_RESULT($with_lxc_netns)
fi
dnl Need to test if pkg-config exists
diff -r 8d2afc533c91 -r 386c067de899 src/Makefile.am
--- a/src/Makefile.am Tue Jun 17 15:55:03 2008 +0000
+++ b/src/Makefile.am Thu Jun 19 08:59:24 2008 -0700
@@ -64,6 +64,7 @@
lxc_driver.c lxc_driver.h \
lxc_conf.c lxc_conf.h \
lxc_container.c lxc_container.h \
+ veth.c veth.h \
nodeinfo.h nodeinfo.c \
util.c util.h
diff -r 8d2afc533c91 -r 386c067de899 src/veth.c
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/src/veth.c Thu Jun 19 08:59:24 2008 -0700
@@ -0,0 +1,247 @@
+/*
+ * Copyright IBM Corp. 2008
+ *
+ * veth.c: file description
+ *
+ * Authors:
+ * David L. Leskovec <dlesko at linux.vnet.ibm.com>
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+
+#include <config.h>
+
+#ifdef HAVE_NETNS
+
+#include <string.h>
+
+#include "veth.h"
+#include "internal.h"
+#include "memory.h"
+#include "util.h"
+
+#define DEBUG(fmt,...) VIR_DEBUG(__FILE__, fmt, __VA_ARGS__)
+#define DEBUG0(msg) VIR_DEBUG(__FILE__, "%s", msg)
Do you know ##__VA_ARGS ?
+/* Functions */
+/**
+ * getFreeVethName:
+ * @veth: name for veth device (NULL to find first open)
+ * @maxLen: max length of veth name
+ * @startDev: device number to start at (x in vethx)
+ *
+ * Looks in /sys/class/net/ to find the first available veth device
+ * name.
+ *
+ * Returns 0 on success or -1 in case of error
+ */
+static int getFreeVethName(char *veth, int maxLen, int startDev)
+{
+ int rc = -1;
+ int devNum = startDev;
+ char path[PATH_MAX];
+
+ snprintf(path, PATH_MAX, "/sys/class/net/veth%d/", devNum);
You can perhaps, use do { ... } while () here.
+ while (virFileExists(path)) {
+ ++devNum;
+ sprintf(path, "/sys/class/net/veth%d/", devNum);
+ }
Is this function safe for concurrent access ? eg. getFreeVethName called
in parallel by two processes or another process creates a pair device
just after you exit the loop ?
+ snprintf(veth, maxLen, "veth%d", devNum);
+
+ rc = devNum;
+
+ return rc;
+}
+
+/**
+ * vethCreate:
+ * @veth1: name for one end of veth pair
+ * @veth1MaxLen: max length of veth1 name
+ * @veth2: name for one end of veth pair
+ * @veth2MaxLen: max length of veth1 name
+ *
+ * Creates a veth device pair using the ip command:
+ * ip link add veth1 type veth peer name veth2
+ * NOTE: If veth1 and veth2 names are not specified, ip will auto assign
+ * names. There seems to be two problems here -
+ * 1) There doesn't seem to be a way to determine the names of the
+ * devices that it creates. They show up in ip link show and
+ * under /sys/class/net/ however there is no guarantee that they
+ * are the devices that this process just created.
+ * 2) Once one of the veth devices is moved to another namespace, it
+ * is no longer visible in the parent namespace. This seems to
+ * confuse the name assignment causing it to fail with File exists.
+ * Because of these issues, this function currently forces the caller
+ * to fully specify the veth device names.
+ *
+ * Returns 0 on success or -1 in case of error
+ */
+int vethCreate(char* veth1, int veth1MaxLen,
+ char* veth2, int veth2MaxLen)
No need of the veth1MaxLen parameter, you already have it, it is
IF_NAMESIZE.
+{
+ int rc = -1;
+ const char *argv[] = {
+ "ip", "link", "add", veth1, "type",
"veth", "peer", "name", veth2, NULL
+ };
+ int cmdResult;
+ int vethDev = 0;
+
+ if ((NULL == veth1) || (NULL == veth2)) {
+ goto error_out;
+ }
+
+ DEBUG("veth1: %s veth2: %s", veth1, veth2);
+
+ if (1 > strlen(veth1)) {
Why do you check with strlen > 1 ?
+ vethDev = getFreeVethName(veth1, veth1MaxLen, 0);
+ ++vethDev;
+ DEBUG("assigned veth1: %s", veth1);
+ }
+
+ if (1 > strlen(veth2)) {
+ vethDev = getFreeVethName(veth2, veth2MaxLen, vethDev);
+ DEBUG("assigned veth2: %s", veth2);
+ }
+
+ rc = virRun(NULL, (char**)argv, &cmdResult);
+
+ if (0 == rc) {
+ rc = cmdResult;
+ }
+
+error_out:
+ return rc;
+}
+
+/**
+ * vethDelete:
+ * @veth: name for one end of veth pair
+ *
+ * This will delete both veth devices in a pair. Only one end needs to
+ * be specified. The ip command will identify and delete the other veth
+ * device as well.
+ * ip link del veth
+ *
+ * Returns 0 on success or -1 in case of error
+ */
+int vethDelete(const char *veth)
+{
+ int rc = -1;
+ const char *argv[] = {"ip", "link", "del", veth,
NULL};
+ int cmdResult;
+
+ if (NULL == veth) {
+ goto error_out;
+ }
+
+ DEBUG("veth: %s", veth);
+
+ rc = virRun(NULL, (char**)argv, &cmdResult);
+
+ if (0 == rc) {
+ rc = cmdResult;
+ }
+
+error_out:
+ return rc;
+}
+
+/**
+ * vethInterfaceUpOrDown:
+ * @veth: name of veth device
+ * @upOrDown: 0 => down, 1 => up
+ *
+ * Enables a veth device using the ifconfig command. A NULL inetAddress
+ * will cause it to be left off the command line.
+ *
+ * Returns 0 on success or -1 in case of error
+ */
+int vethInterfaceUpOrDown(const char* veth, int upOrDown)
+{
+ int rc = -1;
+ char upOrDownString[8];
+ const char *argv[] = {"ifconfig", veth, upOrDownString, NULL};
+ int cmdResult;
+
+ if (NULL == veth) {
+ goto error_out;
+ }
+
+ if (0 == upOrDown) {
+ strcpy(upOrDownString, "down");
+ } else {
+ strcpy(upOrDownString, "up");
+ }
You don't need to copy the string, a const char *upOrDownString and
upOrDownString = "down" will work.
+
+ rc = virRun(NULL, (char**)argv, &cmdResult);
+
+ if (0 == rc) {
+ rc = cmdResult;
+ }
+
+error_out:
+ return rc;
+}
+
+/**
+ * moveInterfaceToNetNs:
+ * @interface: name of device
+ * @pidInNs: PID of process in target net namespace
+ *
+ * Moves the given device into the target net namespace specified by the given
+ * pid using this command:
+ * ip link set interface netns pidInNs
+ *
+ * Returns 0 on success or -1 in case of error
+ */
+int moveInterfaceToNetNs(const char* interface, int pidInNs)
+{
+ int rc;
+ /* offset of the pid field in the following args */
+ const int pidArgvOffset = 5;
+ const char *argv[] = {
+ "ip", "link", "set", interface, "netns",
NULL, NULL
+ };
+ int cmdResult;
+ int len;
+
+ if (NULL == interface) {
+ goto error_out;
+ }
+
+ if (0 != VIR_ALLOC_N(argv[pidArgvOffset], (sizeof(int) * 3) + 1)) {
+ goto error_out;
+ }
+ len = snprintf(argv[pidArgvOffset], (sizeof(int) * 3) + 1, "%d",
pidInNs);
+ if (len >= (sizeof(int) * 3) + 1) {
+ goto cleanup;
+ }
Why don't you just do:
char pidstr[PIDSTRLEN];
const char *argv[] = { "ip", "link", "set", interface,
"netns", pidstr,
NULL };
snprintf(pidstr, PIDSTRLEN, "%d", pidInNs);
That should work, no ?
+
+ rc = virRun(NULL, (char**)argv, &cmdResult);
+
+ if (0 == rc) {
+ rc = cmdResult;
+ }
+
+cleanup:
+ VIR_FREE(argv[pidArgvOffset]);
+
+error_out:
+ return rc;
+}
+
+#endif /* HAVE_NETNS */
+
diff -r 8d2afc533c91 -r 386c067de899 src/veth.h
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/src/veth.h Thu Jun 19 08:59:24 2008 -0700
@@ -0,0 +1,39 @@
+/*
+ * Copyright IBM Corp. 2008
+ *
+ * veth.h: file description
+ *
+ * Authors:
+ * David L. Leskovec <dlesko at linux.vnet.ibm.com>
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+
+#ifndef VETH_H
+#define VETH_H
+
+#include <config.h>
+
+#ifdef HAVE_NETNS
+
+/* Function declarations */
+int vethCreate(char* veth1, int veth1MaxLen, char* veth2,
+ int veth2MaxLen);
+int vethDelete(const char* veth);
+int vethInterfaceUpOrDown(const char* veth, int upOrDown);
+int moveInterfaceToNetNs(const char *interface, int pidInNs);
+
+#endif /* HAVE_NETNS */
+#endif /* VETH_H */
12:52 p.m.
New subject: [libvirt] [PATCH 1 of 3] [LXC] Add functions to manage veth device pairs
> +#define DEBUG(fmt,...) VIR_DEBUG(__FILE__, fmt, __VA_ARGS__)
> +#define DEBUG0(msg) VIR_DEBUG(__FILE__, "%s", msg)
DL> Do you know ##__VA_ARGS ?
Yes, and I agree this not my preferred way. However, this isn't my
code and when I went to look for other instances, I found many so I
decided not to change it.
% cat *.c | grep DEBUG0 -c
47
DL> Is this function safe for concurrent access ? eg. getFreeVethName
DL> called in parallel by two processes or another process creates a
DL> pair device just after you exit the loop ?
No, there is indeed a race. However, since this just makes a
suggestion and relies on the subsequent call to try to instantiate the
interface, I don't think there's a chance for anything other than a
spurious failure.
This is specifically why I wanted to get this on the list sooner than
later. I figured (or hoped) that given the experience in this group
that someone could offer some educated suggestions before I jumped in
and started trying to fix it.
> + if (1 > strlen(veth1)) {
DL> Why do you check with strlen > 1 ?
To be honest, I have no idea why Dave did that. I'll check with him
to see if there's something we're missing. It's probably worth a
comment if so.
--
Dan Smith
IBM Linux Technology Center
Open Hypervisor Team
email: danms(a)us.ibm.com
3:55 p.m.
New subject: [libvirt] [PATCH 1 of 3] [LXC] Add functions to manage veth device pairs
Dan Smith wrote:
>> + if (1 > strlen(veth1)) {
DL> Why do you check with strlen > 1 ?
To be honest, I have no idea why Dave did that. I'll check with him
to see if there's something we're missing. It's probably worth a
comment if so.
So that's checking if veth1 is an empty string and hence needs to have a name
assigned. I don't recall any special reason for using (1 > strlen()).
--
Best Regards,
Dave Leskovec
IBM Linux Technology Center
Open Virtualization
10:59 a.m.
New subject: [libvirt] [PATCH 2 of 3] [LXC] Add XML parsing of container network interfaces
# HG changeset patch
# User Dave Leskovec <dlesko(a)linux.vnet.ibm.com>
# Date 1213891177 25200
# Node ID acf369a2543ad52b235ae8541c8ad05670e255bd
# Parent 386c067de8995028dd11f70602081c31682dd293
[LXC] Add XML parsing of container network interfaces.
diff -r 386c067de899 -r acf369a2543a src/lxc_conf.c
--- a/src/lxc_conf.c Thu Jun 19 08:59:24 2008 -0700
+++ b/src/lxc_conf.c Thu Jun 19 08:59:37 2008 -0700
@@ -70,6 +70,191 @@
codeErrorMessage, errorMessage, NULL, 0, 0,
codeErrorMessage, errorMessage);
}
+
+#ifdef HAVE_NETNS
+/**
+ * lxcParseInterfaceXML:
+ * @conn: pointer to connection
+ * @nodePtr: pointer to xml node structure
+ * @vm: pointer to net definition structure to fill in
+ *
+ * Parses the XML for a network interface and places the configuration
+ * in the given structure.
+ *
+ * Returns 0 on success or -1 in case of error
+ */
+static int lxcParseInterfaceXML(virConnectPtr conn, xmlNodePtr nodePtr,
+ lxc_net_def_t *netDef)
+{
+ int rc = -1;
+ xmlNodePtr cur;
+ xmlChar *type = NULL;
+ xmlChar *parentIfName = NULL;
+ xmlChar *network = NULL;
+ xmlChar *bridge = NULL;
+ xmlChar *macaddr = NULL;
+
+ netDef->type = LXC_NET_NETWORK;
+
+ type = xmlGetProp(nodePtr, BAD_CAST "type");
+ if (type != NULL) {
+ if (xmlStrEqual(type, BAD_CAST "network")) {
+ netDef->type = LXC_NET_NETWORK;
+ }
+ else if (xmlStrEqual(type, BAD_CAST "bridge")) {
+ netDef->type = LXC_NET_BRIDGE;
+ }
+ else {
+ lxcError(conn, NULL, VIR_ERR_XML_ERROR,
+ _("invalid interface type: %s"), type);
+ goto error_out;
+ }
+ }
+
+ cur = nodePtr->children;
+ for (cur = nodePtr->children; cur != NULL; cur = cur->next) {
+ if (cur->type == XML_ELEMENT_NODE) {
+ DEBUG("cur->name: %s", (char*)(cur->name));
+ if ((macaddr == NULL) &&
+ (xmlStrEqual(cur->name, BAD_CAST "mac"))) {
+ macaddr = xmlGetProp(cur, BAD_CAST "address");
+ } else if ((network == NULL) &&
+ (netDef->type == LXC_NET_NETWORK) &&
+ (xmlStrEqual(cur->name, BAD_CAST "source"))) {
+ network = xmlGetProp(cur, BAD_CAST "network");
+ parentIfName = xmlGetProp(cur, BAD_CAST "dev");
+ } else if ((bridge == NULL) &&
+ (netDef->type == LXC_NET_BRIDGE) &&
+ (xmlStrEqual(cur->name, BAD_CAST "source"))) {
+ bridge = xmlGetProp(cur, BAD_CAST "bridge");
+ } else if ((parentIfName == NULL) &&
+ (xmlStrEqual(cur->name, BAD_CAST "target"))) {
+ parentIfName = xmlGetProp(cur, BAD_CAST "dev");
+ }
+ }
+ }
+
+ if (netDef->type == LXC_NET_NETWORK) {
+ if (network == NULL) {
+ lxcError(conn, NULL, VIR_ERR_XML_ERROR,
+ _("No <source> 'network' attribute specified with
<interface type='network'/>"));
+ goto error_out;
+ }
+
+ netDef->txName = strdup((char *)network);
+ if (NULL == netDef->txName) {
+ lxcError(conn, NULL, VIR_ERR_NO_MEMORY,
+ _("No storage for network name"));
+ goto error_out;
+ }
+
+ } else if (netDef->type == LXC_NET_BRIDGE) {
+ if (bridge == NULL) {
+ lxcError(conn, NULL, VIR_ERR_XML_ERROR,
+ _("No <source> 'bridge' attribute specified with
<interface type='bridge'/>"));
+ goto error_out;
+ }
+
+ netDef->txName = strdup((char *)bridge);
+ if (NULL == netDef->txName) {
+ lxcError(conn, NULL, VIR_ERR_NO_MEMORY,
+ _("No storage for bridge name"));
+ goto error_out;
+ }
+ }
+
+ if (parentIfName != NULL) {
+ DEBUG("set netDef->parentVeth: %s", netDef->parentVeth);
+ netDef->parentVeth = strdup((char *)parentIfName);
+ if (NULL == netDef->parentVeth) {
+ lxcError(conn, NULL, VIR_ERR_NO_MEMORY,
+ _("No storage for parent veth device name"));
+ goto error_out;
+ }
+ } else {
+ netDef->parentVeth = NULL;
+ DEBUG0("set netDef->parentVeth: NULL");
+ }
+
+ rc = 0;
+
+error_out:
+ xmlFree(macaddr);
+ macaddr = NULL;
+ xmlFree(network);
+ network = NULL;
+ xmlFree(bridge);
+ bridge = NULL;
+ xmlFree(parentIfName);
+ parentIfName = NULL;
+
+ return rc;
+}
+
+/**
+ * lxcParseDomainInterfaces:
+ * @conn: pointer to connection
+ * @nets: on success, points to an list of net def structs
+ * @contextPtr: pointer to xml context
+ *
+ * Parses the domain network interfaces and returns the information in a list
+ *
+ * Returns 0 on success or -1 in case of error
+ */
+static int lxcParseDomainInterfaces(virConnectPtr conn,
+ lxc_net_def_t **nets,
+ xmlXPathContextPtr contextPtr)
+{
+ int rc = -1;
+ int i;
+ lxc_net_def_t *netDef;
+ lxc_net_def_t *prevDef = NULL;
+ int numNets = 0;
+ xmlNodePtr *list;
+ int res;
+
+ DEBUG0("parsing nets");
+
+ res = virXPathNodeSet("/domain/devices/interface", contextPtr, &list);
+ if (res > 0) {
+ for (i = 0; i < res; ++i) {
+ netDef = calloc(1, sizeof(lxc_net_def_t));
+ if (NULL == netDef) {
+ lxcError(conn, NULL, VIR_ERR_NO_MEMORY,
+ _("No storage for net def structure"));
+ goto parse_complete;
+ }
+
+ rc = lxcParseInterfaceXML(conn, list[i], netDef);
+ if (0 > rc) {
+ DEBUG("failed parsing a net: %d", rc);
+
+ free(netDef);
+ goto parse_complete;
+ }
+
+ DEBUG0("parsed a net");
+
+ /* set the linked list pointers */
+ numNets++;
+ netDef->next = NULL;
+ if (0 == i) {
+ *nets = netDef;
+ } else {
+ prevDef->next = netDef;
+ }
+ prevDef = netDef;
+ }
+ free(list);
+ }
+
+ rc = numNets;
+
+parse_complete:
+ DEBUG("parsed %d nets", rc);
+ return rc;
+}
+#endif /* HAVE_NETNS */
static int lxcParseMountXML(virConnectPtr conn, xmlNodePtr nodePtr,
lxc_mount_t *lxcMount)
@@ -374,6 +559,15 @@
if (0 > containerDef->nmounts) {
goto error;
}
+
+#ifdef HAVE_NETNS
+ containerDef->numNets = lxcParseDomainInterfaces(conn,
+ &(containerDef->nets),
+ contextPtr);
+ if (0 > containerDef->numNets) {
+ goto error;
+ }
+#endif
if (lxcParseDomainTty(conn, &(containerDef->tty), contextPtr) < 0) {
goto error;
@@ -741,6 +935,7 @@
unsigned char *uuid;
char uuidstr[VIR_UUID_STRING_BUFLEN];
lxc_mount_t *mount;
+ lxc_net_def_t *net;
if (lxcIsActiveVM(vm))
virBufferVSprintf(&buf, "<domain type='%s'
id='%d'>\n",
@@ -770,6 +965,27 @@
virBufferAddLit(&buf, " </filesystem>\n");
}
+ /* loop adding nets */
+ for (net = def->nets; net; net = net->next) {
+ if (net->type == LXC_NET_NETWORK) {
+ virBufferAddLit(&buf, " <interface
type='network'>\n");
+ virBufferVSprintf(&buf, " <source
network='%s'/>\n",
+ net->txName);
+ } else {
+ virBufferAddLit(&buf, " <interface
type='bridge'>\n");
+ virBufferVSprintf(&buf, " <source
bridge='%s'/>\n",
+ net->txName);
+ }
+
+ if (NULL != net->parentVeth) {
+ virBufferVSprintf(&buf, " <target
dev='%s'/>\n",
+ net->parentVeth);
+ }
+
+ virBufferAddLit(&buf, " </interface>\n");
+
+ }
+
virBufferVSprintf(&buf, " <console tty='%s'/>\n",
def->tty);
virBufferAddLit(&buf, " </devices>\n");
virBufferAddLit(&buf, "</domain>\n");
@@ -786,6 +1002,8 @@
{
lxc_mount_t *curMount;
lxc_mount_t *nextMount;
+ lxc_net_def_t *curNet;
+ lxc_net_def_t *nextNet;
if (vmdef == NULL)
return;
@@ -795,6 +1013,17 @@
nextMount = curMount->next;
VIR_FREE(curMount);
curMount = nextMount;
+ }
+
+ curNet = vmdef->nets;
+ while (curNet) {
+ nextNet = curNet->next;
+ printf("Freeing %s:%s\n", curNet->parentVeth,
curNet->containerVeth);
+ VIR_FREE(curNet->parentVeth);
+ VIR_FREE(curNet->containerVeth);
+ VIR_FREE(curNet->txName);
+ VIR_FREE(curNet);
+ curNet = nextNet;
}
VIR_FREE(vmdef->name);
diff -r 386c067de899 -r acf369a2543a src/lxc_conf.h
--- a/src/lxc_conf.h Thu Jun 19 08:59:24 2008 -0700
+++ b/src/lxc_conf.h Thu Jun 19 08:59:37 2008 -0700
@@ -36,6 +36,22 @@
#define LXC_MAX_ERROR_LEN 1024
#define LXC_DOMAIN_TYPE "lxc"
+/* types of networks for containers */
+enum lxc_net_type {
+ LXC_NET_NETWORK,
+ LXC_NET_BRIDGE
+};
+
+typedef struct __lxc_net_def lxc_net_def_t;
+struct __lxc_net_def {
+ int type;
+ char *parentVeth; /* veth device in parent namespace */
+ char *containerVeth; /* veth device in container namespace */
+ char *txName; /* bridge or network name */
+
+ lxc_net_def_t *next;
+};
+
typedef struct __lxc_mount lxc_mount_t;
struct __lxc_mount {
char source[PATH_MAX]; /* user's directory */
@@ -61,6 +77,10 @@
/* tty device */
char *tty;
+
+ /* network devices */
+ int numNets;
+ lxc_net_def_t *nets;
};
typedef struct __lxc_vm lxc_vm_t;
10:59 a.m.
New subject: [libvirt] [PATCH 3 of 3] [LXC] Add setup/cleanup of container network interfaces
# HG changeset patch
# User Dan Smith <danms(a)us.ibm.com>
# Date 1213891185 25200
# Node ID cb780a7b3ad591f1a9392d6528218b3aa2c3483d
# Parent acf369a2543ad52b235ae8541c8ad05670e255bd
[LXC] Add setup/cleanup of container network interfaces
diff -r acf369a2543a -r cb780a7b3ad5 src/lxc_conf.h
--- a/src/lxc_conf.h Thu Jun 19 08:59:37 2008 -0700
+++ b/src/lxc_conf.h Thu Jun 19 08:59:45 2008 -0700
@@ -35,6 +35,12 @@
#define LXC_MAX_XML_LENGTH 16384
#define LXC_MAX_ERROR_LEN 1024
#define LXC_DOMAIN_TYPE "lxc"
+#define LXC_PARENT_SOCKET 0
+#define LXC_CONTAINER_SOCKET 1
+
+/* messages between parent and container */
+typedef char lxc_message_t;
+#define LXC_CONTINUE_MSG 'c'
/* types of networks for containers */
enum lxc_net_type {
@@ -96,6 +102,8 @@
int parentTty;
int containerTtyFd;
char *containerTty;
+
+ int sockpair[2];
lxc_vm_def_t *def;
diff -r acf369a2543a -r cb780a7b3ad5 src/lxc_container.c
--- a/src/lxc_container.c Thu Jun 19 08:59:37 2008 -0700
+++ b/src/lxc_container.c Thu Jun 19 08:59:45 2008 -0700
@@ -36,6 +36,7 @@
#include "lxc_conf.h"
#include "util.h"
#include "memory.h"
+#include "veth.h"
#define DEBUG(fmt,...) VIR_DEBUG(__FILE__, fmt, __VA_ARGS__)
#define DEBUG0(msg) VIR_DEBUG(__FILE__, "%s", msg)
@@ -159,6 +160,74 @@
}
/**
+ * lxcWaitForContinue:
+ * @vm: Pointer to vm structure
+ *
+ * This function will wait for the container continue message from the
+ * parent process. It will send this message on the socket pair stored in
+ * the vm structure once it has completed the post clone container setup.
+ *
+ * Returns 0 on success or -1 in case of error
+ */
+static int lxcWaitForContinue(lxc_vm_t *vm)
+{
+ int rc = -1;
+ lxc_message_t msg;
+ int readLen = 0;
+
+ readLen = saferead(vm->sockpair[LXC_CONTAINER_SOCKET], &msg, sizeof(msg));
+ if (readLen != sizeof(msg)) {
+ lxcError(NULL, NULL, VIR_ERR_INTERNAL_ERROR,
+ _("Failed to read the container continue message: %s"),
+ strerror(errno));
+ goto error_out;
+ }
+
+ DEBUG0("Received container continue message");
+
+ close(vm->sockpair[LXC_PARENT_SOCKET]);
+ vm->sockpair[LXC_PARENT_SOCKET] = -1;
+ close(vm->sockpair[LXC_CONTAINER_SOCKET]);
+ vm->sockpair[LXC_CONTAINER_SOCKET] = -1;
+
+ rc = 0;
+
+error_out:
+ return rc;
+}
+
+#ifdef HAVE_NETNS
+/**
+ * lxcEnableInterfaces:
+ * @vm: Pointer to vm structure
+ *
+ * This function will enable the interfaces for this container.
+ *
+ * Returns 0 on success or -1 in case of error
+ */
+static int lxcEnableInterfaces(lxc_vm_t *vm)
+{
+ int rc = -1;
+ lxc_net_def_t *net = vm->def->nets;
+ int i = 0;
+
+ for (i = 0; net; net = net->next) {
+ DEBUG("Enabling %s", net->containerVeth);
+ rc = vethInterfaceUpOrDown(net->containerVeth, 1);
+ if (0 != rc) {
+ goto error_out;
+ }
+ }
+
+ /* enable lo device */
+ rc = vethInterfaceUpOrDown("lo", 1);
+
+error_out:
+ return rc;
+}
+#endif /* HAVE_NETNS */
+
+/**
* lxcChild:
* @argv: Pointer to container arguments
*
@@ -210,6 +279,18 @@
goto cleanup;
}
+ /* Wait for interface devices to show up */
+ if (0 != (rc = lxcWaitForContinue(vm))) {
+ goto cleanup;
+ }
+
+#ifdef HAVE_NETNS
+ /* enable interfaces */
+ if (0 != (rc = lxcEnableInterfaces(vm))) {
+ goto cleanup;
+ }
+#endif
+
rc = lxcExecWithTty(vm);
/* this function will only return if an error occured */
diff -r acf369a2543a -r cb780a7b3ad5 src/lxc_driver.c
--- a/src/lxc_driver.c Thu Jun 19 08:59:37 2008 -0700
+++ b/src/lxc_driver.c Thu Jun 19 08:59:45 2008 -0700
@@ -44,6 +44,9 @@
#include "memory.h"
#include "util.h"
#include "memory.h"
+#include "bridge.h"
+#include "qemu_conf.h"
+#include "veth.h"
/* debug macros */
#define DEBUG(fmt,...) VIR_DEBUG(__FILE__, fmt, __VA_ARGS__)
@@ -66,6 +69,9 @@
#ifndef CLONE_NEWIPC
#define CLONE_NEWIPC 0x08000000
#endif
+#ifndef CLONE_NEWNET
+#define CLONE_NEWNET 0x40000000 /* New network namespace */
+#endif
static int lxcStartup(void);
static int lxcShutdown(void);
@@ -81,6 +87,9 @@
{
int rc = 0;
int flags = CLONE_NEWPID|CLONE_NEWNS|CLONE_NEWUTS|CLONE_NEWUSER|
+#ifdef HAVE_NETNS
+ CLONE_NEWNET|
+#endif
CLONE_NEWIPC|SIGCHLD;
int cpid;
char *childStack;
@@ -237,6 +246,9 @@
static int lxcNumDomains(virConnectPtr conn)
{
lxc_driver_t *driver = (lxc_driver_t *)conn->privateData;
+
+ DEBUG("driver: %p network: %p", conn->privateData,
conn->networkPrivateData);
+
return driver->nactivevms;
}
@@ -384,6 +396,197 @@
return lxcGenerateXML(dom->conn, driver, vm, vm->def);
}
+#ifdef HAVE_NETNS
+/**
+ * lxcSetupInterfaces:
+ * @conn: pointer to connection
+ * @vm: pointer to virtual machine structure
+ *
+ * Sets up the container interfaces by creating the veth device pairs and
+ * attaching the parent end to the appropriate bridge. The container end
+ * will moved into the container namespace later after clone has been called.
+ *
+ * Returns 0 on success or -1 in case of error
+ */
+static int lxcSetupInterfaces(virConnectPtr conn,
+ lxc_vm_t *vm)
+{
+ int rc = -1;
+ struct qemud_driver *networkDriver =
+ (struct qemud_driver *)(conn->networkPrivateData);
+ lxc_net_def_t *net = vm->def->nets;
+ int i = 0;
+ char* bridge;
+ char parentVeth[PATH_MAX] = "";
+ char containerVeth[PATH_MAX] = "";
+
+ for (i = 0; net; net = net->next) {
+ if (LXC_NET_NETWORK == net->type) {
+ virNetworkPtr network = virNetworkLookupByName(conn, net->txName);
+ if (!network) {
+ goto error_exit;
+ }
+
+ bridge = virNetworkGetBridgeName(network);
+
+ virNetworkFree(network);
+
+ } else {
+ bridge = net->txName;
+ }
+
+ DEBUG("bridge: %s", bridge);
+ if (NULL == bridge) {
+ lxcError(conn, NULL, VIR_ERR_INTERNAL_ERROR,
+ _("failed to get bridge for interface"));
+ goto error_exit;
+ }
+
+ DEBUG0("calling vethCreate()");
+ if (NULL != net->parentVeth) {
+ strcpy(parentVeth, net->parentVeth);
+ }
+ if (NULL != net->containerVeth) {
+ strcpy(containerVeth, net->containerVeth);
+ }
+ DEBUG("parentVeth: %s, containerVeth: %s", parentVeth, containerVeth);
+ if (0 != (rc = vethCreate(parentVeth, PATH_MAX, containerVeth, PATH_MAX))) {
+ lxcError(conn, NULL, VIR_ERR_INTERNAL_ERROR,
+ _("failed to create veth device pair: %d"), rc);
+ goto error_exit;
+ }
+ if (NULL == net->parentVeth) {
+ net->parentVeth = strdup(parentVeth);
+ }
+ if (NULL == net->containerVeth) {
+ net->containerVeth = strdup(containerVeth);
+ }
+
+ if (!(networkDriver->brctl) && (rc =
brInit(&(networkDriver->brctl)))) {
+ lxcError(conn, NULL, VIR_ERR_INTERNAL_ERROR,
+ _("cannot initialize bridge support: %s"),
+ strerror(rc));
+ goto error_exit;
+ }
+
+ if (0 != (rc = brAddInterface(networkDriver->brctl, bridge, parentVeth))) {
+ lxcError(conn, NULL, VIR_ERR_INTERNAL_ERROR,
+ _("failed to add %s device to %s: %s"),
+ parentVeth,
+ bridge,
+ strerror(rc));
+ goto error_exit;
+ }
+
+ if (0 != (rc = vethInterfaceUpOrDown(parentVeth, 1))) {
+ lxcError(conn, NULL, VIR_ERR_INTERNAL_ERROR,
+ _("failed to enable parent ns veth device: %d"), rc);
+ goto error_exit;
+ }
+
+ }
+
+ rc = 0;
+
+error_exit:
+ return rc;
+}
+
+/**
+ * lxcMoveInterfacesToNetNs:
+ * @conn: pointer to connection
+ * @vm: pointer to virtual machine structure
+ *
+ * Starts a container process by calling clone() with the namespace flags
+ *
+ * Returns 0 on success or -1 in case of error
+ */
+static int lxcMoveInterfacesToNetNs(virConnectPtr conn,
+ lxc_vm_t *vm)
+{
+ int rc = -1;
+ lxc_net_def_t *net = vm->def->nets;
+ int i = 0;
+
+ for (i = 0; net; net = net->next) {
+ if (0 != moveInterfaceToNetNs(net->containerVeth, vm->def->id)) {
+ lxcError(conn, NULL, VIR_ERR_INTERNAL_ERROR,
+ _("failed to move interface %s to ns %d"),
+ net->containerVeth, vm->def->id);
+ goto error_exit;
+ }
+ }
+
+ rc = 0;
+
+error_exit:
+ return rc;
+}
+
+/**
+ * lxcCleanupInterfaces:
+ * @conn: pointer to connection
+ * @vm: pointer to virtual machine structure
+ *
+ * Cleans up the container interfaces by deleting the veth device pairs.
+ *
+ * Returns 0 on success or -1 in case of error
+ */
+static int lxcCleanupInterfaces(lxc_vm_t *vm)
+{
+ int rc = -1;
+ lxc_net_def_t *net = vm->def->nets;
+ int i = 0;
+
+ for (i = 0; net; net = net->next) {
+ if (0 != (rc = vethDelete(net->parentVeth))) {
+ lxcError(NULL, NULL, VIR_ERR_INTERNAL_ERROR,
+ _("failed to delete veth: %s"), net->parentVeth);
+ /* will continue to try to cleanup any other interfaces */
+ }
+
+ }
+
+ rc = 0;
+
+ return rc;
+}
+#endif /* HAVE_NETNS */
+
+/**
+ * lxcSendContainerContinue:
+ * @vm: pointer to virtual machine structure
+ *
+ * Sends the continue message via the socket pair stored in the vm
+ * structure.
+ *
+ * Returns 0 on success or -1 in case of error
+ */
+static int lxcSendContainerContinue(lxc_vm_t *vm)
+{
+ int rc = -1;
+ lxc_message_t msg = LXC_CONTINUE_MSG;
+ int writeCount = 0;
+
+ if (NULL == vm) {
+ goto error_out;
+ }
+
+ writeCount = safewrite(vm->sockpair[LXC_PARENT_SOCKET], &msg,
+ sizeof(msg));
+ if (writeCount != sizeof(msg)) {
+ lxcError(NULL, NULL, VIR_ERR_INTERNAL_ERROR,
+ _("unable to send container continue message: %s"),
+ strerror(errno));
+ goto error_out;
+ }
+
+ rc = 0;
+
+error_out:
+ return rc;
+}
+
/**
* lxcStartContainer:
* @conn: pointer to connection
@@ -411,7 +614,11 @@
}
stacktop = stack + stacksize;
- flags = CLONE_NEWPID|CLONE_NEWNS|CLONE_NEWUTS|CLONE_NEWUSER|CLONE_NEWIPC|SIGCHLD;
+ flags = CLONE_NEWPID|CLONE_NEWNS|CLONE_NEWUTS|CLONE_NEWUSER|
+#ifdef HAVE_NETNS
+ CLONE_NEWNET|
+#endif
+ CLONE_NEWIPC|SIGCHLD;
vm->def->id = clone(lxcChild, stacktop, flags, (void *)vm);
@@ -809,7 +1016,34 @@
close(vm->parentTty);
close(vm->containerTtyFd);
+#ifdef HAVE_NETNS
+ if (0 != (rc = lxcSetupInterfaces(conn, vm))) {
+ goto cleanup;
+ }
+#endif /* HAVE_NETNS */
+
+ /* create a socket pair to send continue message to the container once */
+ /* we've completed the post clone configuration */
+ if (0 != socketpair(PF_UNIX, SOCK_STREAM, 0, vm->sockpair)) {
+ lxcError(conn, NULL, VIR_ERR_INTERNAL_ERROR,
+ _("sockpair failed: %s"), strerror(errno));
+ goto cleanup;
+ }
+
+ /* check this rc */
+
rc = lxcStartContainer(conn, driver, vm);
+
+#ifdef HAVE_NETNS
+ rc = lxcMoveInterfacesToNetNs(conn, vm);
+#endif
+
+ rc = lxcSendContainerContinue(vm);
+
+ close(vm->sockpair[LXC_PARENT_SOCKET]);
+ vm->sockpair[LXC_PARENT_SOCKET] = -1;
+ close(vm->sockpair[LXC_CONTAINER_SOCKET]);
+ vm->sockpair[LXC_CONTAINER_SOCKET] = -1;
if (rc == 0) {
vm->state = VIR_DOMAIN_RUNNING;
@@ -948,6 +1182,11 @@
int waitRc;
int childStatus = -1;
+ /* if this fails, we'll continue. it will report any errors */
+#ifdef HAVE_NETNS
+ lxcCleanupInterfaces(vm);
+#endif
+
while (((waitRc = waitpid(vm->def->id, &childStatus, 0)) == -1) &&
errno == EINTR);
11:23 a.m.
New subject: [libvirt] [PATCH 3 of 3] [LXC] Add setup/cleanup of container network interfaces
Hi Dan,
Here are a few suggestions.
The bits about strdup are more substantive.
+static int lxcWaitForContinue(lxc_vm_t *vm)
...
+ int rc = -1;
+ lxc_message_t msg;
+ int readLen = 0;
Don't initialize readLen in the declaration,
since it's set unconditionally just below.
+ readLen = saferead(vm->sockpair[LXC_CONTAINER_SOCKET],
&msg, sizeof(msg));
+ if (readLen != sizeof(msg)) {
+ lxcError(NULL, NULL, VIR_ERR_INTERNAL_ERROR,
+ _("Failed to read the container continue message: %s"),
+ strerror(errno));
+ goto error_out;
+ }
+
+ DEBUG0("Received container continue message");
+
+ close(vm->sockpair[LXC_PARENT_SOCKET]);
+ vm->sockpair[LXC_PARENT_SOCKET] = -1;
+ close(vm->sockpair[LXC_CONTAINER_SOCKET]);
+ vm->sockpair[LXC_CONTAINER_SOCKET] = -1;
+
+ rc = 0;
+
+error_out:
+ return rc;
+}
+
+#ifdef HAVE_NETNS
+/**
+ * lxcEnableInterfaces:
+ * @vm: Pointer to vm structure
+ *
+ * This function will enable the interfaces for this container.
+ *
+ * Returns 0 on success or -1 in case of error
+ */
+static int lxcEnableInterfaces(lxc_vm_t *vm)
It looks like the "vm" parameter may be "const".
If so, please declare it as such. Same with all of the ones below.
+{
+ int rc = -1;
+ lxc_net_def_t *net = vm->def->nets;
Once vm is const, it's good to make other pointers "const", too,
if possible. Here, "net" may be one, i.e., if
vethInterfaceUpOrDown doesn't modify memory through
its first parameter.
+ int i = 0;
I was going to suggest making "i" unsigned
if it's never negative. But it's not even used.
So, please remove that declaration and the use below.
+ for (i = 0; net; net = net->next) {
+ DEBUG("Enabling %s", net->containerVeth);
+ rc = vethInterfaceUpOrDown(net->containerVeth, 1);
+ if (0 != rc) {
+ goto error_out;
+ }
+ }
+
+ /* enable lo device */
+ rc = vethInterfaceUpOrDown("lo", 1);
+
+error_out:
+ return rc;
+}
+#endif /* HAVE_NETNS */
+
+/**
* lxcChild:
* @argv: Pointer to container arguments
*
@@ -210,6 +279,18 @@
goto cleanup;
}
+ /* Wait for interface devices to show up */
+ if (0 != (rc = lxcWaitForContinue(vm))) {
+ goto cleanup;
+ }
+
+#ifdef HAVE_NETNS
Please remove this in-function #ifdef.
Instead, arrange for lxcEnableInterfaces to be defined
as a no-op function when HAVE_NETNS is not defined.
+ /* enable interfaces */
+ if (0 != (rc = lxcEnableInterfaces(vm))) {
+ goto cleanup;
+ }
+#endif
...
diff -r acf369a2543a -r cb780a7b3ad5 src/lxc_driver.c
--- a/src/lxc_driver.c Thu Jun 19 08:59:37 2008 -0700
+++ b/src/lxc_driver.c Thu Jun 19 08:59:45 2008 -0700
@@ -44,6 +44,9 @@
#include "memory.h"
#include "util.h"
#include "memory.h"
+#include "bridge.h"
+#include "qemu_conf.h"
+#include "veth.h"
/* debug macros */
#define DEBUG(fmt,...) VIR_DEBUG(__FILE__, fmt, __VA_ARGS__)
@@ -66,6 +69,9 @@
#ifndef CLONE_NEWIPC
#define CLONE_NEWIPC 0x08000000
#endif
+#ifndef CLONE_NEWNET
+#define CLONE_NEWNET 0x40000000 /* New network namespace */
+#endif
static int lxcStartup(void);
static int lxcShutdown(void);
@@ -81,6 +87,9 @@
{
int rc = 0;
int flags = CLONE_NEWPID|CLONE_NEWNS|CLONE_NEWUTS|CLONE_NEWUSER|
+#ifdef HAVE_NETNS
Please remove the #ifdef.
Simply arrange for CLONE_NEWNET to be 0 when HAVE_NETNS is not defined.
Then you can use it without the ugly #ifdef.
> + CLONE_NEWNET|
> +#endif
> CLONE_NEWIPC|SIGCHLD;
> int cpid;
> char *childStack;
> @@ -237,6 +246,9 @@
> static int lxcNumDomains(virConnectPtr conn)
> {
> lxc_driver_t *driver = (lxc_driver_t *)conn->privateData;
> +
> + DEBUG("driver: %p network: %p", conn->privateData,
conn->networkPrivateData);
> +
> return driver->nactivevms;
> }
>
> @@ -384,6 +396,197 @@
> return lxcGenerateXML(dom->conn, driver, vm, vm->def);
> }
>
> +#ifdef HAVE_NETNS
> +/**
> + * lxcSetupInterfaces:
> + * @conn: pointer to connection
> + * @vm: pointer to virtual machine structure
> + *
> + * Sets up the container interfaces by creating the veth device pairs and
> + * attaching the parent end to the appropriate bridge. The container end
> + * will moved into the container namespace later after clone has been called.
> + *
> + * Returns 0 on success or -1 in case of error
> + */
> +static int lxcSetupInterfaces(virConnectPtr conn,
> + lxc_vm_t *vm)
> +{
> + int rc = -1;
> + struct qemud_driver *networkDriver =
> + (struct qemud_driver *)(conn->networkPrivateData);
> + lxc_net_def_t *net = vm->def->nets;
+ int i = 0;
Useless initialization and declaration again.
And a couple more below
+ char* bridge;
+ char parentVeth[PATH_MAX] = "";
+ char containerVeth[PATH_MAX] = "";
+
+ for (i = 0; net; net = net->next) {
+ if (LXC_NET_NETWORK == net->type) {
+ virNetworkPtr network = virNetworkLookupByName(conn, net->txName);
+ if (!network) {
+ goto error_exit;
+ }
+
+ bridge = virNetworkGetBridgeName(network);
+
+ virNetworkFree(network);
+
+ } else {
+ bridge = net->txName;
+ }
+
+ DEBUG("bridge: %s", bridge);
+ if (NULL == bridge) {
+ lxcError(conn, NULL, VIR_ERR_INTERNAL_ERROR,
+ _("failed to get bridge for interface"));
+ goto error_exit;
+ }
+
+ DEBUG0("calling vethCreate()");
+ if (NULL != net->parentVeth) {
+ strcpy(parentVeth, net->parentVeth);
+ }
+ if (NULL != net->containerVeth) {
+ strcpy(containerVeth, net->containerVeth);
+ }
+ DEBUG("parentVeth: %s, containerVeth: %s", parentVeth,
containerVeth);
+ if (0 != (rc = vethCreate(parentVeth, PATH_MAX, containerVeth, PATH_MAX))) {
+ lxcError(conn, NULL, VIR_ERR_INTERNAL_ERROR,
+ _("failed to create veth device pair: %d"), rc);
+ goto error_exit;
+ }
+ if (NULL == net->parentVeth) {
+ net->parentVeth = strdup(parentVeth);
+ }
+ if (NULL == net->containerVeth) {
+ net->containerVeth = strdup(containerVeth);
+ }
Don't you want to handle failed strdup here?
It looks like other places require net->containerVeth and
net->parentVeth to be non-NULL.
> + if (!(networkDriver->brctl) && (rc =
brInit(&(networkDriver->brctl)))) {
> + lxcError(conn, NULL, VIR_ERR_INTERNAL_ERROR,
> + _("cannot initialize bridge support: %s"),
> + strerror(rc));
> + goto error_exit;
> + }
> +
> + if (0 != (rc = brAddInterface(networkDriver->brctl, bridge, parentVeth)))
{
> + lxcError(conn, NULL, VIR_ERR_INTERNAL_ERROR,
> + _("failed to add %s device to %s: %s"),
> + parentVeth,
> + bridge,
> + strerror(rc));
> + goto error_exit;
> + }
> +
> + if (0 != (rc = vethInterfaceUpOrDown(parentVeth, 1))) {
> + lxcError(conn, NULL, VIR_ERR_INTERNAL_ERROR,
> + _("failed to enable parent ns veth device: %d"),
rc);
> + goto error_exit;
> + }
> +
> + }
> +
> + rc = 0;
> +
> +error_exit:
> + return rc;
> +}
> +
> +/**
> + * lxcMoveInterfacesToNetNs:
> + * @conn: pointer to connection
> + * @vm: pointer to virtual machine structure
> + *
> + * Starts a container process by calling clone() with the namespace flags
> + *
> + * Returns 0 on success or -1 in case of error
> + */
> +static int lxcMoveInterfacesToNetNs(virConnectPtr conn,
> + lxc_vm_t *vm)
+{
+ int rc = -1;
+ lxc_net_def_t *net = vm->def->nets;
+ int i = 0;
unused decl.
> +
> + for (i = 0; net; net = net->next) {
> + if (0 != moveInterfaceToNetNs(net->containerVeth, vm->def->id)) {
> + lxcError(conn, NULL, VIR_ERR_INTERNAL_ERROR,
> + _("failed to move interface %s to ns %d"),
> + net->containerVeth, vm->def->id);
> + goto error_exit;
> + }
> + }
> +
> + rc = 0;
> +
> +error_exit:
> + return rc;
> +}
> +
> +/**
> + * lxcCleanupInterfaces:
> + * @conn: pointer to connection
> + * @vm: pointer to virtual machine structure
> + *
> + * Cleans up the container interfaces by deleting the veth device pairs.
> + *
> + * Returns 0 on success or -1 in case of error
> + */
> +static int lxcCleanupInterfaces(lxc_vm_t *vm)
+{
+ int rc = -1;
+ lxc_net_def_t *net = vm->def->nets;
+ int i = 0;
likewise.
stacktop = stack + stacksize;
- flags = CLONE_NEWPID|CLONE_NEWNS|CLONE_NEWUTS|CLONE_NEWUSER|CLONE_NEWIPC|SIGCHLD;
+ flags = CLONE_NEWPID|CLONE_NEWNS|CLONE_NEWUTS|CLONE_NEWUSER|
+#ifdef HAVE_NETNS
remove ifdef
+ CLONE_NEWNET|
+#endif
+ CLONE_NEWIPC|SIGCHLD;
vm->def->id = clone(lxcChild, stacktop, flags, (void *)vm);
@@ -809,7 +1016,34 @@
close(vm->parentTty);
close(vm->containerTtyFd);
+#ifdef HAVE_NETNS
Define a stub function that returns 0, so you can
avoid the #ifdef.
+ if (0 != (rc = lxcSetupInterfaces(conn, vm))) {
+ goto cleanup;
+ }
+#endif /* HAVE_NETNS */
+
+ /* create a socket pair to send continue message to the container once */
+ /* we've completed the post clone configuration */
+ if (0 != socketpair(PF_UNIX, SOCK_STREAM, 0, vm->sockpair)) {
+ lxcError(conn, NULL, VIR_ERR_INTERNAL_ERROR,
+ _("sockpair failed: %s"), strerror(errno));
+ goto cleanup;
+ }
+
+ /* check this rc */
+
rc = lxcStartContainer(conn, driver, vm);
+
+#ifdef HAVE_NETNS
Avoid #ifdefs.
BTW, what's the point of saving return value in "rc"
if the very next statement is going to overwrite that value?
Either test it, or add a comment saying why it's ok to ignore failure,
in which case don't clobber the previous value.
+ rc = lxcMoveInterfacesToNetNs(conn, vm);
+#endif
+
+ rc = lxcSendContainerContinue(vm);
+
+ close(vm->sockpair[LXC_PARENT_SOCKET]);
+ vm->sockpair[LXC_PARENT_SOCKET] = -1;
+ close(vm->sockpair[LXC_CONTAINER_SOCKET]);
+ vm->sockpair[LXC_CONTAINER_SOCKET] = -1;
if (rc == 0) {
vm->state = VIR_DOMAIN_RUNNING;
@@ -948,6 +1182,11 @@
int waitRc;
int childStatus = -1;
+ /* if this fails, we'll continue. it will report any errors */
+#ifdef HAVE_NETNS
Define a no-op version of lxcCleanupInterfaces,
so you can remove this in-function #ifdef.
+ lxcCleanupInterfaces(vm);
+#endif
+
while (((waitRc = waitpid(vm->def->id, &childStatus, 0)) == -1)
&&
errno == EINTR);
12:44 p.m.
New subject: [libvirt] [PATCH 3 of 3] [LXC] Add setup/cleanup of container network interfaces
JM> Don't initialize readLen in the declaration, since it's set
JM> unconditionally just below.
I cleaned up a surprising number of these already when I took
ownership of the patch, but it looks like I missed a bunch more.
JM> Please remove this in-function #ifdef. Instead, arrange for
JM> lxcEnableInterfaces to be defined as a no-op function when
JM> HAVE_NETNS is not defined.
Okay.
JM> Please remove the #ifdef. Simply arrange for CLONE_NEWNET to be 0
JM> when HAVE_NETNS is not defined. Then you can use it without the
JM> ugly #ifdef.
So what happens if CLONE_NEWNET is present on the system (and
supported by the kernel) but the 'ip' binary doesn't support it?
Unless we #undef CLONE_NEWNET, you would create a new network
namespace and not be able to move anything into it. Would that be
your preference?
JM> Don't you want to handle failed strdup here? It looks like other
JM> places require net->containerVeth and
net-> parentVeth to be non-NULL.
Yep.
> + /* check this rc */
> +
> rc = lxcStartContainer(conn, driver, vm);
> +
> +#ifdef HAVE_NETNS
JM> BTW, what's the point of saving return value in "rc" if the very
JM> next statement is going to overwrite that value? Either test it,
JM> or add a comment saying why it's ok to ignore failure, in which
JM> case don't clobber the previous value.
I think the comment above that code is supposed justify it :)
I'll just fix up the checking instead and remove the comment.
Thanks!
--
Dan Smith
IBM Linux Technology Center
Open Hypervisor Team
email: danms(a)us.ibm.com
1:53 p.m.
New subject: [libvirt] [PATCH 3 of 3] [LXC] Add setup/cleanup of container network interfaces
Dan Smith <danms(a)us.ibm.com> wrote:
JM> Please remove the #ifdef. Simply arrange for CLONE_NEWNET to
be 0
JM> when HAVE_NETNS is not defined. Then you can use it without the
JM> ugly #ifdef.
So what happens if CLONE_NEWNET is present on the system (and
supported by the kernel) but the 'ip' binary doesn't support it?
Unless we #undef CLONE_NEWNET, you would create a new network
namespace and not be able to move anything into it. Would that be
your preference?
My suggestion was to eliminate the in-function #ifdef without changing
semantics, by adding something like this outside the function:
#ifndef HAVE_NETNS
# undef CLONE_NEWNET
# define CLONE_NEWNET 0
#endif
...
int flags = CLONE_NEWPID|CLONE_NEWNS|CLONE_NEWUTS|CLONE_NEWUSER|
CLONE_NEWNET|CLONE_NEWIPC|SIGCHLD;
That will work just like the original code.
...
>> + /* check this rc */
>> +
>> rc = lxcStartContainer(conn, driver, vm);
>> +
>> +#ifdef HAVE_NETNS
JM> BTW, what's the point of saving return value in "rc" if the very
JM> next statement is going to overwrite that value? Either test it,
JM> or add a comment saying why it's ok to ignore failure, in which
JM> case don't clobber the previous value.
I think the comment above that code is supposed justify it :)
The way I read it, "check this rc" sounds like it
must be a TODO or FIXME item, because that particular "rc"
value is the one that's being clobbered.
I'll just fix up the checking instead and remove the comment.
Thanks.
5998
days inactive
6002
days old
15 comments
5 participants
participants (5)
-
Dan Smith -
Daniel Lezcano -
Daniel Veillard -
Dave Leskovec -
Jim Meyering