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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Dec 19, 2018 at 09:16:38PM -0800, Christopher Clark wrote:
> 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.

Then I'm afraid that you will have to change the current copy to/from
helpers to return an error code and fix all the callers. I don't think
it's acceptable to have this duplication of functionality in the code
base.

IMO having such redundancy creates confusion, specially with new
developers, so if returning an error code is much better and provides
cleaner code it should be argued for the whole Xen code base, and a
global switch should be made.

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

My issue is not so much with the implementation, but rather the
redundancy.

Thanks, Roger.

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