Re: [libvirt] [PATCH 3 of 3] [LXC] Add setup/cleanup of container network interfaces
Dan Smith <danms(a)us.ibm.com> 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.