Re: [Xen-devel] [PATCH 07/25] xen (ARM, x86): add errno-returning functions for copy

[Christopher Clark <christopher.w.clark@xxxxxxxxx>]

On Wed, Dec 12, 2018 at 8:03 AM Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:
>
> On Fri, Nov 30, 2018 at 05:32:46PM -0800, Christopher Clark wrote:
> > Applied to both x86 and ARM headers.
> >
> > Signed-off-by: Christopher Clark <christopher.clark6@xxxxxxxxxxxxxx>
> > ---
> >  xen/include/asm-arm/guest_access.h | 25 +++++++++++++++++++++++++
> >  xen/include/asm-x86/guest_access.h | 29 +++++++++++++++++++++++++++++
> >  xen/include/xen/guest_access.h     |  3 +++
> >  3 files changed, 57 insertions(+)
> >
> > diff --git a/xen/include/asm-arm/guest_access.h 
> > b/xen/include/asm-arm/guest_access.h
> > index 224d2a0..7b6f89c 100644
> > --- a/xen/include/asm-arm/guest_access.h
> > +++ b/xen/include/asm-arm/guest_access.h
> > @@ -24,6 +24,11 @@ int access_guest_memory_by_ipa(struct domain *d, paddr_t 
> > ipa, void *buf,
> >  #define __raw_copy_from_guest raw_copy_from_guest
> >  #define __raw_clear_guest raw_clear_guest
> >
> > +#define raw_copy_from_guest_errno(dst, src, len)             \
> > +    (raw_copy_from_guest((dst), (src), (len)) ? -EFAULT : 0)
> > +#define raw_copy_to_guest_errno(dst, src, len)               \
> > +    (raw_copy_to_guest((dst), (src), (len)) ? -EFAULT : 0)
>
> Since the only error that you return is EFAULT, I don't really see the
> point in adding all those helpers. You achieve exactly the same by
> returning a boolean and doing the translation to EFAULT in the caller
> if required.
>
> It might have been nice to have the copy to/from set of functions
> return an error value, but adding a new set of helpers that have the
> same functionality but just differ in the return value look
> redundant.

It is true that there is redundancy with these -- but I think there are decent
arguments in favour of taking these in:

* the errno-providing interface is just a better fit for almost every call site
- which means less source code in total, that is easier to read.

* it is promoting good interface design for error handling:
  return of error code.

* since these are in use within the uxen source code, it eases comparison and
  work across both codebases - relevant for Argo, due to v4v.

I've rewritten the implementation of these for the second version of the patch
series -- now much simpler -- and hopefully that will mitigate some of your
concern about them.

Christopher

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel