Hi Laine, Hmm...I was eager to a conclusion. We need some more discussion to a better way. Nonetheless, I hope this fix and another which I found later to be included in the next release that is expected soon. Is it OK for you, Laine? Of course, I'll continue this discussion until we have a consensus. ozaki-r On Mon, Jul 26, 2010 at 11:37 PM, Laine Stump <laine@xxxxxxxxx> wrote: > On 07/26/2010 08:31 AM, Ryota Ozaki wrote: >> >> On Mon, Jul 26, 2010 at 6:51 PM, Daniel P. Berrange<berrange@xxxxxxxxxx> >> 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@xxxxxxxxx> >>>> wrote: >>>>> >>>>> Hi Laine, >>>>> >>>>> On Sat, Jul 24, 2010 at 2:58 AM, Laine Stump<laine@xxxxxxxxx> 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. >> >> Laine, is it ok for you too? >> > > Doing that is fine when all possible failures in the function have an > associated errno. In the case of virRun'ing an external program that could > return a non-zero exit code, this is unfortunately not the case, so those > functions that call virRun will need to report the error themselves and > return -1. > > When non-0 exits from the called program are all failures, the simplest way > to do it is, as you say, to just not pass a pointer to a resultcode to > virRun (as long as the error message reported by virRun is useful enough - > remember that it gives the name of the program being run, and "virRun", but > not the name of the calling function). > > setMacAddr gives another example of a failure mode that breaks the "return > -errno" paradigm - it checks for one of the arguments being NULL, and fails > in that case. If it's important to maintain "-errno on failure" in one of > those cases, possibly examining the code will show that the arg in question > is never NULL, in which case you can mark it in its prototype with > ATTRIBUTE_NONNULL and just eliminate that failure from the code. > > It seems that in general, the -errno convention works better at lower levels > where all the failures are related to some system call failing, but at some > point higher in the call chain the possibility of failure due to config, > etc, comes in, there is no valid errno to describe a problem, and then you > need to start reporting the errors and returning -1. > > -- > libvir-list mailing list > libvir-list@xxxxxxxxxx > https://www.redhat.com/mailman/listinfo/libvir-list > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list