Re: [Xen-devel] [PATCH] xen/physmap: Propagate error rc from xenmem_add_to_physmap_one

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.12.16 at 18:09, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 07/12/16 11:43, Jan Beulich wrote:
>>>>> On 07.12.16 at 02:07, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> On 07/12/2016 01:00, Jiandi An wrote:
>>>> --- a/xen/common/memory.c
>>>> +++ b/xen/common/memory.c
>>>> @@ -762,6 +762,8 @@ static int xenmem_add_to_physmap_batch(struct domain 
>>>> *d,
>>>>          rc = xenmem_add_to_physmap_one(d, xatpb->space,
>>>>                                         xatpb->u,
>>>>                                         idx, _gfn(gpfn));
>>>> +        if ( rc < 0 )
>>>> +            goto out;
>>>>  
>>>>          if ( unlikely(__copy_to_guest_offset(xatpb->errs, 0, &rc, 1)) )
>>>>          {
>>> This can't be correct.  You now skip writing rc into the errs[] array on
>>> a failure, which means that userspace will get an overall failure but an
>>> errs[] array which said that nothing went wrong.
>>>
>>> This code addition looks like it wants to be an "else if" on the end of
>>> this if() in context.
>> Why would this go elsewhere? It's unneeded - it's a property of the
>> hypercall that when seeing overall success you still need to look at
>> errs[].
> 
> Looking at the public header files, the error behaviour isn't even
> documented.  (I'm going to try and remember to pick up on bugs like this
> more closely during future review.)

I agree it would be nice for behavior to be spelled out if it's
possibly unexpected, but ...

> Similar batch hypercalls return the index of the first failing
> operation, which is a far more helpful behaviour for the caller.

... this is not really an option here. It would make sense if the
operation stopped at that slot, but that's not the case. errs[]
needs to be scanned in full anyway to find all (and not just
the first) error slots. Furthermore positive return values are
used by the function to indicate the need for a continuation
to be invoked.

Jan


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