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