Re: [Qemu-devel] [PATCH for-2.11 3/5] block: Guard against NULL bs->drv

[Eric Blake <eblake@redhat.com>]

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

On 11/10/2017 02:31 PM, Max Reitz wrote:
> We currently do not guard everywhere against a NULL bs->drv where we
> should be doing so.  Most of the places fixed here just do not care
> about that case at all.
> 
> Some care implicitly, e.g. through a prior function call to
> bdrv_getlength() which would always fail for an ejected BDS.  Add an
> assert there to make it more obvious.
> 
> Other places seem to care, but do so insufficiently: Freeing clusters in
> a qcow2 image is an error-free operation, but it may leave the image in
> an unusable state anyway.  Giving qcow2_free_clusters() an error code is
> not really viable, it is much easier to note that bs->drv may be NULL
> even after a successful driver call.  This concerns bdrv_co_flush(), and
> the way the check is added to bdrv_co_pdiscard() (in every iteration
> instead of only once).
> 
> Finally, some places employ at least an assert(bs->drv); somewhere, that
> may be reasonable (such as in the reopen code), but in
> bdrv_has_zero_init(), it is definitely not.  Returning 0 there in case
> of an ejected BDS saves us much headache instead.
> 
> Reported-by: R. Nageswara Sastry <nasastry@in.ibm.com>
> Buglink: https://bugs.launchpad.net/qemu/+bug/1728660
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---

> +++ b/block/replication.c

>  
> +    if (!s->hidden_disk->bs->drv) {
> +        error_setg(errp, "Hidden disk %s is ejected",
> +                   s->hidden_disk->bs->node_name);
> +        return;
> +    }

How would the hidden disk ever be ejected?  Could this be an assert instead?

But what you have is equally safe, so
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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