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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 20.12.18 at 06:16, <christopher.w.clark@xxxxxxxxx> 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.
> 
> * 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.

Without having looked at their v2 forms, I continue to fully agree
with Roger here: There's no reason to introduce these flavors just
for argo to use. We've been doing fine with what we have, and if
you want to change things, then you'd want to do so everywhere.
That would then also eliminate the need for separate flavors: The
ones that are there would then simply behave along the lines of
the principles you outline above.

Jan


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