[libvirt] [PATCH] lxc: Fix return value handlings of vethInterfaceUpOrDown and moveInterfaceToNetNs

[Laine Stump]

  On 07/23/2010 01:25 PM, Ryota Ozaki wrote:
> Both may return a positive value when they fail. We should check
> if the value is not zero instead of checking if it's negative.

I notice that  lxcSetupInterfaces has a comment saying that it returns 
-1 on failure, but it actually just passes on what is returned by 
vethInterfaceUpOrDown.

Would you be willing to consider instead just changing 
vethInterfaceUpOrDown and moveInterfaceToNetNs to return -1 in all error 
cases (and checking the return for < 0), rather than grabbing the exit 
code of the exec'ed command? None of the callers do anything with that 
extra information anyway, and it would help to make the return values 
more consistent (which makes it easier to catch bugs like this, or 
eliminates them altogether ;-)

(I was recently bitten by a similar bug...)

> lxcContainerRenameAndEnableInterfaces is expected to return a negative
> value on a failure, so the patch changes the return value to -1
> if vethInterfaceUpOrDown fails.
>
> Note that this patch may be related to the bug:
> https://bugzilla.redhat.com/show_bug.cgi?id=607496 .
> It would not fix the bug, but would unveil what happens.
> ---
>   src/lxc/lxc_container.c  |    7 ++++++-
>   src/lxc/lxc_controller.c |    2 +-
>   2 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
> index 4371dba..c77d262 100644
> --- a/src/lxc/lxc_container.c
> +++ b/src/lxc/lxc_container.c
> @@ -273,8 +273,13 @@ static int lxcContainerRenameAndEnableInterfaces(unsigned int nveths,
>       }
>
>       /* enable lo device only if there were other net devices */
> -    if (veths)
> +    if (veths) {
>           rc = vethInterfaceUpOrDown("lo", 1);
> +        if (0 != rc) {
> +            VIR_ERROR(_("Failed to enable lo (%d)"), rc);
> +            rc = -1;
> +        }
> +    }
>
>   error_out:
>       VIR_FREE(newname);
> diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
> index d8b7bc7..9829a69 100644
> --- a/src/lxc/lxc_controller.c
> +++ b/src/lxc/lxc_controller.c
> @@ -477,7 +477,7 @@ static int lxcControllerMoveInterfaces(unsigned int nveths,
>   {
>       unsigned int i;
>       for (i = 0 ; i<  nveths ; i++)
> -        if (moveInterfaceToNetNs(veths[i], container)<  0) {
> +        if (moveInterfaceToNetNs(veths[i], container) != 0) {
>               lxcError(VIR_ERR_INTERNAL_ERROR,
>                        _("Failed to move interface %s to ns %d"),
>                        veths[i], container);