[libvirt] [PATCH 3 of 3] [LXC] Add setup/cleanup of container network interfaces

[Dan Smith]

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 at us.ibm.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 188 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20080620/a4579fd1/attachment-0001.sig>