Re: [PATCH v3 2/6] libxl: Fix libxl_set_memory_target return value

[George Dunlap <george.dunlap@citrix.com>]

On 08/12/15 17:19, Ian Campbell wrote:
> On Tue, 2015-12-01 at 11:53 +0000, George Dunlap wrote:
>> libxl_set_memory_target seems to have the following return values:
>>
>> * 1 on failure, if the failure happens because of a xenstore error *or*
>> invalid target
>>
>> * -1 if the setmaxmem hypercall
>>
>> * -errno if the set_pod_target hypercall target fails
>>
>> * 1 on success (!)
>>
>> Make it consistenstly return ERROR_FAIL, unless the parameters were
> 
> "consistently"
> 
>> invalid.
>>
>> To make this more robust, use 'lrc' for return values to functions
> 
> tools/libxl/CODING_STYLE recommends "r" for such variables (return values
> of syscalls or libxc calls).
> 
>> whose return values are a different error space (like
>> xc_domain_setmaxmem and xc_domain_set_pod_target), or where a failure
>> means retry, rather than fail the whole function
>> (libxl__fill_dom0_memory_info), to reduce the risk that future code
>> shuffles will accidentally clobber the return value again.
>>
>> Also remove the final call to xc_domain_getinfolist. There's no
>> obvious reason for this call -- all it seems to be doing is checking
>> to see if the domain exists; but if it doesn't exist, it will have
>> already failed by this point.
> 
> If we aren't sure what it is for then I'd rather remove it in an
> independent change, just in case.
> 
>> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
>> ---
>> CC: Ian Campbell <ian.campbell@citrix.com>
>> CC: Ian Jackson <ian.jackson@citrix.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
>> ---
>>  tools/libxl/libxl.c | 27 +++++++++++++--------------
>>  1 file changed, 13 insertions(+), 14 deletions(-)
>>
>> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
>> index 814d056..f8a0642 100644
>> --- a/tools/libxl/libxl.c
>> +++ b/tools/libxl/libxl.c
>> @@ -4722,7 +4722,7 @@ int libxl_set_memory_target(libxl_ctx *ctx,
>> uint32_t domid,
>>          int32_t target_memkb, int relative, int enforce)
>>  {
>>      GC_INIT(ctx);
>> -    int rc = 1, abort_transaction = 0;
>> +    int rc = ERROR_FAIL, abort_transaction = 0, lrc;
> 
> CODING_STYLE asks that rc not be initialised on declaration but set on the
> failure paths (it allows a single line if () { rc = ; goto ... } to
> mitigate the verbosity of this somewhat).

Ack to all of the above.

 -George