[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 07/14] qemu-img: Use BlockBackend as far as p
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v3 07/14] qemu-img: Use BlockBackend as far as possible |
Date: |
Mon, 26 Jan 2015 20:38:05 -0700 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 |
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 <address@hidden>
> ---
> 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 <address@hidden>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
- Re: [Qemu-devel] [PATCH v3 05/14] qemu-img: Use blk_new_open() in img_open(), (continued)
- [Qemu-devel] [PATCH v3 03/14] blockdev: Use blk_new_open() in blockdev_init(), Max Reitz, 2015/01/26
- [Qemu-devel] [PATCH v3 06/14] qemu-img: Use blk_new_open() in img_rebase(), Max Reitz, 2015/01/26
- [Qemu-devel] [PATCH v3 09/14] qemu-io: Use blk_new_open() in openfile(), Max Reitz, 2015/01/26
- [Qemu-devel] [PATCH v3 07/14] qemu-img: Use BlockBackend as far as possible, Max Reitz, 2015/01/26
- Re: [Qemu-devel] [PATCH v3 07/14] qemu-img: Use BlockBackend as far as possible,
Eric Blake <=
- [Qemu-devel] [PATCH v3 10/14] qemu-io: Remove "growable" option, Max Reitz, 2015/01/26
[Qemu-devel] [PATCH v3 14/14] block: Keep bdrv_check*_request()'s return value, Max Reitz, 2015/01/26
[Qemu-devel] [PATCH v3 12/14] block: Clamp BlockBackend requests, Max Reitz, 2015/01/26