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]>
Message-id:
20171110203111[email protected]
Reviewed-by: Eric Blake <[email protected]>
Signed-off-by: Max Reitz <[email protected]>
{
BlockDriver *drv = bs->drv;
+ if (!drv) {
+ return -ENOMEDIUM;
+ }
+
/* Do not attempt drv->bdrv_getlength() on scsi-generic devices */
if (bdrv_is_sg(bs))
return 0;
BlockDriver *drv = bs->drv;
int ret;
+ if (!drv) {
+ return -ENOMEDIUM;
+ }
+
/* Backing file format doesn't make sense without a backing file */
if (backing_fmt && !backing_file) {
return -EINVAL;
int bdrv_has_zero_init(BlockDriverState *bs)
{
- assert(bs->drv);
+ if (!bs->drv) {
+ return 0;
+ }
/* If BS is a copy on write image, it is initialized to
the contents of the base image, which may not be zeroes. */
BdrvChild *child, *parent;
int ret;
+ if (!bs->drv) {
+ return -ENOMEDIUM;
+ }
+
if (!setting_flag && bs->drv->bdrv_inactivate) {
ret = bs->drv->bdrv_inactivate(bs);
if (ret < 0) {
int bdrv_amend_options(BlockDriverState *bs, QemuOpts *opts,
BlockDriverAmendStatusCB *status_cb, void *cb_opaque)
{
+ if (!bs->drv) {
+ return -ENOMEDIUM;
+ }
if (!bs->drv->bdrv_amend_options) {
return -ENOTSUP;
}
assert(!(flags & ~BDRV_REQ_MASK));
+ if (!drv) {
+ return -ENOMEDIUM;
+ }
+
if (drv->bdrv_co_preadv) {
return drv->bdrv_co_preadv(bs, offset, bytes, qiov, flags);
}
assert(!(flags & ~BDRV_REQ_MASK));
+ if (!drv) {
+ return -ENOMEDIUM;
+ }
+
if (drv->bdrv_co_pwritev) {
ret = drv->bdrv_co_pwritev(bs, offset, bytes, qiov,
flags & bs->supported_write_flags);
{
BlockDriver *drv = bs->drv;
+ if (!drv) {
+ return -ENOMEDIUM;
+ }
+
if (!drv->bdrv_co_pwritev_compressed) {
return -ENOTSUP;
}
BDRV_REQUEST_MAX_BYTES);
unsigned int progress = 0;
+ if (!drv) {
+ return -ENOMEDIUM;
+ }
+
/* FIXME We cannot require callers to have write permissions when all they
* are doing is a read request. If we did things right, write permissions
* would be obtained anyway, but internally by the copy-on-read code. As
bs->bl.request_alignment);
int max_transfer = MIN_NON_ZERO(bs->bl.max_transfer, MAX_BOUNCE_BUFFER);
+ if (!drv) {
+ return -ENOMEDIUM;
+ }
+
assert(alignment % bs->bl.request_alignment == 0);
head = offset % alignment;
tail = (offset + bytes) % alignment;
uint64_t bytes_remaining = bytes;
int max_transfer;
+ if (!drv) {
+ return -ENOMEDIUM;
+ }
+
if (bdrv_has_readonly_bitmaps(bs)) {
return -EPERM;
}
bytes = n;
}
+ /* Must be non-NULL or bdrv_getlength() would have failed */
+ assert(bs->drv);
if (!bs->drv->bdrv_co_get_block_status) {
*pnum = bytes;
ret = BDRV_BLOCK_DATA | BDRV_BLOCK_ALLOCATED;
}
BLKDBG_EVENT(bs->file, BLKDBG_FLUSH_TO_DISK);
+ if (!bs->drv) {
+ /* bs->drv->bdrv_co_flush() might have ejected the BDS
+ * (even in case of apparent success) */
+ ret = -ENOMEDIUM;
+ goto out;
+ }
if (bs->drv->bdrv_co_flush_to_disk) {
ret = bs->drv->bdrv_co_flush_to_disk(bs);
} else if (bs->drv->bdrv_aio_flush) {
num = max_pdiscard;
}
+ if (!bs->drv) {
+ ret = -ENOMEDIUM;
+ goto out;
+ }
if (bs->drv->bdrv_co_pdiscard) {
ret = bs->drv->bdrv_co_pdiscard(bs, offset, num);
} else {
{
ImageInfo **p_image_info;
BlockDriverState *bs0;
- BlockDeviceInfo *info = g_malloc0(sizeof(*info));
+ BlockDeviceInfo *info;
+ if (!bs->drv) {
+ error_setg(errp, "Block device %s is ejected", bs->node_name);
+ return NULL;
+ }
+
+ info = g_malloc0(sizeof(*info));
info->file = g_strdup(bs->filename);
info->ro = bs->read_only;
info->drv = g_strdup(bs->drv->format_name);
return;
}
+ if (!s->active_disk->bs->drv) {
+ error_setg(errp, "Active disk %s is ejected",
+ s->active_disk->bs->node_name);
+ return;
+ }
+
ret = s->active_disk->bs->drv->bdrv_make_empty(s->active_disk->bs);
if (ret < 0) {
error_setg(errp, "Cannot make active disk empty");
return;
}
+ if (!s->hidden_disk->bs->drv) {
+ error_setg(errp, "Hidden disk %s is ejected",
+ s->hidden_disk->bs->node_name);
+ return;
+ }
+
ret = s->hidden_disk->bs->drv->bdrv_make_empty(s->hidden_disk->bs);
if (ret < 0) {
error_setg(errp, "Cannot make hidden disk empty");
return;
}
+ /* Must be true, or the bdrv_getlength() calls would have failed */
+ assert(s->active_disk->bs->drv && s->hidden_disk->bs->drv);
+
if (!s->active_disk->bs->drv->bdrv_make_empty ||
!s->hidden_disk->bs->drv->bdrv_make_empty) {
error_setg(errp,
return ret;
}
- if (s->qcow->bs->drv->bdrv_make_empty) {
+ if (s->qcow->bs->drv && s->qcow->bs->drv->bdrv_make_empty) {
s->qcow->bs->drv->bdrv_make_empty(s->qcow->bs);
}
# Can't repair this yet (TODO: We can just deallocate the cluster)
+echo
+echo '=== Discarding with an unaligned refblock ==='
+echo
+
+_make_test_img 64M
+
+$QEMU_IO -c "write 0 128k" "$TEST_IMG" | _filter_qemu_io
+# Make our refblock unaligned
+poke_file "$TEST_IMG" "$(($rt_offset))" "\x00\x00\x00\x00\x00\x00\x2a\x00"
+# Now try to discard something that will be submitted as two requests
+# (main part + tail)
+$QEMU_IO -c "discard 0 65537" "$TEST_IMG"
+
+echo '--- Repairing ---'
+# Fails the first repair because the corruption prevents the check
+# function from double-checking
+# (Using -q for the first invocation, because otherwise the
+# double-check error message appears above the summary for some
+# reason -- so let's just hide the summary)
+_check_test_img -q -r all
+_check_test_img -r all
+
# success, all done
echo "*** done"
rm -f $seq.full
64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
qcow2: Marking image as corrupt: Preallocated zero cluster offset 0x2a00 unaligned (guest offset: 0); further corruption events will be suppressed
write failed: Input/output error
+
+=== Discarding with an unaligned refblock ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+wrote 131072/131072 bytes at offset 0
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+qcow2: Marking image as corrupt: Refblock offset 0x2a00 unaligned (reftable index: 0); further corruption events will be suppressed
+qcow2_free_clusters failed: Input/output error
+discard failed: No medium found
+--- Repairing ---
+ERROR refcount block 0 is not cluster aligned; refcount table entry corrupted
+qcow2: Marking image as corrupt: Refblock offset 0x2a00 unaligned (reftable index: 0); further corruption events will be suppressed
+Can't get refcount for cluster 0: Input/output error
+Can't get refcount for cluster 1: Input/output error
+Can't get refcount for cluster 2: Input/output error
+Can't get refcount for cluster 3: Input/output error
+Can't get refcount for cluster 4: Input/output error
+Can't get refcount for cluster 5: Input/output error
+Can't get refcount for cluster 6: Input/output error
+Rebuilding refcount structure
+Repairing cluster 1 refcount=1 reference=0
+qemu-img: Check failed: No medium found
+Leaked cluster 1 refcount=1 reference=0
+Repairing cluster 1 refcount=1 reference=0
+The following inconsistencies were found and repaired:
+
+ 1 leaked clusters
+ 0 corruptions
+
+Double checking the fixed image now...
+No errors were found on the image.
*** done
projects / qemu.git / commitdiff