Re: [PATCH v2 4/7] block/block-copy: refactor interfaces to use bytes instead of end

[Vladimir Sementsov-Ogievskiy]

07.02.2020 21:01, Max Reitz wrote:
> On 27.11.19 19:08, Vladimir Sementsov-Ogievskiy wrote:
> > We have a lot of "chunk_end - start" invocations, let's switch to
> > bytes/cur_bytes scheme instead.
> >
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> > ---
> >   include/block/block-copy.h |  4 +--
> >   block/block-copy.c         | 68 ++++++++++++++++++++------------------
> >   2 files changed, 37 insertions(+), 35 deletions(-)
>
> [...]
>
> > diff --git a/block/block-copy.c b/block/block-copy.c
> > index 94e7e855ef..cc273b6cb8 100644
> > --- a/block/block-copy.c
> > +++ b/block/block-copy.c
>
> [...]
>
> > @@ -150,24 +150,26 @@ void block_copy_set_callbacks(
>
> [...]
>
> >   static int coroutine_fn block_copy_do_copy(BlockCopyState *s,
> > -                                           int64_t start, int64_t end,
> > +                                           int64_t start, int64_t bytes,
>
> I wonder whether it would make more sense to make some of these @bytes
> parameters plain ints, because...
>
> >                                              bool zeroes, bool *error_is_read)
> >   {
> >       int ret;
> > -    int nbytes = MIN(end, s->len) - start;
> > +    int nbytes = MIN(start + bytes, s->len) - start;
>
> ...things like this look a bit dangerous now.  So if the interface
> already clearly shows that we’re always expecting something less than
> INT_MAX, it might all be a bit clearer.

Hmm, yes. And it's preexisting, just becomes more obvious with new semantics.
All block-copy tasks are limited to s->copy_size so it's actually safe.
I'd better add an assertion, as I believe that 64bit write_zeroes will appear 
in not far future.

> I’ll leave it up to you:
>
> Reviewed-by: Max Reitz <address@hidden>


--
Best regards,
Vladimir