Re: [Xen-devel] [PATCH 2/4] hvm/dmop: Implement copy_{to, from}_guest_buf() in terms of raw accessors

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 20.04.17 at 19:59, <jennifer.herbert@xxxxxxxxxx> wrote:
> From: Jennifer Herbert <Jennifer.Herbert@xxxxxxxxxx>

Is this correct, considering that iirc the patch was new in v5 and ...

> This also allows the usual cases to be simplified, by omitting an 
> unnecessary
> buf parameters, and because the macros can appropriately size the object.
> 
> This makes copying to or from a buf that isn't big enough an error.
> If the buffer isnt big enough, trying to carry on regardless
> can only cause trouble later on.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Signed-off-by: Jennifer Herbert <Jennifer.Herbert@xxxxxxxxxx>

... this sequence of S-o-b-s?

> --- a/xen/arch/x86/hvm/dm.c
> +++ b/xen/arch/x86/hvm/dm.c
> @@ -32,36 +32,47 @@ struct dmop_args {
>      struct xen_dm_op_buf buf[2];
>  };
>  
> -static bool copy_buf_from_guest(const xen_dm_op_buf_t bufs[],
> -                                unsigned int nr_bufs, void *dst,
> -                                unsigned int idx, size_t dst_size)
> +static bool _raw_copy_from_guest_buf(void *dst,
> +                                     const struct dmop_args *args,
> +                                     unsigned int buf_idx,
> +                                     size_t dst_bytes)
>  {
> -    size_t size;
> +    size_t buf_bytes;
>  
> -    if ( idx >= nr_bufs )
> +    if ( buf_idx >= args->nr_bufs )
>          return false;
>  
> -    memset(dst, 0, dst_size);
> +    buf_bytes =  args->buf[buf_idx].size;
>  
> -    size = min_t(size_t, dst_size, bufs[idx].size);
> +    if ( dst_bytes > buf_bytes )
> +        return false;

While this behavioral change is now being mentioned in the
description, I'm not sure I buy the argument of basically being
guaranteed to cause trouble down the road. Did you consider the
forward compatibility aspect here, allowing us to extend interface
structures by adding fields to their ends without breaking old
callers? Paul, what are your thoughts here?

Jan


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