From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35863) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YFxGV-0005Hb-Mw for qemu-devel@nongnu.org; Mon, 26 Jan 2015 22:56:37 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YFxGR-0001Fo-GN for qemu-devel@nongnu.org; Mon, 26 Jan 2015 22:56:35 -0500 Received: from mx1.redhat.com ([209.132.183.28]:38839) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YFwyg-0002PC-Vs for qemu-devel@nongnu.org; Mon, 26 Jan 2015 22:38:11 -0500 Message-ID: <54C7081D.9030706@redhat.com> Date: Mon, 26 Jan 2015 20:38:05 -0700 From: Eric Blake MIME-Version: 1.0 References: <1422284444-12529-1-git-send-email-mreitz@redhat.com> <1422284444-12529-8-git-send-email-mreitz@redhat.com> In-Reply-To: <1422284444-12529-8-git-send-email-mreitz@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="xQbhcP74kXa1wSeeR6Uxm3I3KCTOhKJhn" Subject: Re: [Qemu-devel] [PATCH v3 07/14] qemu-img: Use BlockBackend as far as possible List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz , qemu-devel@nongnu.org Cc: Kevin Wolf , Markus Armbruster , Stefan Hajnoczi , Stefano Stabellini This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --xQbhcP74kXa1wSeeR6Uxm3I3KCTOhKJhn Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 01/26/2015 08:00 AM, Max Reitz wrote: > Although qemu-img already creates BlockBackends, it does not do accesse= s > 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. >=20 > Signed-off-by: Max Reitz > --- > qemu-img.c | 98 ++++++++++++++++++++++++++++++++++--------------------= -------- > 1 file changed, 54 insertions(+), 44 deletions(-) >=20 > 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 =3D blk_bs(blk2); > =20 > - buf1 =3D qemu_blockalign(bs1, IO_BUF_SIZE); > - buf2 =3D qemu_blockalign(bs2, IO_BUF_SIZE); > - total_sectors1 =3D bdrv_nb_sectors(bs1); > + buf1 =3D blk_blockalign(blk1, IO_BUF_SIZE); > + buf2 =3D blk_blockalign(blk2, IO_BUF_SIZE); > + total_sectors1 =3D blk_getlength(blk1); > if (total_sectors1 < 0) { > error_report("Can't get size of %s: %s", > filename1, strerror(-total_sectors1)); > ret =3D 4; > goto out; > } > - total_sectors2 =3D bdrv_nb_sectors(bs2); > + total_sectors2 =3D 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 =3D 4; > goto out; > } > + > + total_sectors1 /=3D BDRV_SECTOR_SIZE; > + total_sectors2 /=3D BDRV_SECTOR_SIZE; =2E..at least you end up converting to sectors after all. But it makes m= e 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] =3D blk_bs(blk[bs_i]); > - bs_sectors[bs_i] =3D bdrv_nb_sectors(bs[bs_i]); > + bs_sectors[bs_i] =3D 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 =3D -1; > goto out; > } > + bs_sectors[bs_i] /=3D BDRV_SECTOR_SIZE; Another instance of the confusion. > total_sectors +=3D bs_sectors[bs_i]; > } > =20 > @@ -1625,16 +1630,19 @@ static int img_convert(int argc, char **argv) > out_bs->bl.discard_alignment)= ) > ); > =20 > - buf =3D qemu_blockalign(out_bs, bufsectors * BDRV_SECTOR_SIZE); > + buf =3D blk_blockalign(out_blk, bufsectors * BDRV_SECTOR_SIZE); > =20 > if (skip_create) { > - int64_t output_sectors =3D bdrv_nb_sectors(out_bs); > + int64_t output_sectors =3D blk_getlength(out_blk); > if (output_sectors < 0) { > error_report("unable to get output image length: %s\n", > strerror(-output_sectors)); > ret =3D -1; > goto out; > - } else if (output_sectors < total_sectors) { > + } > + > + output_sectors /=3D 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 =3D 0; > =20 > - buf_old =3D qemu_blockalign(bs, IO_BUF_SIZE); > - buf_new =3D qemu_blockalign(bs, IO_BUF_SIZE); > + buf_old =3D blk_blockalign(blk, IO_BUF_SIZE); > + buf_new =3D blk_blockalign(blk, IO_BUF_SIZE); > =20 > - num_sectors =3D bdrv_nb_sectors(bs); > + num_sectors =3D blk_getlength(blk); > if (num_sectors < 0) { =2E.. > - if (bs_new_backing) { > - new_backing_num_sectors =3D bdrv_nb_sectors(bs_new_backing= ); > + > + num_sectors /=3D BDRV_SECTOR_SIZE; > + old_backing_num_sectors /=3D 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 --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --xQbhcP74kXa1wSeeR6Uxm3I3KCTOhKJhn Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJUxwgdAAoJEKeha0olJ0NqhgcIAIHFiUgb3jtSpc49jWac8hr+ WlMkNG96mW/W+lrAYHwZbTJMJgXvXoyX5DJBRKwpI5N4tn0yItTdY16iMOtnT7lW qtsvN4WT2cCF/OoRZB8ZboCBm0jdE8s8q++SfJ3BPUL6ZhOiWLKeao9ocYU4nNsW btGmAfEZEyrG8vEjSigdprimp65GayN0tU1oy875fhYM3VmAlXhsj1jkW3yUFDRo Yks6naqd2Tks+TBBIJ+GJgp4bh9MeFQttOzZ8Y9h8wWQpnSZCGiCGry9HeZ5qpGJ ICQtSI5+RLn1BK4u8CDow4hk0zjFp7X5qkaXB0H1y45EWaWu8uAtHfY7SCMi5qY= =cqs8 -----END PGP SIGNATURE----- --xQbhcP74kXa1wSeeR6Uxm3I3KCTOhKJhn--