[Qemu-devel] [PATCH 0/7] Miscellaneous fixes for block and snapshots

[Qemu-devel] [PATCH 0/7] Miscellaneous fixes for block and snapshots

[Paolo Bonzini]

These patches fix a few miscellaneous bugs in live snapshots and block
streaming.

Paolo Bonzini (7):
  block: add mode argument to blockdev-snapshot-sync
  block: fail live snapshot if disk has no medium
  block: fix snapshot with QED format
  block: close unused image files at the end of streaming
  block: pass new base image format to bdrv_change_backing_file
  block: push bdrv_change_backing_file error checking up from drivers
  block: update in-memory backing file and format

 block.c         |   23 +++++++++++++++++++++--
 block/qcow2.c   |    5 -----
 block/qed.c     |    7 +++++++
 block/stream.c  |   30 ++++++++++++++++++++++++++++--
 block/vvfat.c   |    7 +++++++
 block_int.h     |    1 +
 blockdev.c      |    9 ++++++---
 qmp-commands.hx |    2 +-
 8 files changed, 71 insertions(+), 13 deletions(-)

-- 
1.7.9.3

[Qemu-devel] [PATCH 1/7] block: add mode argument to blockdev-snapshot-sync

[Paolo Bonzini]

This was missing in the patch that added the argument.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qmp-commands.hx |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qmp-commands.hx b/qmp-commands.hx
index 9447871..a5accc9 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -762,7 +762,7 @@ EQMP
 
     {
         .name       = "blockdev-snapshot-sync",
-        .args_type  = "device:B,snapshot-file:s,format:s?",
+        .args_type  = "device:B,snapshot-file:s,format:s,mode:s?",
         .mhandler.cmd_new = qmp_marshal_input_blockdev_snapshot_sync,
     },
 
-- 
1.7.9.3

[Qemu-devel] [PATCH 2/7] block: fail live snapshot if disk has no medium

[Paolo Bonzini]

This would cause a segfault accessing states->old_bs->drv.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 blockdev.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 4d17486..be9cdc5 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -752,14 +752,17 @@ void qmp_transaction(BlockdevActionList *dev_list, Error **errp)
             goto delete_and_fail;
         }
 
+        if (!bdrv_is_inserted(states->old_bs)) {
+            error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
+            goto delete_and_fail;
+        }
+
         if (bdrv_in_use(states->old_bs)) {
             error_set(errp, QERR_DEVICE_IN_USE, device);
             goto delete_and_fail;
         }
 
