Re: [Xen-devel] [PATCH v4 01/31] libxl: fix libxl__build_hvm error handling

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

El 07/08/15 a les 13.03, Wei Liu ha escrit:
> On Fri, Aug 07, 2015 at 12:55:21PM +0200, Roger Pau Monné wrote:
>> El 07/08/15 a les 12.49, Wei Liu ha escrit:
>>> On Fri, Aug 07, 2015 at 12:17:38PM +0200, Roger Pau Monne wrote:
>>>> With the current code in libxl__build_hvm it is possible for the function 
>>>> to
>>>> fail and still return 0.
> 
> I care about this bit, which states clearly there is a bug that needs
> fixing.

There are a bunch of error paths in this function that needs fixing,
every error path below the call to libxl__domain_device_construct_rdm
will simply return with rc = 0, because the return code of the functions
is stored in ret, but the return code for libxl__build_hvm is fetched
from rc.

>>> It's hard to see where the bug is when this patch also does a bunch of
>>> refactoring.
>>
>> It refactors the error paths only, mainly replacing:
>>
>> if (libxl_call_foo(bar))
>>     <error>
>>
>> to
>>
>> rc = libxl_call_foo(bar)
>> if (rc != 0)
>>     <error>
>>
> 
> But this suggests there is no bug?

The bug is that we return with rc = 0, that's why it's important to
change this to follow the coding style. I agree that the patch could be
simpler by setting rc to some sane value in each error path (or just
setting rc to ERROR_FAIL in the out label), but it doesn't make much
sense to me to do this kind of fixing. If we fix it, we fix it for good.

> 
>> So we can keep the error codes returned by auxiliary functions.
>>
>>> It would be good if you can separate the bug fix from other name
>>> changing bits, so that we can apply that bug fix for 4.6 possibly queue
>>> it up for backporting.
>>
>> There are no name changing bits AFAICT.
>>
> 
> Changing ret for rc is naming changing to me. It's a good thing to do to
> comply with coding style, but mixing this with bug fix makes it hard to
> backport the fix itself.

I agree that the patch could be simplified, and I can send a simpler fix
iif needed, but as said above I would prefer to fix it in a proper way.

Roger.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel