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);