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);
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list