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

[Ryota Ozaki]

Hi Laine,

On Sat, Jul 24, 2010 at 2:58 AM, Laine Stump <laine at laine.org> wrote:
>  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.

Oh, I didn't know that.

Additionally, I found that other functions, e.g., setMacAddr, are also handled
with the wrong way. And also handling return values with virReportSystemError
is also wrong because it expects an errno, not an exit code. We have to fix
all of them ;-<

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

Yeah, I'm also a bit annoying with the return values. Hmm, but we now show error
messages with the return values outside the functions. Without that, we have to
show the error message in the functions or some other place, otherwise we lose
useful information of errors. It seems not good idea.

One option is to let virRun show an error message by passing NULL to the second
argument (status) of it, like brSetEnableSTP in util/bridge.c, and
always return -1
on a failure. It'd satisfy what you suggest.

Honestly said, I cannot decide. Anyone has any suggestions on that?

Thanks,
  ozaki-r

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