Re: [Qemu-devel] [PATCH v3 07/14] qemu-img: Use BlockBackend as far as possible

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4851 bytes --]

On 01/26/2015 08:00 AM, Max Reitz wrote:
> Although qemu-img already creates BlockBackends, it does not do accesses
> to the images through them. This patch converts all of the bdrv_* calls
> for which this is currently possible to blk_* calls. Most of the
> remaining calls will probably stay bdrv_* calls because they really do
> operate on the BDS level instead of the BB level.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  qemu-img.c | 98 ++++++++++++++++++++++++++++++++++----------------------------
>  1 file changed, 54 insertions(+), 44 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 0b23c87..8b4139e 100644
> --- a/qemu-img.c

> @@ -1130,22 +1130,26 @@ static int img_compare(int argc, char **argv)
>      }
>      bs2 = blk_bs(blk2);
>  
> -    buf1 = qemu_blockalign(bs1, IO_BUF_SIZE);
> -    buf2 = qemu_blockalign(bs2, IO_BUF_SIZE);
> -    total_sectors1 = bdrv_nb_sectors(bs1);
> +    buf1 = blk_blockalign(blk1, IO_BUF_SIZE);
> +    buf2 = blk_blockalign(blk2, IO_BUF_SIZE);
> +    total_sectors1 = blk_getlength(blk1);
>      if (total_sectors1 < 0) {
>          error_report("Can't get size of %s: %s",
>                       filename1, strerror(-total_sectors1));
>          ret = 4;
>          goto out;
>      }
> -    total_sectors2 = bdrv_nb_sectors(bs2);
> +    total_sectors2 = blk_getlength(blk2);

The naming feels awkward; your conversion is now using bytes while the
old code was using sectors, so 'total_sectors2' feels weird...

>      if (total_sectors2 < 0) {
>          error_report("Can't get size of %s: %s",
>                       filename2, strerror(-total_sectors2));
>          ret = 4;
>          goto out;
>      }
> +
> +    total_sectors1 /= BDRV_SECTOR_SIZE;
> +    total_sectors2 /= BDRV_SECTOR_SIZE;

...at least you end up converting to sectors after all.  But it makes me
wonder if you should have blk_nb_sectors(), and/or temporary
intermediate variables to avoid cross-unit confusion.

> @@ -1476,13 +1480,14 @@ static int img_convert(int argc, char **argv)
>              goto out;
>          }
>          bs[bs_i] = blk_bs(blk[bs_i]);
> -        bs_sectors[bs_i] = bdrv_nb_sectors(bs[bs_i]);
> +        bs_sectors[bs_i] = blk_getlength(blk[bs_i]);
>          if (bs_sectors[bs_i] < 0) {
>              error_report("Could not get size of %s: %s",
>                           argv[optind + bs_i], strerror(-bs_sectors[bs_i]));
>              ret = -1;
>              goto out;
>          }
> +        bs_sectors[bs_i] /= BDRV_SECTOR_SIZE;

Another instance of the confusion.

>          total_sectors += bs_sectors[bs_i];
>      }
>  
> @@ -1625,16 +1630,19 @@ static int img_convert(int argc, char **argv)
>                                           out_bs->bl.discard_alignment))
>                      );
>  
> -    buf = qemu_blockalign(out_bs, bufsectors * BDRV_SECTOR_SIZE);
> +    buf = blk_blockalign(out_blk, bufsectors * BDRV_SECTOR_SIZE);
>  
>      if (skip_create) {
> -        int64_t output_sectors = bdrv_nb_sectors(out_bs);
> +        int64_t output_sectors = blk_getlength(out_blk);
>          if (output_sectors < 0) {
>              error_report("unable to get output image length: %s\n",
>                           strerror(-output_sectors));
>              ret = -1;
>              goto out;
> -        } else if (output_sectors < total_sectors) {
> +        }
> +
> +        output_sectors /= BDRV_SECTOR_SIZE;
> +        if (output_sectors < total_sectors) {

And another.

> @@ -2585,17 +2591,17 @@ static int img_rebase(int argc, char **argv)
>          uint8_t * buf_new;
>          float local_progress = 0;
>  
> -        buf_old = qemu_blockalign(bs, IO_BUF_SIZE);
> -        buf_new = qemu_blockalign(bs, IO_BUF_SIZE);
> +        buf_old = blk_blockalign(blk, IO_BUF_SIZE);
> +        buf_new = blk_blockalign(blk, IO_BUF_SIZE);
>  
> -        num_sectors = bdrv_nb_sectors(bs);
> +        num_sectors = blk_getlength(blk);
>          if (num_sectors < 0) {
...

> -        if (bs_new_backing) {
> -            new_backing_num_sectors = bdrv_nb_sectors(bs_new_backing);
> +
> +        num_sectors /= BDRV_SECTOR_SIZE;
> +        old_backing_num_sectors /= BDRV_SECTOR_SIZE;

and another.

I did not closely audit if there were any other conversions that should
have been made.  Also, I suspect that a blk_nb_sectors() as a pre-req
patch would make this one feel cleaner if you respin and rebase.  But if
we don't add blk_nb_sectors(), at least this version of the patch
appears to be clean with what it does, so you can consider this to be a
rather weak:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]