From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50495) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aqo9O-0001z9-Rc for qemu-devel@nongnu.org; Thu, 14 Apr 2016 16:46:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aqo9K-0004tn-8V for qemu-devel@nongnu.org; Thu, 14 Apr 2016 16:46:06 -0400 References: <1460553847-29886-1-git-send-email-rkx1209dev@gmail.com> <1460553847-29886-2-git-send-email-rkx1209dev@gmail.com> From: Max Reitz Message-ID: <5710017B.1080500@redhat.com> Date: Thu, 14 Apr 2016 22:45:47 +0200 MIME-Version: 1.0 In-Reply-To: <1460553847-29886-2-git-send-email-rkx1209dev@gmail.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="vomI32AJtU7PLoIvCQm4SiqAAxtCdokSG" Subject: Re: [Qemu-devel] [Qemu-block] [PATCH 1/1] qemu-img: check block status of backing file when converting. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Ren Kimura , kwolf@redhat.com Cc: qemu-devel@nongnu.org, qemu-block@nongnu.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --vomI32AJtU7PLoIvCQm4SiqAAxtCdokSG Content-Type: multipart/mixed; boundary="LBxd7HHM28qRDxeide8WkgMDc3IvbMepk" From: Max Reitz To: Ren Kimura , kwolf@redhat.com Cc: qemu-devel@nongnu.org, qemu-block@nongnu.org Message-ID: <5710017B.1080500@redhat.com> Subject: Re: [Qemu-block] [PATCH 1/1] qemu-img: check block status of backing file when converting. References: <1460553847-29886-1-git-send-email-rkx1209dev@gmail.com> <1460553847-29886-2-git-send-email-rkx1209dev@gmail.com> In-Reply-To: <1460553847-29886-2-git-send-email-rkx1209dev@gmail.com> --LBxd7HHM28qRDxeide8WkgMDc3IvbMepk Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: quoted-printable On 13.04.2016 15:24, Ren Kimura wrote: > Signed-off-by: Ren Kimura How about you put what you wrote in the 0/1 email here? (But with s/it's/its/ :-)) > --- > qemu-img.c | 27 ++++++++++++++++++++++++--- > 1 file changed, 24 insertions(+), 3 deletions(-) >=20 > diff --git a/qemu-img.c b/qemu-img.c > index 06264d9..53471a1 100644 > --- a/qemu-img.c > +++ b/qemu-img.c > @@ -1451,6 +1451,21 @@ static void convert_select_part(ImgConvertState = *s, int64_t sector_num) > } > } > =20 > +static int64_t get_backing_status(BlockDriverState *bs, > + int64_t sector_num, > + int nb_sectors, int *pnum, > + BlockDriverState **file) > +{ > + int64_t ret; > + if (bs->backing) { > + ret =3D get_backing_status(bs->backing->bs, sector_num, > + nb_sectors, pnum, file); This won't really work. You may not fall through to the backing file if the overlay file has an allocated range here. So you'll have to call bdrv_get_block_status() unconditionally first, and only if its return value has neither the BDRV_BLOCK_DATA nor the BDRV_BLOCK_ZERO flag set you can (and then have to) fall through to its backing file. > + } else { > + ret =3D bdrv_get_block_status(bs, sector_num, nb_sectors, pnum= , file); > + } > + return ret; > +} > + Apart from that, I'd suggest to drop the "file" parameter. We don't care about whatever is returned in it anyway. Just declare a local BlockDriverState pointer in this function which will receive this value. The reason for why I'm suggesting this is because this would make it clearer to the reader that this value is discarded. Feel free to follow or ignore this suggestion, based on whether you consider it useful or not. Max > static int convert_iteration_sectors(ImgConvertState *s, int64_t secto= r_num) > { > int64_t ret; > @@ -1469,7 +1484,6 @@ static int convert_iteration_sectors(ImgConvertSt= ate *s, int64_t sector_num) > if (ret < 0) { > return ret; > } > - > if (ret & BDRV_BLOCK_ZERO) { > s->status =3D BLK_ZERO; > } else if (ret & BDRV_BLOCK_DATA) { > @@ -1477,10 +1491,17 @@ static int convert_iteration_sectors(ImgConvert= State *s, int64_t sector_num) > } else if (!s->target_has_backing) { > /* Without a target backing file we must copy over the con= tents of > * the backing file as well. */ > - /* TODO Check block status of the backing file chain to av= oid > + /* Check block status of the backing file chain to avoid > * needlessly reading zeroes and limiting the iteration to= the > * buffer size */ > - s->status =3D BLK_DATA; > + ret =3D get_backing_status(blk_bs(s->src[s->src_cur]), > + sector_num - s->src_cur_offset, > + n, &n, &file); > + if (ret & BDRV_BLOCK_ZERO) { > + s->status =3D BLK_ZERO; > + } else { > + s->status =3D BLK_DATA; > + } > } else { > s->status =3D BLK_BACKING_FILE; > } >=20 --LBxd7HHM28qRDxeide8WkgMDc3IvbMepk-- --vomI32AJtU7PLoIvCQm4SiqAAxtCdokSG Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJXEAF8AAoJEDuxQgLoOKyta2wH/jdGpcVl4QvKFAYt5xR94xCh EP+jaw1oW70xabQyhgOebRRIrcdn0D+XpalStHK0uF32a/AZkprSyr3I/cCazOwn TljZge91yBSLZ1p2v6SwCc6uDCXLulx2rsQ3QBjBqworSlws6SKgZRsRuGuwfNFi jses+IcLbNSLerzRH8lZMcTMObvHIZq8gDsCn0n79Es4Or36tvMKS5Woq/FAu4Ux JcvkOWFmKAJ9TAiyXS4IAQywnoikHAhKjPgpDITlDOp+n4DX6ISyoIIAVSEvajkT ViadbIGXQmFwZvTfDEX3D3cJafM0nfwcuiOCFwyHqX1jsQ2sJJPrMJFgUZRvy/o= =e8Y/ -----END PGP SIGNATURE----- --vomI32AJtU7PLoIvCQm4SiqAAxtCdokSG--