Re: [Xen-devel] ARM fixes for my improved privcmd patch.

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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.

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



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