On 02/20/2014 12:33 PM, Jeff Cody wrote: > On Thu, Feb 20, 2014 at 03:57:20PM +0100, Kevin Wolf wrote: >> Instead of ignoring all option values but the last one, multiple -o >> options now have the same meaning as having a single option with all >> settings in the order of their respective -o options. >>
> There is a 'return -1' that is missed, that I think needs to be 'goto > out;' for cleanup. It is right after the call to > bdrv_parse_cache_flags() in the img_convert() function: > > ret = bdrv_parse_cache_flags(cache, &flags); > if (ret < 0) { > error_report("Invalid cache option: %s", cache); > return -1; > } > > Looks like this has been this way for a while, though. And this is an > instance (the only instance) where img_convert returns -1 instead of 0 > or 1. I'm not sure the implications if that changed, and became '1', > so we'd probably want to preserve the return value. No, exit status of 255 on an invalid cache option is a bug; fixing it to return EXIT_FAILURE (1) is indeed correct. [Hmm, this shows that I only reviewed the changed sections, rather than also looking for all other 'return' statements - thanks Jeff for the more thorough review] > > Since this doesn't have anything to do with this patch (although > leaking 'options' now, in addition to the other stuff leaked), I'll go > ahead and give my reviewed-by, and we can fix this in another > follow-up patch: Agreed to that plan of attack (we've now racked up several good followups). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature