Re: ARM fixes for my improved privcmd patch.

[Mats Petersson <mats.petersson@citrix.com>]

On 19/12/12 15:51, Ian Campbell wrote:
> On Wed, 2012-12-19 at 15:47 +0000, Mats Petersson wrote:
>> On 19/12/12 15:45, Ian Campbell wrote:
>>> On Wed, 2012-12-19 at 15:08 +0000, Mats Petersson wrote:
>>>> On 19/12/12 12:22, Ian Campbell wrote:
>>>>> On Wed, 2012-12-19 at 12:10 +0000, Mats Petersson wrote:
>>>>>
>>>>>>>> +                  only likely to return EFAULT or some other "things are very
>>>>>>>> +                  bad" error code, which the rest of the calling code won't
>>>>>>>> +                  be able to fix up. So we just exit with the error we got.
>>>>>>> It expect it is more important to accumulate the individual errors from
>>>>>>> remap_pte_fn into err_ptr.
>>>>>> Yes, but since that exits on error with EFAULT, the calling code won't
>>>>>> "accept" the errors, and thus the whole house of cards fall apart at
>>>>>> this point.
>>>>>>
>>>>>> There should probably be a task to fix this up properly, hence the
>>>>>> comment. But right now, any error besides ENOENT is "unacceptable" by
>>>>>> the callers of this code. If you want me to add this to the comment, I'm
>>>>>> happy to. But as long as remap_pte_fn returns EFAULT on error, nothing
>>>>>> will work after an error.
>>>>> Are you sure? privcmd.c has some special casing for ENOENT but it looks
>>>>> like it should just pass through other errors back to userspace.
>>>>>
>>>>> In any case surely this needs fixing?
>>>>>
>>>>> On the X86 side err_ptr is the result of the mmupdate hypercall which
>>>>> can already be other than ENOENT, including EFAULT, ESRCH etc.
>>>> Yes, but the ONLY error that is "acceptable" (as in, doesn't lead to the
>>>> calling code revoking the mapping and returning an error) is ENOENT.
>>> Hr, Probably the right thing is for map_foreign_page to propagate the
>>> result of XENMEM_add_to_physmap_range and for remap_pte_fn to store it
>>> in err_ptr. That -EFAULT thing just looks wrong to me.
>> Ok, so you want me to fix that up, I suppose? I mean, I just copied the
>> behaviour that was already there - just massaged the code around a bit...
> Yes please, it didn't really matter before but I think it matters after
> your changes.
Ok, I will try to fix. But since I can't test it, it will still be "does 
it compile" testing... {Would be nice to understand what has changed - 
as far as I see, the old code was just as much broken as the new code}

--
Mats
>
>> --
>> Mats
>>>> Or at least, that's how I believe it should SHOULD be - since only
>>>> ENOENT is a "success" error code, anything else pretty much means that
>>>> the operation requested didn't work properly. If you are aware of any
>>>> use-case where EFAULT, ESRCH or other error codes would still result in
>>>> a valid, usable memory mapping - I have a fair understanding of the xc_*
>>>> code, and although I may not know every piece of that code, I'm fairly
>>>> certainly that is the expected behaviour.
>>>>
>>>> --
>>>> Mats
>>>>> Ian.
>>>>>
>