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

[Ian Campbell <ian.campbell@citrix.com>]

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).

Ian.

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