From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42710) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bHWsf-0004wd-DE for qemu-devel@nongnu.org; Mon, 27 Jun 2016 09:47:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bHWsa-0001Oc-As for qemu-devel@nongnu.org; Mon, 27 Jun 2016 09:47:16 -0400 References: <1466500894-9710-1-git-send-email-kwolf@redhat.com> <1466500894-9710-13-git-send-email-kwolf@redhat.com> From: Max Reitz Message-ID: <0caba858-787c-ccdf-69df-f8dc92528c1f@redhat.com> Date: Mon, 27 Jun 2016 15:47:03 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="RdgAOkPF3pV9CeSSD8epo7oOJieBGVKjn" Subject: Re: [Qemu-devel] [PATCH 12/17] block: Convert bdrv_write() to BdrvChild List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf , qemu-block@nongnu.org Cc: stefanha@redhat.com, famz@redhat.com, jcody@redhat.com, qemu-devel@nongnu.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --RdgAOkPF3pV9CeSSD8epo7oOJieBGVKjn From: Max Reitz To: Kevin Wolf , qemu-block@nongnu.org Cc: stefanha@redhat.com, famz@redhat.com, jcody@redhat.com, qemu-devel@nongnu.org Message-ID: <0caba858-787c-ccdf-69df-f8dc92528c1f@redhat.com> Subject: Re: [PATCH 12/17] block: Convert bdrv_write() to BdrvChild References: <1466500894-9710-1-git-send-email-kwolf@redhat.com> <1466500894-9710-13-git-send-email-kwolf@redhat.com> In-Reply-To: Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: quoted-printable On 27.06.2016 15:44, Max Reitz wrote: > On 21.06.2016 11:21, Kevin Wolf wrote: >> Signed-off-by: Kevin Wolf >> --- >> block/io.c | 5 +++-- >> block/qcow.c | 10 +++++++++- >> block/qcow2-cluster.c | 2 +- >> block/qcow2-refcount.c | 2 +- >> block/qcow2.c | 10 +++++++++- >> block/vdi.c | 4 ++-- >> block/vvfat.c | 5 ++--- >> include/block/block.h | 2 +- >> 8 files changed, 28 insertions(+), 12 deletions(-) >=20 > Because I think this is better than saying "I won't give an R-b because= > of (1), (2)..." at the bottom: >=20 > Review key: > [o] -- completely optional suggestion > [r] -- request, optional, but I'll wait with an R-b until I get feedbac= k > [x] -- requires a fix >=20 > [...] >=20 >> diff --git a/block/qcow.c b/block/qcow.c >> index e09827b..c80df78 100644 >> --- a/block/qcow.c >> +++ b/block/qcow.c >> @@ -969,7 +969,15 @@ static int qcow_write_compressed(BlockDriverState= *bs, int64_t sector_num, >> =20 >> if (ret !=3D Z_STREAM_END || out_len >=3D s->cluster_size) { >> /* could not compress: write normal cluster */ >> - ret =3D bdrv_write(bs, sector_num, buf, s->cluster_sectors); >> + QEMUIOVector qiov; >> + struct iovec iov =3D { >> + .iov_base =3D (uint8_t*) buf, >=20 > [o] ERROR: "(foo*)" should be "(foo *)" >=20 > !!11!1elf >=20 >> + .iov_len =3D nb_sectors * BDRV_SECTOR_SIZE, >> + }; >> + qemu_iovec_init_external(&qiov, &iov, 1); >> + >> + ret =3D bs->drv->bdrv_co_writev(bs, sector_num, s->cluster_se= ctors, >> + &qiov); >=20 > [r] I'd use nb_sectors instead of s->cluster_sectors (or use > s->cluster_sectors above), I think it should be the same variable in > both places. >=20 > [o] Also, but this is a much weaker proposal, I'd have preferred a > direct call to qcow_co_writev(). But maybe that's just me. [x] qcow_write_compressed() is not a coroutine_fn, resulting in: qemu-img: ./util/qemu-coroutine-lock.c:143: qemu_co_mutex_unlock: Assertion `qemu_in_coroutine()' failed. >> if (ret < 0) { >> goto fail; >> } >=20 > [...] >=20 >> diff --git a/block/qcow2.c b/block/qcow2.c >> index 4718f82..bc78af3 100644 >> --- a/block/qcow2.c >> +++ b/block/qcow2.c >> @@ -2597,7 +2597,15 @@ static int qcow2_write_compressed(BlockDriverSt= ate *bs, int64_t sector_num, >> =20 >> if (ret !=3D Z_STREAM_END || out_len >=3D s->cluster_size) { >> /* could not compress: write normal cluster */ >> - ret =3D bdrv_write(bs, sector_num, buf, s->cluster_sectors); >> + QEMUIOVector qiov; >> + struct iovec iov =3D { >> + .iov_base =3D (uint8_t*) buf, >=20 > [o] ERROR: "(foo*)" should be "(foo *)" >=20 >> + .iov_len =3D nb_sectors * BDRV_SECTOR_SIZE, >> + }; >> + qemu_iovec_init_external(&qiov, &iov, 1); >> + >> + ret =3D bs->drv->bdrv_co_writev(bs, sector_num, s->cluster_se= ctors, >> + &qiov); >=20 > [x] I don't think qcow2 has a bdrv_co_writev(). >=20 > ($ dd if=3D/dev/urandom of=3Dfoo.raw bs=3D64k count=3D1 > $ ./qemu-img convert -c -O qcow2 foo.raw foo.qcow2 > [1] 11808 segmentation fault (core dumped)) >=20 > [r] Same as in qcow.c, I'd rather use nb_sectors instead of > s->cluster_sectors. >=20 > [o] Same as in qcow.c, I'd call qcow2_co_pwritev() directly. Doing so > would have prevented [x]. [x] Same as in qcow.c, qcow2_write_compressed() is not a coroutine_fn either. > Max >=20 >> if (ret < 0) { >> goto fail; >> } >=20 --RdgAOkPF3pV9CeSSD8epo7oOJieBGVKjn 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 iQEvBAEBCAAZBQJXcS5XEhxtcmVpdHpAcmVkaGF0LmNvbQAKCRA7sUIC6DisrbxX B/0dLCZMwH3inYCHTlwRWlSqydDWYR4e+ivrnI9sLPxJ4Ho/uRHB0U4lDbjFARC6 jD0xYPoYAjGYUtET5Dj5fyosWbssS97Qjl2XQMsvY++GgeXhb+CrGvu52C+p4p6f BAUt7Aiqx2g30zyDd9Ky/6rxxK+tgZGyWeEXi3ze0zgkR2zUeQFr3+l8K1Iqfwxm dRplPh0yw+byBNWpvklo0cNSJOMztMA+2D0yyOBj9wZrvka2XILgSP1qWTjBBOq7 ePTZJLmXFYKCtOO4wAR0wvloQtzC6E7kbru2K31+BhztW/llNq9el8ogYK9nWPPd 56Ti+jgl62HoZ153I0x8k0ie =vqIJ -----END PGP SIGNATURE----- --RdgAOkPF3pV9CeSSD8epo7oOJieBGVKjn--