Re: [libvirt] [PATCH 3 of 3] [LXC] Add setup/cleanup of container network interfaces
Dan Smith <[EMAIL PROTECTED]> 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. -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [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: [EMAIL PROTECTED] pgpG3N5Awd9vx.pgp Description: PGP signature -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [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.cThu Jun 19 08:59:37 2008 -0700
> +++ b/src/lxc_driver.cThu 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 0x0800
> #endif
> +#ifndef CLONE_NEWNET
> +#define CLONE_NEWNET 0x4000 /* 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 parentVet
[libvirt] [PATCH 3 of 3] [LXC] Add setup/cleanup of container network interfaces
# HG changeset patch
# User Dan Smith <[EMAIL PROTECTED]>
# 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.hThu Jun 19 08:59:37 2008 -0700
+++ b/src/lxc_conf.hThu 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 0x0800
#endif
+#ifndef CLONE_NEWNET
+#define CLONE_NEWNET 0x4000 /* 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 bridg
