On 2017-11-10 22:46, Eric Blake wrote: > 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 <[email protected]> >> Buglink: https://bugs.launchpad.net/qemu/+bug/1728660 >> Signed-off-by: Max Reitz <[email protected]> >> --- > >> +++ 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?
Maybe? :-) Isn't the hidden disk usually a qcow2 file? As such I guess there can be corruptions in it that make the qcow2 driver eject it (even though qemu isn't writing to it). Max > But what you have is equally safe, so > Reviewed-by: Eric Blake <[email protected]>
signature.asc
Description: OpenPGP digital signature
