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

["Jan Beulich" <JBeulich@suse.com>]

>>> On 07.04.17 at 21:35, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/hvm/dm.c
> +++ b/xen/arch/x86/hvm/dm.c
> @@ -37,36 +37,48 @@ struct dmop_bufs {
>  #undef MAX_NR_BUFS
>  };
>  
> -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(
> +    const struct dmop_bufs *bufs, unsigned int idx,
> +    void *dst, size_t dst_bytes)

If the parameters all get re-arranged anyway, could I talk you into
ordering them in the more memcpy()-like destination, source, size
sequence?

Also, while certainly a matter of taste, I liked the previous way of
indenting better.

>  {
> -    size_t size;
> +    size_t buf_bytes;
>  
> -    if ( idx >= nr_bufs )
> +    if ( idx >= bufs->nr )
>          return false;
>  
> -    memset(dst, 0, dst_size);
> +    buf_bytes = bufs->buf[idx].size;
>  
> -    size = min_t(size_t, dst_size, bufs[idx].size);
> +    if ( dst_bytes > buf_bytes )
> +        return false;
> +
> +    memset(dst, 0, dst_bytes);
>  
> -    return !copy_from_guest(dst, bufs[idx].h, size);
> +    return !copy_from_guest(dst, bufs->buf[idx].h, dst_bytes);
>  }

The change away from using min() is a behavioral change which you
give no reason for, and which I don't see belonging here (if it is
wanted at all). And if that's what we want, then the memset() is
now pointless.

Jan


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