Re: [PATCH 01/18] block: Make blk_{pread, pwrite}() return 0 on success

[Alberto Faria <afaria@redhat.com>]

On Mon, Jul 4, 2022 at 2:52 PM Hanna Reitz <hreitz@redhat.com> wrote:
> There are a couple of places where you decided to replace “*len”
> variables that used to store the return value by a plain “ret”. That
> seems good to me, given how these functions no longer return length
> values, but you haven’t done so consistently.  Below, I have noted
> places where this wasn’t done; I wonder why, but my R-b stands
> regardless of whether you change them too or not.

Thanks, this was just an oversight on my part. I'll fix it.

> > diff --git a/qemu-img.c b/qemu-img.c
> > index 4cf4d2423d..9d98ef63ac 100644
> > --- a/qemu-img.c
> > +++ b/qemu-img.c
> > @@ -5120,30 +5120,27 @@ static int img_dd(int argc, char **argv)
> >       in.buf = g_new(uint8_t, in.bsz);
> >
> >       for (out_pos = 0; in_pos < size; block_count++) {
> > -        int in_ret, out_ret;
> > +        int bytes, in_ret, out_ret;
> >
> > -        if (in_pos + in.bsz > size) {
> > -            in_ret = blk_pread(blk1, in_pos, in.buf, size - in_pos);
> > -        } else {
> > -            in_ret = blk_pread(blk1, in_pos, in.buf, in.bsz);
> > -        }
> > +        bytes = (in_pos + in.bsz > size) ? size - in_pos : in.bsz;
> > +
> > +        in_ret = blk_pread(blk1, in_pos, in.buf, bytes);
> >           if (in_ret < 0) {
> >               error_report("error while reading from input image file: %s",
> >                            strerror(-in_ret));
> >               ret = -1;
> >               goto out;
> >           }
> > -        in_pos += in_ret;
> > -
> > -        out_ret = blk_pwrite(blk2, out_pos, in.buf, in_ret, 0);
> > +        in_pos += bytes;
> >
> > +        out_ret = blk_pwrite(blk2, out_pos, in.buf, bytes, 0);
> >           if (out_ret < 0) {
> >               error_report("error while writing to output image file: %s",
> >                            strerror(-out_ret));
> >               ret = -1;
> >               goto out;
> >           }
> > -        out_pos += out_ret;
> > +        out_pos += bytes;
> >       }
> >
> >   out:
>
> We could use this opportunity to drop in_ret and out_ret altogether (but
> we can also decide not to).

Dropping them sounds good to me.

Alberto