-        if (!bdrv_is_read_only(states->old_bs) &&
-             bdrv_is_inserted(states->old_bs)) {
-
+        if (!bdrv_is_read_only(states->old_bs)) {
             if (bdrv_flush(states->old_bs)) {
                 error_set(errp, QERR_IO_ERROR);
                 goto delete_and_fail;
-- 
1.7.9.3

[Qemu-devel] [PATCH 3/7] block: fix snapshot with QED format

[Paolo Bonzini]

QED's opaque data includes a pointer back to the BlockDriverState.
Creating a snapshot whose format is QED breaks the relationship when
bdrv_append shuffles data between bs_new and bs_top.  To avoid this,
add a "rebind" function that tells the driver about the new relationship
between the BlockDriverState and its opaque.

The patch also adds rebind to VVFAT for completeness, even though it is
not used with live snapshots.

Thanks to Stefan and Jeff for help reproducing and debugging this.

Cc: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
Cc: Jeff Cody <jcody@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block.c       |   10 ++++++++++
 block/qed.c   |    7 +++++++
 block/vvfat.c |    7 +++++++
 block_int.h   |    1 +
 4 files changed, 25 insertions(+)

diff --git a/block.c b/block.c
index b3117ef..a3a2e92 100644
--- a/block.c
+++ b/block.c
@@ -885,6 +885,13 @@ void bdrv_make_anon(BlockDriverState *bs)
     bs->device_name[0] = '\0';
 }
 
+static void bdrv_rebind(BlockDriverState *bs)
+{
+    if (bs->drv && bs->drv->bdrv_rebind) {
+        bs->drv->bdrv_rebind(bs);
+    }
+}
+
 /*
  * Add new bs contents at the top of an image chain while the chain is
  * live, while keeping required fields on the top layer.
@@ -973,6 +980,9 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top)
     bs_new->slice_time        = 0;
     bs_new->slice_start       = 0;
     bs_new->slice_end         = 0;
+
+    bdrv_rebind(bs_new);
+    bdrv_rebind(bs_top);
 }
 
 void bdrv_delete(BlockDriverState *bs)
diff --git a/block/qed.c b/block/qed.c
index 19d87f3..9364212 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -367,6 +367,12 @@ static void qed_cancel_need_check_timer(BDRVQEDState *s)
     qemu_del_timer(s->need_check_timer);
 }
 
+static void bdrv_qed_rebind(BlockDriverState *bs)
+{
+    BDRVQEDState *s = bs->opaque;
+    s->bs = bs;
+}
+
 static int bdrv_qed_open(BlockDriverState *bs, int flags)
 {
     BDRVQEDState *s = bs->opaque;
@@ -1549,6 +1555,7 @@ static BlockDriver bdrv_qed = {
     .create_options           = qed_create_options,
 
     .bdrv_probe               = bdrv_qed_probe,
+    .bdrv_rebind              = bdrv_qed_rebind,
     .bdrv_open                = bdrv_qed_open,
     .bdrv_close               = bdrv_qed_close,
     .bdrv_create              = bdrv_qed_create,
diff --git a/block/vvfat.c b/block/vvfat.c
index 9ef21dd..2dc9d50 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -982,6 +982,12 @@ static BDRVVVFATState *vvv = NULL;
 static int enable_write_target(BDRVVVFATState *s);
 static int is_consistent(BDRVVVFATState *s);
 
+static void vvfat_rebind(BlockDriverState *bs)
+{
+    BDRVVVFATState *s = bs->opaque;
+    s->bs = bs;
+}
+
 static int vvfat_open(BlockDriverState *bs, const char* dirname, int flags)
 {
     BDRVVVFATState *s = bs->opaque;
@@ -2855,6 +2861,7 @@ static BlockDriver bdrv_vvfat = {
     .format_name	= "vvfat",
     .instance_size	= sizeof(BDRVVVFATState),
     .bdrv_file_open	= vvfat_open,
+    .bdrv_rebind	= vvfat_rebind,
     .bdrv_read          = vvfat_co_read,
     .bdrv_write         = vvfat_co_write,
     .bdrv_close		= vvfat_close,
diff --git a/block_int.h b/block_int.h
index 0e5a032..58e3eea 100644
--- a/block_int.h
+++ b/block_int.h
@@ -139,6 +139,7 @@ struct BlockDriver {
     int (*bdrv_write)(BlockDriverState *bs, int64_t sector_num,
                       const uint8_t *buf, int nb_sectors);
     void (*bdrv_close)(BlockDriverState *bs);
+    void (*bdrv_rebind)(BlockDriverState *bs);
     int (*bdrv_create)(const char *filename, QEMUOptionParameter *options);
     int (*bdrv_set_key)(BlockDriverState *bs, const char *key);
     int (*bdrv_make_empty)(BlockDriverState *bs);
-- 
1.7.9.3

[Qemu-devel] [PATCH 5/7] block: pass new base image format to bdrv_change_backing_file

[Paolo Bonzini]

The format was not written to the image, only the file name.

This will overwrite an auto-probe image with the right format.
Format probing is unsafe and we want to avoid it whenever possible.
An alternative would be to look out the backing file from the image
above base.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/stream.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/block/stream.c b/block/stream.c
index 54f2b39..2b492d5 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -240,11 +240,14 @@ retry:
     }
 
     if (!block_job_is_cancelled(&s->common) && sector_num == end && ret == 0) {
-        const char *base_id = NULL;
+        const char *base_id = NULL, *base_fmt = NULL;
         if (base) {
             base_id = s->backing_file_id;
+            if (base->drv) {
+                base_fmt = base->drv->format_name;
+            }
         }
-        ret = bdrv_change_backing_file(bs, base_id, NULL);
+        ret = bdrv_change_backing_file(bs, base_id, base_fmt);
         close_unused_images(bs, base, base_id);
     }
 
-- 
1.7.9.3

[Qemu-devel] [PATCH 6/7] block: push bdrv_change_backing_file error checking up from drivers

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block.c       |    5 +++++
 block/qcow2.c |    5 -----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/block.c b/block.c
index e806ad8..e293882 100644
--- a/block.c
+++ b/block.c
@@ -1403,6 +1403,11 @@ int bdrv_change_backing_file(BlockDriverState *bs,
 {
     BlockDriver *drv = bs->drv;
 
+    /* Backing file format doesn't make sense without a backing file */
+    if (backing_fmt && !backing_file) {
+        return -EINVAL;
+    }
+
     if (drv->bdrv_change_backing_file != NULL) {
         return drv->bdrv_change_backing_file(bs, backing_file, backing_fmt);
     } else {
diff --git a/block/qcow2.c b/block/qcow2.c
index 70d3141..342d8ff 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -843,11 +843,6 @@ fail:
 static int qcow2_change_backing_file(BlockDriverState *bs,
     const char *backing_file, const char *backing_fmt)
 {
-    /* Backing file format doesn't make sense without a backing file */
-    if (backing_fmt && !backing_file) {
-        return -EINVAL;
-    }
-
     pstrcpy(bs->backing_file, sizeof(bs->backing_file), backing_file ?: "");
     pstrcpy(bs->backing_format, sizeof(bs->backing_format), backing_fmt ?: "");
 
-- 
1.7.9.3

[Qemu-devel] [PATCH 7/7] block: update in-memory backing file and format

[Paolo Bonzini]

These are needed to print "info block" output correctly.  QCOW2 does
this already because it needs it to write the header, but QED does not,
and common code is a better place to do it.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block.c |   11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index e293882..afff23f 100644
--- a/block.c
+++ b/block.c
@@ -1402,6 +1402,7 @@ int bdrv_change_backing_file(BlockDriverState *bs,
     const char *backing_file, const char *backing_fmt)
 {
     BlockDriver *drv = bs->drv;
+    int ret;
 
     /* Backing file format doesn't make sense without a backing file */
     if (backing_fmt && !backing_file) {
@@ -1409,10 +1410,16 @@ int bdrv_change_backing_file(BlockDriverState *bs,
     }
 
     if (drv->bdrv_change_backing_file != NULL) {
-        return drv->bdrv_change_backing_file(bs, backing_file, backing_fmt);
+        ret = drv->bdrv_change_backing_file(bs, backing_file, backing_fmt);
     } else {
-        return -ENOTSUP;
+        ret = -ENOTSUP;
+    }
+
+    if (ret == 0) {
+        pstrcpy(bs->backing_file, sizeof(bs->backing_file), backing_file ?: "");
+        pstrcpy(bs->backing_format, sizeof(bs->backing_format), backing_fmt ?: "");
     }
+    return ret;
 }
 
 static int bdrv_check_byte_request(BlockDriverState *bs, int64_t offset,
-- 
1.7.9.3