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? Thanks, ozaki-r > > 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 :| > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list