From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47456) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VF9sQ-00082e-EG for qemu-devel@nongnu.org; Thu, 29 Aug 2013 17:35:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VF9sK-00010r-Go for qemu-devel@nongnu.org; Thu, 29 Aug 2013 17:35:38 -0400 Message-ID: <521FBEA2.3090703@redhat.com> Date: Thu, 29 Aug 2013 15:35:30 -0600 From: Eric Blake MIME-Version: 1.0 References: <1377784821-29561-1-git-send-email-pbonzini@redhat.com> <1377784821-29561-9-git-send-email-pbonzini@redhat.com> In-Reply-To: <1377784821-29561-9-git-send-email-pbonzini@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="oIhGnkdh7jOEG5H7jUBub5LF9XVXpxQ4M" Subject: Re: [Qemu-devel] [PATCH v4 08/21] block: expect errors from bdrv_co_is_allocated List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: kwolf@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, qemu-stable@nongnu.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --oIhGnkdh7jOEG5H7jUBub5LF9XVXpxQ4M Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 08/29/2013 08:00 AM, Paolo Bonzini wrote: Subject line mentions bdrv_co_is_allocated, but patch body deals with bdrv_is_allocated. > Some bdrv_is_allocated callers do not expect errors, but the fallback > in qcow2.c might make other callers trip on assertion failures or > infinite loops. >=20 > Fix the callers to always look for errors. >=20 > Cc: qemu-stable@nongnu.org > Reviewed-by: Eric Blake > Signed-off-by: Paolo Bonzini > --- > v4: also fix bdrv_commit, cow_read, img_convert, alloc_f Hmm - the v4 changelog implies things changed since my review. Thankfully, this patch still looks sane when looking at just this patch (what you have is good). But your comment made me grep the rest of the source code for ALL bdrv_is_allocated callers (since that's the harder task - ensuring we didn't forget anything): block-migration.c uses !bdrv_is_allocated as a condition for a while loop; should that check for errors? block/vvfat.c contains an if (bdrv_is_allocated(...)); should that handle errors? If you can justify that those don't need changes, then I'm okay with: Reviewed-by: Eric Blake > block.c | 7 +++++-- > block/cow.c | 6 +++++- > block/qcow2.c | 4 +--- > block/stream.c | 2 +- > qemu-img.c | 16 ++++++++++++++-- > qemu-io-cmds.c | 4 ++++ > 6 files changed, 30 insertions(+), 9 deletions(-) >=20 --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --oIhGnkdh7jOEG5H7jUBub5LF9XVXpxQ4M 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.4.14 (GNU/Linux) Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJSH76iAAoJEKeha0olJ0NqUWQH/ivNnZkkTGOVr0VtT+c0DKCd VDTyeG8LtZ+0PcoMSnjss6RF057HqT1SB0Wg52rtd+ubdcNdbI8xItNqLz23ENuk 9LEhuCugDlBJGn/i0BdPkQ334ARucmuzNOvUIkcgdQe2tOOnwrIS1BO5wAP5kqsb YabRmvr/J+mSc0L2kNB+AMqQwdfJX5GlI8gqkYpmd147Xy5BCLQxyTRAGMDgFkPQ lEITzZjScxFcc5MjJ+JMtkTmhl87U1YFl/Z5yRdpEC/bi3hTE+Gm9RjX+j1/4WBT EH4ljImSdSiPn8w3SdWLbOPuZR9C6EiwhYdoUAwlvePxTBWJYczI2E9RAzS5Q4A= =2y/K -----END PGP SIGNATURE----- --oIhGnkdh7jOEG5H7jUBub5LF9XVXpxQ4M--