From mboxrd@z Thu Jan  1 00:00:00 1970
From: George Dunlap <george.dunlap@citrix.com>
Subject: Re: [PATCH v3 2/6] libxl: Fix libxl_set_memory_target
	return value
Date: Tue, 8 Dec 2015 17:25:04 +0000
Message-ID: <56671270.9010007@citrix.com>
References: <1448970835-2706-1-git-send-email-george.dunlap@eu.citrix.com>
	<1448970835-2706-2-git-send-email-george.dunlap@eu.citrix.com>
	<1449595195.16124.125.camel@citrix.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <1449595195.16124.125.camel@citrix.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Ian Campbell <ian.campbell@citrix.com>, George Dunlap <george.dunlap@eu.citrix.com>, xen-devel@lists.xen.org
Cc: Ian Jackson <ian.jackson@citrix.com>, Wei Liu <wei.liu2@citrix.com>
List-Id: xen-devel@lists.xenproject.org

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