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

[Daniel P. Berrange]

On Mon, Jul 26, 2010 at 09:31:21PM +0900, Ryota Ozaki wrote:
> On Mon, Jul 26, 2010 at 6:51 PM, Daniel P. Berrange <berrange at redhat.com> wrote:
> > On Sun, Jul 25, 2010 at 02:25:05AM +0900, Ryota Ozaki wrote:
> >> On Sat, Jul 24, 2010 at 11:44 PM, Ryota Ozaki <ozaki.ryota at gmail.com> wrote:
> >> > 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?
> >
> > You could just change
> >
> >   return cmdResult
> >
> > to
> >
> >   return -cmdResult;
> >
> > That would still let you give the error code, while also keeping the value
> > < 0
> 
> It looks better than mine ;-) I'll rewrite my patch in such a way.

Actually the other third option is to just call virReportError from
within the veth functions themselves, instead of logging errors in
the caller. This might be a better long term approach.

Daniel
-- 
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|