Re: [Qemu-devel] [PATCH v4 08/21] block: expect errors from bdrv_co_is_allocated

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1568 bytes --]

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.
> 
> Fix the callers to always look for errors.
> 
> Cc: qemu-stable@nongnu.org
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>         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 <eblake@redhat.com>

>  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(-)
> 


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]