From mboxrd@z Thu Jan  1 00:00:00 1970
From: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Subject: Re: [PATCH v5 7/8] libxl/libxc: Move
 libxl_get_numainfo()'s hypercall buffer management to libxc
Date: Fri, 20 Mar 2015 10:59:17 -0400
Message-ID: <550C35C5.1070103@oracle.com>
References: <1426802044-19444-1-git-send-email-boris.ostrovsky@oracle.com>		
	<1426802044-19444-8-git-send-email-boris.ostrovsky@oracle.com>	
	<1426859767.21742.194.camel@citrix.com>
	<550C2F4B.8080508@oracle.com>
	<1426862796.21742.212.camel@citrix.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <1426862796.21742.212.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>
Cc: elena.ufimtseva@oracle.com, wei.liu2@citrix.com, stefano.stabellini@eu.citrix.com, andrew.cooper3@citrix.com, dario.faggioli@citrix.com, ian.jackson@eu.citrix.com, xen-devel@lists.xen.org, jbeulich@suse.com, keir@xen.org
List-Id: xen-devel@lists.xenproject.org

On 03/20/2015 10:46 AM, Ian Campbell wrote:
> On Fri, 2015-03-20 at 10:31 -0400, Boris Ostrovsky wrote:
>> On 03/20/2015 09:56 AM, Ian Campbell wrote:
>>> On Thu, 2015-03-19 at 17:54 -0400, Boris Ostrovsky wrote:
>>>> diff --git a/tools/libxc/xc_misc.c b/tools/libxc/xc_misc.c
>>>> index 411128e..607ae61 100644
>>>> --- a/tools/libxc/xc_misc.c
>>>> +++ b/tools/libxc/xc_misc.c
>>>> @@ -209,22 +209,49 @@ out:
>>>>        return ret;
>>>>    }
>>>>    
>>>> -int xc_numainfo(xc_interface *xch,
>>>> -                xc_numainfo_t *put_info)
>>>> +int xc_numainfo(xc_interface *xch, unsigned *max_nodes,
>>>> +                xc_meminfo_t *meminfo, uint32_t *distance)
>>>>    {
>>>>        int ret;
>>>>        DECLARE_SYSCTL;
>>>> +    DECLARE_HYPERCALL_BOUNCE(meminfo, *max_nodes * sizeof(*meminfo),
>>>> +                             XC_HYPERCALL_BUFFER_BOUNCE_OUT);
>>>> +    DECLARE_HYPERCALL_BOUNCE(distance,
>>>> +                             *max_nodes * *max_nodes * sizeof(*distance),
>>>> +                             XC_HYPERCALL_BUFFER_BOUNCE_OUT);
>>>>    
>>>> -    sysctl.cmd = XEN_SYSCTL_numainfo;
>>>> +    if (meminfo && distance) {
>>>> +        if ((ret = xc_hypercall_bounce_pre(xch, meminfo)))
>>>> +            goto out;
>>>> +        if ((ret = xc_hypercall_bounce_pre(xch, distance)))
>>>> +            goto out;
>>> Same comment about handling NULL as before.
>>>
>>> In addition what if only one of meminfo and distance is NULL? Is that
>>> valid or do you need a !!meminfo ^ !!distance check?
>> I want to treat this as as an error here, which is why I have
>>           } else if (meminfo || distance) {
>>               errno = EINVAL;
>>               return -1;
>>           }
>>
>> because the hypervisor will only attempt to copy numainfo things when
>> both are valid. Otherwise (i.e. even if only one is a NULL) it will
>> assume that this is a request for size. The alternative would be to add
>> another error there, which I decided not to do.
> Sorry, I'd trimmed that bit before I thought of the issue. Yes, what you
> had was right (although in general it is nicer to test for parameter
> validity first).

I'll flip the test order.

-boris