Re: [Xen-devel] [PATCH v3 2/6] libxl: Fix libxl_set_memory_target return value
On Tue, 2015-12-01 at 11:53 +, 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
> ---
> CC: Ian Campbell
> CC: Ian Jackson
> CC: Wei Liu
> ---
> 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).
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 2/6] libxl: Fix libxl_set_memory_target return value
On 08/12/15 17:19, Ian Campbell wrote:
> On Tue, 2015-12-01 at 11:53 +, 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
>> ---
>> CC: Ian Campbell
>> CC: Ian Jackson
>> CC: Wei Liu
>> ---
>> 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
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
