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

On Sat, Jul 24, 2010 at 11:44 PM, Ryota Ozaki <ozaki.ryota(a)gmail.com&gt; wrote: > Hi Laine, > > On Sat, Jul 24, 2010 at 2:58 AM, Laine Stump <laine(a)laine.org&gt; 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?