[Qemu-devel] [PULL v2 00/40] Block layer patches

[Qemu-devel] [PULL v2 00/40] Block layer patches

[Kevin Wolf]

The following changes since commit a8b4f9585a0bf5186fca793ce2c5d754cd8ec49a:

  Merge remote-tracking branch 'remotes/armbru/tags/pull-qapi-2015-11-10' into staging (2015-11-10 09:39:24 +0000)

are available in the git repository at:


  git://repo.or.cz/qemu/kevin.git tags/for-upstream

for you to fetch changes up to c400bddb916268394e352f82809eb4728424a5b1:

  Merge remote-tracking branch 'mreitz/tags/pull-block-for-kevin-2015-11-10' into queue-block (2015-11-10 14:59:26 +0100)

----------------------------------------------------------------

Block layer patches

----------------------------------------------------------------
Alberto Garcia (17):
      block: Don't call blk_bs() twice in bdrv_lookup_bs()
      block: check for existing device IDs in external_snapshot_prepare()
      block: rename BlockdevSnapshot to BlockdevSnapshotSync
      block: support passing 'backing': '' to 'blockdev-add'
      block: add a 'blockdev-snapshot' QMP command
      block: add tests for the 'blockdev-snapshot' command
      commit: reopen overlay_bs before base
      qemu-iotests: Test the reopening of overlay_bs in 'block-commit'
      throttle: Check for pending requests in throttle_group_unregister_bs()
      throttle: Use bs->throttle_state instead of bs->io_limits_enabled
      block: Disallow snapshots if the overlay doesn't support backing files
      block: Remove inner quotation marks in iotest 085
      block: test 'blockdev-snapshot' using a file BDS as the overlay
      mirror: block all operations on the target image during the job
      block: Add blk_get_refcnt()
      block: Add 'x-blockdev-del' QMP command
      iotests: Add tests for the x-blockdev-del command

Jeff Cody (2):
      qemu-iotests: fix cleanup of background processes
      qemu-iotests: fix -valgrind option for check

John Snow (5):
      qcow2: avoid misaligned 64bit bswap
      qemu-img: add check for zero-length job len
      qemu-io: fix cvtnum lval types
      qemu-io: Check for trailing chars
      qemu-io: Correct error messages

Kevin Wolf (2):
      qcow2: Fix qcow2_get_cluster_offset() for zero clusters
      Merge remote-tracking branch 'mreitz/tags/pull-block-for-kevin-2015-11-10' into queue-block

Max Reitz (15):
      block: Add blk_remove_bs()
      block: Make bdrv_states public
      block: Add functions for inheriting a BBRS
      blockdev: Add blockdev-open-tray
      blockdev: Add blockdev-close-tray
      blockdev: Add blockdev-remove-medium
      blockdev: Add blockdev-insert-medium
      blockdev: Implement eject with basic operations
      blockdev: Implement change with basic operations
      block: Inquire tray state before tray-moved events
      qmp: Introduce blockdev-change-medium
      hmp: Use blockdev-change-medium for change command
      blockdev: read-only-mode for blockdev-change-medium
      hmp: Add read-only-mode option to change command
      iotests: Add test for change-related QMP commands

 block.c                          |  22 +-
 block/block-backend.c            |  61 +++-
 block/commit.c                   |   8 +-
 block/mirror.c                   |   4 +
 block/qapi.c                     |   2 +-
 block/qcow2-cluster.c            |  15 +-
 block/qcow2-refcount.c           |  11 +-
 block/throttle-groups.c          |   7 +
 blockdev.c                       | 527 ++++++++++++++++++++--------
 hmp-commands.hx                  |  20 +-
 hmp.c                            |  47 ++-
 include/block/block_int.h        |   7 +-
 include/sysemu/block-backend.h   |   4 +
 include/sysemu/blockdev.h        |   2 -
 qapi-schema.json                 |  10 +-
 qapi/block-core.json             | 192 ++++++++++-
 qemu-img.c                       |   3 +-
 qemu-io-cmds.c                   | 185 +++++++---
 qmp-commands.hx                  | 317 ++++++++++++++++-
 qmp.c                            |   3 +-
 tests/qemu-iotests/039.out       |  30 +-
 tests/qemu-iotests/040           |  30 ++
 tests/qemu-iotests/040.out       |   4 +-
 tests/qemu-iotests/058           |  12 +-
 tests/qemu-iotests/061.out       |  12 +-
 tests/qemu-iotests/085           | 120 ++++++-
 tests/qemu-iotests/085.out       |  38 ++-
 tests/qemu-iotests/118           | 720 +++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/118.out       |   5 +
 tests/qemu-iotests/137.out       |   6 +-
 tests/qemu-iotests/139           | 414 ++++++++++++++++++++++
 tests/qemu-iotests/139.out       |   5 +
 tests/qemu-iotests/common        |   9 +-
 tests/qemu-iotests/common.config |  32 +-
 tests/qemu-iotests/common.qemu   |  18 +-
 tests/qemu-iotests/common.rc     |  18 +-
 tests/qemu-iotests/group         |   2 +
 ui/cocoa.m                       |  10 +-
 38 files changed, 2621 insertions(+), 311 deletions(-)
 create mode 100755 tests/qemu-iotests/118
 create mode 100644 tests/qemu-iotests/118.out
 create mode 100644 tests/qemu-iotests/139
 create mode 100644 tests/qemu-iotests/139.out

[Qemu-devel] [PULL v2 01/40] block: Don't call blk_bs() twice in bdrv_lookup_bs()

[Kevin Wolf]

From: Alberto Garcia <berto@igalia.com>

Signed-off-by: Alberto Garcia <berto@igalia.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/block.c b/block.c
index e9f40dc..eb8158a 100644
--- a/block.c
+++ b/block.c
@@ -2683,12 +2683,12 @@ BlockDriverState *bdrv_lookup_bs(const char *device,
         blk = blk_by_name(device);
 
         if (blk) {
-            if (!blk_bs(blk)) {
+            bs = blk_bs(blk);
+            if (!bs) {
                 error_setg(errp, "Device '%s' has no medium", device);
-                return NULL;
             }
 
-            return blk_bs(blk);
+            return bs;
         }
     }
 
-- 
1.8.3.1

[Qemu-devel] [PULL v2 02/40] block: Add blk_remove_bs()

[Kevin Wolf]

From: Max Reitz <mreitz@redhat.com>

This function removes the BlockDriverState associated with the given
BlockBackend from that BB and sets the BDS pointer in the BB to NULL.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/block-backend.c          | 12 ++++++++++++
 include/sysemu/block-backend.h |  1 +
 2 files changed, 13 insertions(+)

diff --git a/block/block-backend.c b/block/block-backend.c
index 19fdaae..878c448 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -334,6 +334,18 @@ void blk_hide_on_behalf_of_hmp_drive_del(BlockBackend *blk)
 }
 
 /*
+ * Disassociates the currently associated BlockDriverState from @blk.
+ */
+void blk_remove_bs(BlockBackend *blk)
+{
+    blk_update_root_state(blk);
+
+    blk->bs->blk = NULL;
+    bdrv_unref(blk->bs);
+    blk->bs = NULL;
+}
+
+/*
  * Associates a new BlockDriverState with @blk.
  */
 void blk_insert_bs(BlockBackend *blk, BlockDriverState *bs)
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 9306a52..14a6d32 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -72,6 +72,7 @@ BlockBackend *blk_by_name(const char *name);
 BlockBackend *blk_next(BlockBackend *blk);
 
 BlockDriverState *blk_bs(BlockBackend *blk);
+void blk_remove_bs(BlockBackend *blk);
 void blk_insert_bs(BlockBackend *blk, BlockDriverState *bs);
 
 void blk_hide_on_behalf_of_hmp_drive_del(BlockBackend *blk);
-- 
1.8.3.1

[Qemu-devel] [PULL v2 03/40] block: Make bdrv_states public

[Kevin Wolf]

From: Max Reitz <mreitz@redhat.com>

When inserting a BDS tree into a BB, we will need to add the root BDS to
this list. Since we will want to do that in the blockdev-insert-medium
implementation in blockdev.c, we will need access to it there.

This patch is not exactly elegant, but bdrv_states will be removed in
the future anyway because we no longer need it since we have BBs.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c                   | 3 +--
 include/block/block_int.h | 2 ++
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index eb8158a..a99e6d8 100644
--- a/block.c
+++ b/block.c
@@ -73,8 +73,7 @@ struct BdrvDirtyBitmap {
 
 #define NOT_DONE 0x7fffffff /* used while emulated sync operation in progress */
 
-static QTAILQ_HEAD(, BlockDriverState) bdrv_states =
-    QTAILQ_HEAD_INITIALIZER(bdrv_states);
+struct BdrvStates bdrv_states = QTAILQ_HEAD_INITIALIZER(bdrv_states);
 
 static QTAILQ_HEAD(, BlockDriverState) graph_bdrv_states =
     QTAILQ_HEAD_INITIALIZER(graph_bdrv_states);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 3ceeb5a..6a3f64d 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -473,6 +473,8 @@ extern BlockDriver bdrv_file;
 extern BlockDriver bdrv_raw;
 extern BlockDriver bdrv_qcow2;
 
+extern QTAILQ_HEAD(BdrvStates, BlockDriverState) bdrv_states;
+
 /**
  * bdrv_setup_io_funcs:
  *
-- 
1.8.3.1

[Qemu-devel] [PULL v2 04/40] block: Add functions for inheriting a BBRS

[Kevin Wolf]

From: Max Reitz <mreitz@redhat.com>

In order to open a BDS which inherits a BB's root state,
blk_get_open_flags_from_root_state() is used to inquire the flags to be
passed to bdrv_open(), and blk_apply_root_state() is used to apply the
remaining state after the BDS has been opened.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/block-backend.c          | 27 +++++++++++++++++++++++++++
 include/sysemu/block-backend.h |  2 ++
 2 files changed, 29 insertions(+)

diff --git a/block/block-backend.c b/block/block-backend.c
index 878c448..7d49539 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1239,6 +1239,33 @@ void blk_update_root_state(BlockBackend *blk)
     }
 }
 
+/*
+ * Applies the information in the root state to the given BlockDriverState. This
+ * does not include the flags which have to be specified for bdrv_open(), use
+ * blk_get_open_flags_from_root_state() to inquire them.
+ */
+void blk_apply_root_state(BlockBackend *blk, BlockDriverState *bs)
+{
+    bs->detect_zeroes = blk->root_state.detect_zeroes;
+    if (blk->root_state.throttle_group) {
+        bdrv_io_limits_enable(bs, blk->root_state.throttle_group);
+    }
+}
+
+/*
+ * Returns the flags to be used for bdrv_open() of a BlockDriverState which is
+ * supposed to inherit the root state.
+ */
+int blk_get_open_flags_from_root_state(BlockBackend *blk)
+{
+    int bs_flags;
+
+    bs_flags = blk->root_state.read_only ? 0 : BDRV_O_RDWR;
+    bs_flags |= blk->root_state.open_flags & ~BDRV_O_RDWR;
+
+    return bs_flags;
+}
+
 BlockBackendRootState *blk_get_root_state(BlockBackend *blk)
 {
     return &blk->root_state;
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 14a6d32..40e315b 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -167,6 +167,8 @@ void blk_io_unplug(BlockBackend *blk);
 BlockAcctStats *blk_get_stats(BlockBackend *blk);
 BlockBackendRootState *blk_get_root_state(BlockBackend *blk);
 void blk_update_root_state(BlockBackend *blk);
+void blk_apply_root_state(BlockBackend *blk, BlockDriverState *bs);
+int blk_get_open_flags_from_root_state(BlockBackend *blk);
 
 void *blk_aio_get(const AIOCBInfo *aiocb_info, BlockBackend *blk,
                   BlockCompletionFunc *cb, void *opaque);
-- 
1.8.3.1

[Qemu-devel] [PULL v2 05/40] blockdev: Add blockdev-open-tray

[Kevin Wolf]

From: Max Reitz <mreitz@redhat.com>

Signed-off-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 blockdev.c           | 36 ++++++++++++++++++++++++++++++++++++
 qapi/block-core.json | 32 ++++++++++++++++++++++++++++++++
 qmp-commands.hx      | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 116 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index 97be42f..d01a99e 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2088,6 +2088,42 @@ out:
     aio_context_release(aio_context);
 }
 
+void qmp_blockdev_open_tray(const char *device, bool has_force, bool force,
+                            Error **errp)
+{
+    BlockBackend *blk;
+    bool locked;
+
+    if (!has_force) {
+        force = false;
+    }
+
+    blk = blk_by_name(device);
+    if (!blk) {
+        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
+                  "Device '%s' not found", device);
+        return;
+    }
+
+    if (!blk_dev_has_removable_media(blk)) {
+        error_setg(errp, "Device '%s' is not removable", device);
+        return;
+    }
+
+    if (blk_dev_is_tray_open(blk)) {
+        return;
+    }
+
+    locked = blk_dev_is_medium_locked(blk);
+    if (locked) {
+        blk_dev_eject_request(blk, force);
+    }
+
+    if (!locked || force) {
+        blk_dev_change_media_cb(blk, false);
+    }
+}
+
 /* throttling disk I/O limits */
 void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd,
                                int64_t bps_wr,
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 425fdab..b65ab81 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1876,6 +1876,38 @@
 ##
 { 'command': 'blockdev-add', 'data': { 'options': 'BlockdevOptions' } }
 
+##
+# @blockdev-open-tray:
+#
+# Opens a block device's tray. If there is a block driver state tree inserted as
+# a medium, it will become inaccessible to the guest (but it will remain
+# associated to the block device, so closing the tray will make it accessible
+# again).
+#
+# If the tray was already open before, this will be a no-op.
+#
+# Once the tray opens, a DEVICE_TRAY_MOVED event is emitted. There are cases in
+# which no such event will be generated, these include:
+# - if the guest has locked the tray, @force is false and the guest does not
+#   respond to the eject request
+# - if the BlockBackend denoted by @device does not have a guest device attached
+#   to it
+# - if the guest device does not have an actual tray and is empty, for instance
+#   for floppy disk drives
+#
+# @device: block device name
+#
+# @force:  #optional if false (the default), an eject request will be sent to
+#          the guest if it has locked the tray (and the tray will not be opened
+#          immediately); if true, the tray will be opened regardless of whether
+#          it is locked
+#
+# Since: 2.5
+##
+{ 'command': 'blockdev-open-tray',
+  'data': { 'device': 'str',
+            '*force': 'bool' } }
+
 
 ##
 # @BlockErrorAction
diff --git a/qmp-commands.hx b/qmp-commands.hx
index d7cf0ff..0b796f9 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -3936,6 +3936,54 @@ Example (2):
 EQMP
 
     {
+        .name       = "blockdev-open-tray",
+        .args_type  = "device:s,force:b?",
+        .mhandler.cmd_new = qmp_marshal_blockdev_open_tray,
+    },
+
+SQMP
+blockdev-open-tray
+------------------
+
+Opens a block device's tray. If there is a block driver state tree inserted as a
+medium, it will become inaccessible to the guest (but it will remain associated
+to the block device, so closing the tray will make it accessible again).
+
+If the tray was already open before, this will be a no-op.
+
+Once the tray opens, a DEVICE_TRAY_MOVED event is emitted. There are cases in
+which no such event will be generated, these include:
+- if the guest has locked the tray, @force is false and the guest does not
+  respond to the eject request
+- if the BlockBackend denoted by @device does not have a guest device attached
+  to it
+- if the guest device does not have an actual tray and is empty, for instance
+  for floppy disk drives
+
+Arguments:
+
+- "device": block device name (json-string)
+- "force": if false (the default), an eject request will be sent to the guest if
+           it has locked the tray (and the tray will not be opened immediately);
+           if true, the tray will be opened regardless of whether it is locked
+           (json-bool, optional)
+
+Example:
+
+-> { "execute": "blockdev-open-tray",
+     "arguments": { "device": "ide1-cd0" } }
+
+<- { "timestamp": { "seconds": 1418751016,
+                    "microseconds": 716996 },
+     "event": "DEVICE_TRAY_MOVED",
+     "data": { "device": "ide1-cd0",
+               "tray-open": true } }
+
+<- { "return": {} }
+
+EQMP
+
+    {
         .name       = "query-named-block-nodes",
         .args_type  = "",
         .mhandler.cmd_new = qmp_marshal_query_named_block_nodes,
-- 
1.8.3.1

[Qemu-devel] [PULL v2 06/40] blockdev: Add blockdev-close-tray

[Kevin Wolf]

From: Max Reitz <mreitz@redhat.com>

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 blockdev.c           | 23 +++++++++++++++++++++++
 qapi/block-core.json | 16 ++++++++++++++++
 qmp-commands.hx      | 35 +++++++++++++++++++++++++++++++++++
 3 files changed, 74 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index d01a99e..cb8a2f0 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2124,6 +2124,29 @@ void qmp_blockdev_open_tray(const char *device, bool has_force, bool force,
     }
 }
 
+void qmp_blockdev_close_tray(const char *device, Error **errp)
+{
+    BlockBackend *blk;
+
+    blk = blk_by_name(device);
+    if (!blk) {
+        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
+                  "Device '%s' not found", device);
+        return;
+    }
+
+    if (!blk_dev_has_removable_media(blk)) {
+        error_setg(errp, "Device '%s' is not removable", device);
+        return;
+    }
+
+    if (!blk_dev_is_tray_open(blk)) {
+        return;
+    }
+
+    blk_dev_change_media_cb(blk, true);
+}
+
 /* throttling disk I/O limits */
 void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd,
                                int64_t bps_wr,
diff --git a/qapi/block-core.json b/qapi/block-core.json
index b65ab81..1cb719a 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1908,6 +1908,22 @@
   'data': { 'device': 'str',
             '*force': 'bool' } }
 
+##
+# @blockdev-close-tray:
+#
+# Closes a block device's tray. If there is a block driver state tree associated
+# with the block device (which is currently ejected), that tree will be loaded
+# as the medium.
+#
+# If the tray was already closed before, this will be a no-op.
+#
+# @device: block device name
+#
+# Since: 2.5
+##
+{ 'command': 'blockdev-close-tray',
+  'data': { 'device': 'str' } }
+
 
 ##
 # @BlockErrorAction
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 0b796f9..4b16d73 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -3984,6 +3984,41 @@ Example:
 EQMP
 
     {
+        .name       = "blockdev-close-tray",
+        .args_type  = "device:s",
+        .mhandler.cmd_new = qmp_marshal_blockdev_close_tray,
+    },
+
+SQMP
+blockdev-close-tray
+-------------------
+
+Closes a block device's tray. If there is a block driver state tree associated
+with the block device (which is currently ejected), that tree will be loaded as
+the medium.
+
+If the tray was already closed before, this will be a no-op.
+
+Arguments:
+
+- "device": block device name (json-string)
+
+Example:
+
+-> { "execute": "blockdev-close-tray",
+     "arguments": { "device": "ide1-cd0" } }
+
+<- { "timestamp": { "seconds": 1418751345,
+                    "microseconds": 272147 },
+     "event": "DEVICE_TRAY_MOVED",
+     "data": { "device": "ide1-cd0",
+               "tray-open": false } }
+
+<- { "return": {} }
+
+EQMP
+
+    {
         .name       = "query-named-block-nodes",
         .args_type  = "",
         .mhandler.cmd_new = qmp_marshal_query_named_block_nodes,
-- 
1.8.3.1

[Qemu-devel] [PULL v2 07/40] blockdev: Add blockdev-remove-medium

[Kevin Wolf]

From: Max Reitz <mreitz@redhat.com>

Signed-off-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 blockdev.c           | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi/block-core.json | 16 ++++++++++++++++
 qmp-commands.hx      | 45 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 112 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index cb8a2f0..25635e3 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2147,6 +2147,57 @@ void qmp_blockdev_close_tray(const char *device, Error **errp)
     blk_dev_change_media_cb(blk, true);
 }
 
+void qmp_blockdev_remove_medium(const char *device, Error **errp)
+{
+    BlockBackend *blk;
+    BlockDriverState *bs;
+    AioContext *aio_context;
+    bool has_device;
+
+    blk = blk_by_name(device);
+    if (!blk) {
+        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
+                  "Device '%s' not found", device);
+        return;
+    }
+
+    /* For BBs without a device, we can exchange the BDS tree at will */
+    has_device = blk_get_attached_dev(blk);
+
+    if (has_device && !blk_dev_has_removable_media(blk)) {
+        error_setg(errp, "Device '%s' is not removable", device);
+        return;
+    }
+
+    if (has_device && !blk_dev_is_tray_open(blk)) {
+        error_setg(errp, "Tray of device '%s' is not open", device);
+        return;
+    }
+
+    bs = blk_bs(blk);
+    if (!bs) {
+        return;
+    }
+
+    aio_context = bdrv_get_aio_context(bs);
+    aio_context_acquire(aio_context);
+
+    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_EJECT, errp)) {
+        goto out;
+    }
+
+    /* This follows the convention established by bdrv_make_anon() */
+    if (bs->device_list.tqe_prev) {
+        QTAILQ_REMOVE(&bdrv_states, bs, device_list);
+        bs->device_list.tqe_prev = NULL;
+    }
+
+    blk_remove_bs(blk);
+
+out:
+    aio_context_release(aio_context);
+}
+
 /* throttling disk I/O limits */
 void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd,
                                int64_t bps_wr,
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 1cb719a..e19e82c 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1924,6 +1924,22 @@
 { 'command': 'blockdev-close-tray',
   'data': { 'device': 'str' } }
 
+##
+# @blockdev-remove-medium:
+#
+# Removes a medium (a block driver state tree) from a block device. That block
+# device's tray must currently be open (unless there is no attached guest
+# device).
+#
+# If the tray is open and there is no medium inserted, this will be a no-op.
+#
+# @device: block device name
+#
+# Since: 2.5
+##
+{ 'command': 'blockdev-remove-medium',
+  'data': { 'device': 'str' } }
+
 
 ##
 # @BlockErrorAction
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 4b16d73..6165398 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -4019,6 +4019,51 @@ Example:
 EQMP
 
     {
+        .name       = "blockdev-remove-medium",
+        .args_type  = "device:s",
+        .mhandler.cmd_new = qmp_marshal_blockdev_remove_medium,
+    },
+
+SQMP
+blockdev-remove-medium
+----------------------
+
+Removes a medium (a block driver state tree) from a block device. That block
+device's tray must currently be open (unless there is no attached guest device).
+
+If the tray is open and there is no medium inserted, this will be a no-op.
+
+Arguments:
+
+- "device": block device name (json-string)
+
+Example:
+
+-> { "execute": "blockdev-remove-medium",
+     "arguments": { "device": "ide1-cd0" } }
+
+<- { "error": { "class": "GenericError",
+                "desc": "Tray of device 'ide1-cd0' is not open" } }
+
+-> { "execute": "blockdev-open-tray",
+     "arguments": { "device": "ide1-cd0" } }
+
+<- { "timestamp": { "seconds": 1418751627,
+                    "microseconds": 549958 },
+     "event": "DEVICE_TRAY_MOVED",
+     "data": { "device": "ide1-cd0",
+               "tray-open": true } }
+
+<- { "return": {} }
+
+-> { "execute": "blockdev-remove-medium",
+     "arguments": { "device": "ide1-cd0" } }
+
+<- { "return": {} }
+
+EQMP
+
+    {
         .name       = "query-named-block-nodes",
         .args_type  = "",
         .mhandler.cmd_new = qmp_marshal_query_named_block_nodes,
-- 
1.8.3.1

[Qemu-devel] [PULL v2 08/40] blockdev: Add blockdev-insert-medium

[Kevin Wolf]

From: Max Reitz <mreitz@redhat.com>

And a helper function for that, which directly takes a pointer to the
BDS to be inserted instead of its node-name (which will be used for
implementing 'change' using blockdev-insert-medium).

Signed-off-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 blockdev.c           | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi/block-core.json | 17 ++++++++++++++++
 qmp-commands.hx      | 37 ++++++++++++++++++++++++++++++++++
 3 files changed, 110 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index 25635e3..0f2a7e2 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2198,6 +2198,62 @@ out:
     aio_context_release(aio_context);
 }
 
+static void qmp_blockdev_insert_anon_medium(const char *device,
+                                            BlockDriverState *bs, Error **errp)
+{
+    BlockBackend *blk;
+    bool has_device;
+
+    blk = blk_by_name(device);
+    if (!blk) {
+        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
+                  "Device '%s' not found", device);
+        return;
+    }
+
+    /* For BBs without a device, we can exchange the BDS tree at will */
+    has_device = blk_get_attached_dev(blk);
+
+    if (has_device && !blk_dev_has_removable_media(blk)) {
+        error_setg(errp, "Device '%s' is not removable", device);
+        return;
+    }
+
+    if (has_device && !blk_dev_is_tray_open(blk)) {
+        error_setg(errp, "Tray of device '%s' is not open", device);
+        return;
+    }
+
+    if (blk_bs(blk)) {
+        error_setg(errp, "There already is a medium in device '%s'", device);
+        return;
+    }
+
+    blk_insert_bs(blk, bs);
+
+    QTAILQ_INSERT_TAIL(&bdrv_states, bs, device_list);
+}
+
+void qmp_blockdev_insert_medium(const char *device, const char *node_name,
+                                Error **errp)
+{
+    BlockDriverState *bs;
+
+    bs = bdrv_find_node(node_name);
+    if (!bs) {
+        error_setg(errp, "Node '%s' not found", node_name);
+        return;
+    }
+
+    if (bs->blk) {
+        error_setg(errp, "Node '%s' is already in use by '%s'", node_name,
+                   blk_name(bs->blk));
+        return;
+    }
+
+    qmp_blockdev_insert_anon_medium(device, bs, errp);
+}
+
 /* throttling disk I/O limits */
 void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd,
                                int64_t bps_wr,
diff --git a/qapi/block-core.json b/qapi/block-core.json
index e19e82c..5c4fc72 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1940,6 +1940,23 @@
 { 'command': 'blockdev-remove-medium',
   'data': { 'device': 'str' } }
 
+##
+# @blockdev-insert-medium:
+#
+# Inserts a medium (a block driver state tree) into a block device. That block
+# device's tray must currently be open (unless there is no attached guest
+# device) and there must be no medium inserted already.
+#
+# @device:    block device name
+#
+# @node-name: name of a node in the block driver state graph
+#
+# Since: 2.5
+##
+{ 'command': 'blockdev-insert-medium',
+  'data': { 'device': 'str',
+            'node-name': 'str'} }
+
 
 ##
 # @BlockErrorAction
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 6165398..1dfa809 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -4064,6 +4064,43 @@ Example:
 EQMP
 
     {
+        .name       = "blockdev-insert-medium",
+        .args_type  = "device:s,node-name:s",
+        .mhandler.cmd_new = qmp_marshal_blockdev_insert_medium,
+    },
+
+SQMP
+blockdev-insert-medium
+----------------------
+
+Inserts a medium (a block driver state tree) into a block device. That block
+device's tray must currently be open (unless there is no attached guest device)
+and there must be no medium inserted already.
+
+Arguments:
+
+- "device": block device name (json-string)
+- "node-name": root node of the BDS tree to insert into the block device
+
+Example:
+
+-> { "execute": "blockdev-add",
+     "arguments": { "options": { "node-name": "node0",
+                                 "driver": "raw",
+                                 "file": { "driver": "file",
+                                           "filename": "fedora.iso" } } } }
+
+<- { "return": {} }
+
+-> { "execute": "blockdev-insert-medium",
+     "arguments": { "device": "ide1-cd0",
+                    "node-name": "node0" } }
+
+<- { "return": {} }
+
+EQMP
+
+    {
         .name       = "query-named-block-nodes",
         .args_type  = "",
         .mhandler.cmd_new = qmp_marshal_query_named_block_nodes,
-- 
1.8.3.1

[Qemu-devel] [PULL v2 09/40] blockdev: Implement eject with basic operations

[Kevin Wolf]

From: Max Reitz <mreitz@redhat.com>

Implement 'eject' by calling blockdev-open-tray and
blockdev-remove-medium.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 blockdev.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 0f2a7e2..1624297 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1980,16 +1980,15 @@ out:
 
 void qmp_eject(const char *device, bool has_force, bool force, Error **errp)
 {
-    BlockBackend *blk;
+    Error *local_err = NULL;
 
-    blk = blk_by_name(device);
-    if (!blk) {
-        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
-                  "Device '%s' not found", device);
+    qmp_blockdev_open_tray(device, has_force, force, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
         return;
     }
 
-    eject_device(blk, force, errp);
+    qmp_blockdev_remove_medium(device, errp);
 }
 
 void qmp_block_passwd(bool has_device, const char *device,
-- 
1.8.3.1

[Qemu-devel] [PULL v2 10/40] blockdev: Implement change with basic operations

[Kevin Wolf]

From: Max Reitz <mreitz@redhat.com>

Implement 'change' on block devices by calling blockdev-open-tray,
blockdev-remove-medium, blockdev-insert-medium (a variation of that
which does not need a node-name) and blockdev-close-tray.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 blockdev.c | 178 +++++++++++++++++++++++--------------------------------------
 1 file changed, 68 insertions(+), 110 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 1624297..53d4edf 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1940,44 +1940,6 @@ exit:
     }
 }
 
-
-static void eject_device(BlockBackend *blk, int force, Error **errp)
-{
-    BlockDriverState *bs = blk_bs(blk);
-    AioContext *aio_context;
-
-    if (!bs) {
-        /* No medium inserted, so there is nothing to do */
-        return;
-    }
-
-    aio_context = bdrv_get_aio_context(bs);
-    aio_context_acquire(aio_context);
-
-    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_EJECT, errp)) {
-        goto out;
-    }
-    if (!blk_dev_has_removable_media(blk)) {
-        error_setg(errp, "Device '%s' is not removable",
-                   bdrv_get_device_name(bs));
-        goto out;
-    }
-
-    if (blk_dev_is_medium_locked(blk) && !blk_dev_is_tray_open(blk)) {
-        blk_dev_eject_request(blk, force);
-        if (!force) {
-            error_setg(errp, "Device '%s' is locked",
-                       bdrv_get_device_name(bs));
-            goto out;
-        }
-    }
-
-    bdrv_close(bs);
-
-out:
-    aio_context_release(aio_context);
-}
-
 void qmp_eject(const char *device, bool has_force, bool force, Error **errp)
 {
     Error *local_err = NULL;
@@ -2015,78 +1977,6 @@ void qmp_block_passwd(bool has_device, const char *device,
     aio_context_release(aio_context);
 }
 
-/* Assumes AioContext is held */
-static void qmp_bdrv_open_encrypted(BlockDriverState **pbs,
-                                    const char *filename,
-                                    int bdrv_flags, const char *format,
-                                    const char *password, Error **errp)
-{
-    Error *local_err = NULL;
-    QDict *options = NULL;
-    int ret;
-
-    if (format) {
-        options = qdict_new();
-        qdict_put(options, "driver", qstring_from_str(format));
-    }
-
-    ret = bdrv_open(pbs, filename, NULL, options, bdrv_flags, &local_err);
-    if (ret < 0) {
-        error_propagate(errp, local_err);
-        return;
-    }
-
-    bdrv_add_key(*pbs, password, errp);
-}
-
-void qmp_change_blockdev(const char *device, const char *filename,
-                         const char *format, Error **errp)
-{
-    BlockBackend *blk;
-    BlockDriverState *bs;
-    AioContext *aio_context;
-    int bdrv_flags;
-    bool new_bs;
-    Error *err = NULL;
-
-    blk = blk_by_name(device);
-    if (!blk) {
-        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
-                  "Device '%s' not found", device);
-        return;
-    }
-    bs = blk_bs(blk);
-    new_bs = !bs;
-
-    aio_context = blk_get_aio_context(blk);
-    aio_context_acquire(aio_context);
-
-    eject_device(blk, 0, &err);
-    if (err) {
-        error_propagate(errp, err);
-        goto out;
-    }
-
-    bdrv_flags = blk_is_read_only(blk) ? 0 : BDRV_O_RDWR;
-    bdrv_flags |= blk_get_root_state(blk)->open_flags & ~BDRV_O_RDWR;
-
-    qmp_bdrv_open_encrypted(&bs, filename, bdrv_flags, format, NULL, &err);
-    if (err) {
-        error_propagate(errp, err);
-        goto out;
-    }
-
-    if (new_bs) {
-        blk_insert_bs(blk, bs);
-        /* Has been sent automatically by bdrv_open() if blk_bs(blk) was not
-         * NULL */
-        blk_dev_change_media_cb(blk, true);
-    }
-
-out:
-    aio_context_release(aio_context);
-}
-
 void qmp_blockdev_open_tray(const char *device, bool has_force, bool force,
                             Error **errp)
 {
@@ -2253,6 +2143,74 @@ void qmp_blockdev_insert_medium(const char *device, const char *node_name,
     qmp_blockdev_insert_anon_medium(device, bs, errp);
 }
 
+void qmp_change_blockdev(const char *device, const char *filename,
+                         const char *format, Error **errp)
+{
+    BlockBackend *blk;
+    BlockDriverState *medium_bs = NULL;
+    int bdrv_flags, ret;
+    QDict *options = NULL;
+    Error *err = NULL;
+
+    blk = blk_by_name(device);
+    if (!blk) {
+        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
+                  "Device '%s' not found", device);
+        goto fail;
+    }
+
+    if (blk_bs(blk)) {
+        blk_update_root_state(blk);
+    }
+
+    bdrv_flags = blk_get_open_flags_from_root_state(blk);
+
+    if (format) {
+        options = qdict_new();
+        qdict_put(options, "driver", qstring_from_str(format));
+    }
+
+    assert(!medium_bs);
+    ret = bdrv_open(&medium_bs, filename, NULL, options, bdrv_flags, errp);
+    if (ret < 0) {
+        goto fail;
+    }
+
+    blk_apply_root_state(blk, medium_bs);
+
+    bdrv_add_key(medium_bs, NULL, &err);
+    if (err) {
+        error_propagate(errp, err);
+        goto fail;
+    }
+
+    qmp_blockdev_open_tray(device, false, false, &err);
+    if (err) {
+        error_propagate(errp, err);
+        goto fail;
+    }
+
+    qmp_blockdev_remove_medium(device, &err);
+    if (err) {
+        error_propagate(errp, err);
+        goto fail;
+    }
+
+    qmp_blockdev_insert_anon_medium(device, medium_bs, &err);
+    if (err) {
+        error_propagate(errp, err);
+        goto fail;
+    }
+
+    qmp_blockdev_close_tray(device, errp);
+
+fail:
+    /* If the medium has been inserted, the device has its own reference, so
+     * ours must be relinquished; and if it has not been inserted successfully,
+     * the reference must be relinquished anyway */
+    bdrv_unref(medium_bs);
+}
+
 /* throttling disk I/O limits */
 void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd,
                                int64_t bps_wr,
-- 
1.8.3.1

Re: [Qemu-devel] [PULL v2 10/40] blockdev: Implement change with basic operations

[Peter Maydell]

On 10 November 2015 at 14:09, Kevin Wolf <kwolf@redhat.com> wrote:
> From: Max Reitz <mreitz@redhat.com>
>
> Implement 'change' on block devices by calling blockdev-open-tray,
> blockdev-remove-medium, blockdev-insert-medium (a variation of that
> which does not need a node-name) and blockdev-close-tray.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

I think this commit broke the monitor 'change' command for
sd card devices. In 2.4, for a Zaurus ('spitz') machine:
change sd0 /home/petmay01/test-images/RaspberryPi/pifi-4g.img

causes the guest to print
mmc0: new SDHC card at address 4567
mmcblk0: mmc0:4567 QEMU! 4.00 GiB
 mmcblk0: p1 p2 p3 p4

ie it detects we have just provided a new SD card.

In 2.5 trying this gives an error in the monitor:
Tray of device 'sd0' is not open

This seems to be because with this commit we now try to do
a qmp_blockdev_open_tray() on the device. This fails for SD cards
(which don't have any kind of tray in real life), because we
don't implement the is_tray_open hook and so get the default
"tray always closed" behaviour. But in the old code you could
perfectly well change the backing medium even on a device
without a tray...

Was this breakage intentional?

thanks
-- PMM

Re: [Qemu-devel] [PULL v2 10/40] blockdev: Implement change with basic operations

[Max Reitz]

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

On 07.01.2016 19:06, Peter Maydell wrote:
> On 10 November 2015 at 14:09, Kevin Wolf <kwolf@redhat.com> wrote:
>> From: Max Reitz <mreitz@redhat.com>
>>
>> Implement 'change' on block devices by calling blockdev-open-tray,
>> blockdev-remove-medium, blockdev-insert-medium (a variation of that
>> which does not need a node-name) and blockdev-close-tray.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> 
> I think this commit broke the monitor 'change' command for
> sd card devices. In 2.4, for a Zaurus ('spitz') machine:
> change sd0 /home/petmay01/test-images/RaspberryPi/pifi-4g.img
> 
> causes the guest to print
> mmc0: new SDHC card at address 4567
> mmcblk0: mmc0:4567 QEMU! 4.00 GiB
>  mmcblk0: p1 p2 p3 p4
> 
> ie it detects we have just provided a new SD card.
> 
> In 2.5 trying this gives an error in the monitor:
> Tray of device 'sd0' is not open
> 
> This seems to be because with this commit we now try to do
> a qmp_blockdev_open_tray() on the device. This fails for SD cards
> (which don't have any kind of tray in real life), because we
> don't implement the is_tray_open hook and so get the default
> "tray always closed" behaviour. But in the old code you could
> perfectly well change the backing medium even on a device
> without a tray...
> 
> Was this breakage intentional?

No, it definitely was not. I'm sorry.

Compare floppy disks, for which we now have a "virtual" tray status:
Whenever a medium is inserted, the "tray" is considered closed.
Otherwise, it is open. This works pretty much like a physical tray would
work; whenever the tray is closed, you cannot exchange the medium, but
when it is open, you can.

There is only one difference to devices which actually have a tray: For
floppy disks, you cannot have a closed tray without a medium in it.

Anyway, we can implement the same model for SD cards. I'll see to it.

Max


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

Re: [Qemu-devel] [PULL v2 10/40] blockdev: Implement change with basic operations

[Peter Maydell]

On 7 January 2016 at 19:37, Max Reitz <mreitz@redhat.com> wrote:
> Compare floppy disks, for which we now have a "virtual" tray status:
> Whenever a medium is inserted, the "tray" is considered closed.
> Otherwise, it is open. This works pretty much like a physical tray would
> work; whenever the tray is closed, you cannot exchange the medium, but
> when it is open, you can.
>
> There is only one difference to devices which actually have a tray: For
> floppy disks, you cannot have a closed tray without a medium in it.
>
> Anyway, we can implement the same model for SD cards. I'll see to it.

It looks like sd.c is the only one which implements a change_media_cb
but no is_tray_open, but it would be nice if we could implement this
in the default blk_dev_is_tray_open() method rather than in the
sd and floppy models (ie if I don't implement tray-open then the
tray is closed if there's a medium attached, and the block backend
ought to know if there's a medium attached itself already).

thanks
-- PMM

Re: [Qemu-devel] [PULL v2 10/40] blockdev: Implement change with basic operations

[Max Reitz]

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

On 07.01.2016 20:56, Peter Maydell wrote:
> On 7 January 2016 at 19:37, Max Reitz <mreitz@redhat.com> wrote:
>> Compare floppy disks, for which we now have a "virtual" tray status:
>> Whenever a medium is inserted, the "tray" is considered closed.
>> Otherwise, it is open. This works pretty much like a physical tray would
>> work; whenever the tray is closed, you cannot exchange the medium, but
>> when it is open, you can.
>>
>> There is only one difference to devices which actually have a tray: For
>> floppy disks, you cannot have a closed tray without a medium in it.
>>
>> Anyway, we can implement the same model for SD cards. I'll see to it.
> 
> It looks like sd.c is the only one which implements a change_media_cb
> but no is_tray_open, but it would be nice if we could implement this
> in the default blk_dev_is_tray_open() method rather than in the
> sd and floppy models (ie if I don't implement tray-open then the
> tray is closed if there's a medium attached, and the block backend
> ought to know if there's a medium attached itself already).

That would be nice, but there's a difference between "there's a medium
attached" (tray can be both open and closed) and "the medium is
accessible by the guest" (tray must be closed). The BlockBackend does
not know this difference, only the guest devices does.

It gets told of when to open/close the tray by invocation of the
change_media_cb() (the @load parameter set to false or true,
respectively), and we could track this state in the BlockBackend instead
of in the SDState. But that looks like the wrong place to me.

Right now, sd.c completely ignores the @load parameter of
change_media_cb(), which seems wrong; this means that "opening the tray"
keeps the medium accessible for the guest. In the past that was fine,
because eject closed the associated BDS (the medium) right afterwards,
so it actually wasn't accessible any more. The new
blockdev-open-tray no longer does that, so now we need to respect the
value of @load and no longer rely on blk_is_inserted() alone any more.

Therefore, I believe we need to track the medium insertion state in
SDState, because this fixes the issue of having ignored @load (which
kept the SD card accessible to the guest even after blockdev-open-tray).

We can then use this tracked state to fix this issue here, by
implementing a trivial is_tray_open() which simply returns false iff
there is a medium inserted into the slot.

Max


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

Re: [Qemu-devel] [PULL v2 10/40] blockdev: Implement change with basic operations

[Peter Maydell]

On 7 January 2016 at 20:14, Max Reitz <mreitz@redhat.com> wrote:
> On 07.01.2016 20:56, Peter Maydell wrote:
>> It looks like sd.c is the only one which implements a change_media_cb
>> but no is_tray_open, but it would be nice if we could implement this
>> in the default blk_dev_is_tray_open() method rather than in the
>> sd and floppy models (ie if I don't implement tray-open then the
>> tray is closed if there's a medium attached, and the block backend
>> ought to know if there's a medium attached itself already).
>
> That would be nice, but there's a difference between "there's a medium
> attached" (tray can be both open and closed) and "the medium is
> accessible by the guest" (tray must be closed). The BlockBackend does
> not know this difference, only the guest devices does.
>
> It gets told of when to open/close the tray by invocation of the
> change_media_cb() (the @load parameter set to false or true,
> respectively), and we could track this state in the BlockBackend instead
> of in the SDState. But that looks like the wrong place to me.

Well, previously sd.c didn't need to have any state for this
to all work right (or indeed care about implementing a fake
tray status for a device that doesn't have a tray), so it seems
odd that we need to invent some extra state for it to work now.

> Right now, sd.c completely ignores the @load parameter of
> change_media_cb(), which seems wrong; this means that "opening the tray"
> keeps the medium accessible for the guest. In the past that was fine,
> because eject closed the associated BDS (the medium) right afterwards,
> so it actually wasn't accessible any more. The new
> blockdev-open-tray no longer does that, so now we need to respect the
> value of @load and no longer rely on blk_is_inserted() alone any more.

I think blockdev-open-tray should probably not work for SD cards
(or for floppies?), because saying "open the tray" on a device
without a tray is meaningless.

thanks
-- PMM

Re: [Qemu-devel] [PULL v2 10/40] blockdev: Implement change with basic operations

[Max Reitz]

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

On 07.01.2016 22:42, Peter Maydell wrote:
> On 7 January 2016 at 20:14, Max Reitz <mreitz@redhat.com> wrote:
>> On 07.01.2016 20:56, Peter Maydell wrote:
>>> It looks like sd.c is the only one which implements a change_media_cb
>>> but no is_tray_open, but it would be nice if we could implement this
>>> in the default blk_dev_is_tray_open() method rather than in the
>>> sd and floppy models (ie if I don't implement tray-open then the
>>> tray is closed if there's a medium attached, and the block backend
>>> ought to know if there's a medium attached itself already).
>>
>> That would be nice, but there's a difference between "there's a medium
>> attached" (tray can be both open and closed) and "the medium is
>> accessible by the guest" (tray must be closed). The BlockBackend does
>> not know this difference, only the guest devices does.
>>
>> It gets told of when to open/close the tray by invocation of the
>> change_media_cb() (the @load parameter set to false or true,
>> respectively), and we could track this state in the BlockBackend instead
>> of in the SDState. But that looks like the wrong place to me.
> 
> Well, previously sd.c didn't need to have any state for this
> to all work right (or indeed care about implementing a fake
> tray status for a device that doesn't have a tray), so it seems
> odd that we need to invent some extra state for it to work now.

Before, it took the state from the block layer by calling
blk_is_inserted(). Works fine as long we only have the high-level
operations change and eject. Stops to work once we introduce lower-level
operations (open-tray, remove-medium, insert-medium, close-tray).

Why do we need the low-level operations? Mainly because they integrate
much better into the model of a more freely configurable block layer
(there aren't many options one can give to the 'change' operation; now
you can use blockdev-add and the lower-level operations afterwards).

Why did I reimplement 'change' and 'eject' using these new operations?
Because otherwise we'd have those operations doing one kind of thing,
and the lower-level ones doing something completely different. I'd find
that bad.

>> Right now, sd.c completely ignores the @load parameter of
>> change_media_cb(), which seems wrong; this means that "opening the tray"
>> keeps the medium accessible for the guest. In the past that was fine,
>> because eject closed the associated BDS (the medium) right afterwards,
>> so it actually wasn't accessible any more. The new
>> blockdev-open-tray no longer does that, so now we need to respect the
>> value of @load and no longer rely on blk_is_inserted() alone any more.
> 
> I think blockdev-open-tray should probably not work for SD cards
> (or for floppies?), because saying "open the tray" on a device
> without a tray is meaningless.

Depends on what you think it is. For me, blockdev-open-tray generally
means "pushing the eject button".

For actual tray devices, this means that the tray may open (or the OS
may be asked to do so). For floppy disk drives, this means the floppy
disk will be ejected. For SD slots (where there often is no dedicated
button, but one can push the SD card further in for it to get released),
this means the SD card will be ejected.

Note that this is different from the 'eject' command which models a
drive where the user pushes the eject button and suddenly the medium
bursts into flames/is dropped on the floor/just disappears. Therefore, I
find the 'eject' command quite misnamed, but having named the new
command 'blockdev-eject-medium' wouldn't have made things any better.

Max


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

Re: [Qemu-devel] [PULL v2 10/40] blockdev: Implement change with basic operations

[Peter Maydell]

On 7 January 2016 at 21:57, Max Reitz <mreitz@redhat.com> wrote:
> On 07.01.2016 22:42, Peter Maydell wrote:
>> Well, previously sd.c didn't need to have any state for this
>> to all work right (or indeed care about implementing a fake
>> tray status for a device that doesn't have a tray), so it seems
>> odd that we need to invent some extra state for it to work now.
>
> Before, it took the state from the block layer by calling
> blk_is_inserted(). Works fine as long we only have the high-level
> operations change and eject. Stops to work once we introduce lower-level
> operations (open-tray, remove-medium, insert-medium, close-tray).
>
> Why do we need the low-level operations? Mainly because they integrate
> much better into the model of a more freely configurable block layer
> (there aren't many options one can give to the 'change' operation; now
> you can use blockdev-add and the lower-level operations afterwards).
>
> Why did I reimplement 'change' and 'eject' using these new operations?
> Because otherwise we'd have those operations doing one kind of thing,
> and the lower-level ones doing something completely different. I'd find
> that bad.

But these lower level operations now let you put the system
conceptually into states that it really can't be in in hardware.
That seems wrong to me and we should fail those commands where
it makes sense (eg trying to "open a tray" when there is no tray),
rather than forcing the sd.c layer code to cope with the user
putting the system into impossible conditions.

>>> Right now, sd.c completely ignores the @load parameter of
>>> change_media_cb(), which seems wrong; this means that "opening the tray"
>>> keeps the medium accessible for the guest. In the past that was fine,
>>> because eject closed the associated BDS (the medium) right afterwards,
>>> so it actually wasn't accessible any more. The new
>>> blockdev-open-tray no longer does that, so now we need to respect the
>>> value of @load and no longer rely on blk_is_inserted() alone any more.
>>
>> I think blockdev-open-tray should probably not work for SD cards
>> (or for floppies?), because saying "open the tray" on a device
>> without a tray is meaningless.
>
> Depends on what you think it is. For me, blockdev-open-tray generally
> means "pushing the eject button".
>
> For actual tray devices, this means that the tray may open (or the OS
> may be asked to do so). For floppy disk drives, this means the floppy
> disk will be ejected. For SD slots (where there often is no dedicated
> button, but one can push the SD card further in for it to get released),
> this means the SD card will be ejected.
>
> Note that this is different from the 'eject' command which models a
> drive where the user pushes the eject button and suddenly the medium
> bursts into flames/is dropped on the floor/just disappears. Therefore, I
> find the 'eject' command quite misnamed, but having named the new
> command 'blockdev-eject-medium' wouldn't have made things any better.

I don't understand what the difference is between your
"floppy disk will be ejected" and "SD card will be ejected"
cases, and "medium is dropped on the floor" case. Those seem
like exactly the same thing to me, so we should just use "eject"
and not have an extra command.

I can see why we want to model tray state for devices with trays,
but I don't see why this is bleeding into devices without trays.

thanks
-- PMM

Re: [Qemu-devel] [PULL v2 10/40] blockdev: Implement change with basic operations

[Max Reitz]

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

On 07.01.2016 23:19, Peter Maydell wrote:
> On 7 January 2016 at 21:57, Max Reitz <mreitz@redhat.com> wrote:
>> On 07.01.2016 22:42, Peter Maydell wrote:
>>> Well, previously sd.c didn't need to have any state for this
>>> to all work right (or indeed care about implementing a fake
>>> tray status for a device that doesn't have a tray), so it seems
>>> odd that we need to invent some extra state for it to work now.
>>
>> Before, it took the state from the block layer by calling
>> blk_is_inserted(). Works fine as long we only have the high-level
>> operations change and eject. Stops to work once we introduce lower-level
>> operations (open-tray, remove-medium, insert-medium, close-tray).
>>
>> Why do we need the low-level operations? Mainly because they integrate
>> much better into the model of a more freely configurable block layer
>> (there aren't many options one can give to the 'change' operation; now
>> you can use blockdev-add and the lower-level operations afterwards).
>>
>> Why did I reimplement 'change' and 'eject' using these new operations?
>> Because otherwise we'd have those operations doing one kind of thing,
>> and the lower-level ones doing something completely different. I'd find
>> that bad.
> 
> But these lower level operations now let you put the system
> conceptually into states that it really can't be in in hardware.
> That seems wrong to me and we should fail those commands where
> it makes sense (eg trying to "open a tray" when there is no tray),
> rather than forcing the sd.c layer code to cope with the user
> putting the system into impossible conditions.

Hm, OK. So I assume you are proposing just not doing any *-tray
operations on devices without a tray? I personally still consider
"pushing the eject button" and "actually taking the medium out of the
slot" to distinct operations, but I think I could live with that.

Technically, the block layer should handle that just fine.

>>>> Right now, sd.c completely ignores the @load parameter of
>>>> change_media_cb(), which seems wrong; this means that "opening the tray"
>>>> keeps the medium accessible for the guest. In the past that was fine,
>>>> because eject closed the associated BDS (the medium) right afterwards,
>>>> so it actually wasn't accessible any more. The new
>>>> blockdev-open-tray no longer does that, so now we need to respect the
>>>> value of @load and no longer rely on blk_is_inserted() alone any more.
>>>
>>> I think blockdev-open-tray should probably not work for SD cards
>>> (or for floppies?), because saying "open the tray" on a device
>>> without a tray is meaningless.
>>
>> Depends on what you think it is. For me, blockdev-open-tray generally
>> means "pushing the eject button".
>>
>> For actual tray devices, this means that the tray may open (or the OS
>> may be asked to do so). For floppy disk drives, this means the floppy
>> disk will be ejected. For SD slots (where there often is no dedicated
>> button, but one can push the SD card further in for it to get released),
>> this means the SD card will be ejected.
>>
>> Note that this is different from the 'eject' command which models a
>> drive where the user pushes the eject button and suddenly the medium
>> bursts into flames/is dropped on the floor/just disappears. Therefore, I
>> find the 'eject' command quite misnamed, but having named the new
>> command 'blockdev-eject-medium' wouldn't have made things any better.
> 
> I don't understand what the difference is between your
> "floppy disk will be ejected" and "SD card will be ejected"
> cases, and "medium is dropped on the floor" case. Those seem
> like exactly the same thing to me, so we should just use "eject"
> and not have an extra command.

From the guest's perspective, you are right, there is no difference. I
do see a difference from the user's perspective, but I think you are
right in that that difference may not actually matter to anyone.

The difference is the following (let's use a floppy disk drive, because
there are all kinds of SD slots in common use): On a physical device,
you push the eject button and the disk comes out. You can then just push
the disk back in, or you can remove it, and maybe replace it.

Using 'eject', you cannot push the disk back in. It's gone. You'll have
to find a new one to insert. Yes, this is a use case that rarely anybody
ever needs, but it still is a difference from what physical hardware does.

Using 'blockdev-open-tray' means pushing the eject button. You can use
'blockdev-close-tray' to push the medium back in. Or you invoke
'(x-)blockdev-remove-medium' to remove it and
'(x-)blockdev-insert-medium' to insert a new one (which you'd then push
in using 'blockdev-close-tray').

Technically, what happens is this: The *-tray commands are for the
device model. blockdev-close-tray invokes the change_media_cb with
load=false; blockdev-open-tray invokes it with load=true. The general
block layer doesn't do anything here.

The *-medium commands are for the block layer. blockdev-remove-medium
removes BlockDriverState from the BlockBackend attached to the device,
and blockdev-insert-medium attaches a BDS to the BB. The device model
does not get notified of this, because it should not care at this point
(the tray, be it actually existing or just some boolean in its state, is
open, so the virtual device cannot see the medium anyway).

> I can see why we want to model tray state for devices with trays,
> but I don't see why this is bleeding into devices without trays.

I hope that the above explanation helped you understand why it bled into
tray-less devices, from a technical perspective.

Anyway, from a usability standpoint, you are right in that probably
nobody ever needs the *-tray commands for tray-less devices.

I don't think however that having these commands will hurt anyone, so I
think (please correct me if I'm wrong (not just here, of course :-))
that this is mainly a technical discussion of how ugly having these
commands for tray-less devices is, and of how difficult it is to
implement it reasonably.

Without migration, I would have said that I do not find a simple boolean
in the device state struct to be too bad. However, you are correct in
that this field needs to be migrated, so now it becomes a different story.

The alternative, which I'll be looking into next week and which I guess
is what you'd find more appealing, is to drop the *-tray commands for
tray-less devices by design. The problem here is that then the
blockdev-*-medium devices actually need to notify tray-less devices
(invoke their change_medium_cb), which they do not for devices that do
have a tray. While that's probably not difficult to implement, it is a
bit ugly. I'll look into it.

Thank you for this discussion!

Max


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

Re: [Qemu-devel] [PULL v2 10/40] blockdev: Implement change with basic operations

[Peter Maydell]

On 7 January 2016 at 22:43, Max Reitz <mreitz@redhat.com> wrote:
> I hope that the above explanation helped you understand why it bled into
> tray-less devices, from a technical perspective.

Yes, thanks, that was definitely a helpful explanation for why
the design is the way it is. I'm still not sure how useful it
is to model "floppy is in the drive slot but not pushed in"
(we don't model "floppy is sat on my kitchen table", which
is also a situation that a real floppy disk could be in ;-)).
But I can see the attraction of trying to separate commands
operating on the device from commands operating on the backend.

thanks
-- PMM

Re: [Qemu-devel] [PULL v2 10/40] blockdev: Implement change with basic operations

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 7 January 2016 at 22:43, Max Reitz <mreitz@redhat.com> wrote:
>> I hope that the above explanation helped you understand why it bled into
>> tray-less devices, from a technical perspective.
>
> Yes, thanks, that was definitely a helpful explanation for why
> the design is the way it is. I'm still not sure how useful it
> is to model "floppy is in the drive slot but not pushed in"
> (we don't model "floppy is sat on my kitchen table", which
> is also a situation that a real floppy disk could be in ;-)).
> But I can see the attraction of trying to separate commands
> operating on the device from commands operating on the backend.

Hysterical raisins, as usual.

The modelling of devices with removable media evolved to do the right
thing for CD-ROMs with a tray-type loading mechanism.  There are others,
notably slot loading.  Anyway, this model has been "close enough" for
all devices, or at least nobody got bothered enough to complicate the
model with additional cases.

[Qemu-devel] [PULL v2 11/40] block: Inquire tray state before tray-moved events

[Kevin Wolf]

From: Max Reitz <mreitz@redhat.com>

blk_dev_change_media_cb() is called for all potential tray movements;
however, it is possible to request closing the tray but nothing actually
happening (on a floppy disk drive without a medium).

Thus, the actual tray status should be inquired before sending a
tray-moved event (and an event should be sent whenever the status
changed).

Checking @load is now superfluous; it was necessary because it was
possible to change a medium without having explicitly opened the tray
and closed it again (or it might have been possible, at least). This is
no longer possible, though.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/block-backend.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 7d49539..1ac6982 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -429,18 +429,15 @@ void blk_set_dev_ops(BlockBackend *blk, const BlockDevOps *ops,
 void blk_dev_change_media_cb(BlockBackend *blk, bool load)
 {
     if (blk->dev_ops && blk->dev_ops->change_media_cb) {
-        bool tray_was_closed = !blk_dev_is_tray_open(blk);
+        bool tray_was_open, tray_is_open;
 
+        tray_was_open = blk_dev_is_tray_open(blk);
         blk->dev_ops->change_media_cb(blk->dev_opaque, load);
-        if (tray_was_closed) {
-            /* tray open */
-            qapi_event_send_device_tray_moved(blk_name(blk),
-                                              true, &error_abort);
-        }
-        if (load) {
-            /* tray close */
-            qapi_event_send_device_tray_moved(blk_name(blk),
-                                              false, &error_abort);
+        tray_is_open = blk_dev_is_tray_open(blk);
+
+        if (tray_was_open != tray_is_open) {
+            qapi_event_send_device_tray_moved(blk_name(blk), tray_is_open,
+                                              &error_abort);
         }
     }
 }
-- 
1.8.3.1

[Qemu-devel] [PULL v2 12/40] qmp: Introduce blockdev-change-medium

[Kevin Wolf]

From: Max Reitz <mreitz@redhat.com>

Introduce a new QMP command 'blockdev-change-medium' which is intended
to replace the 'change' command for block devices. The existing function
qmp_change_blockdev() is accordingly renamed to
qmp_blockdev_change_medium().

Signed-off-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 blockdev.c                |  7 ++++---
 include/sysemu/blockdev.h |  2 --
 qapi-schema.json          |  6 ++++--
 qapi/block-core.json      | 23 +++++++++++++++++++++++
 qmp-commands.hx           | 31 +++++++++++++++++++++++++++++++
 qmp.c                     |  2 +-
 ui/cocoa.m                | 10 ++++++----
 7 files changed, 69 insertions(+), 12 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 53d4edf..b3a958c 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2143,8 +2143,9 @@ void qmp_blockdev_insert_medium(const char *device, const char *node_name,
     qmp_blockdev_insert_anon_medium(device, bs, errp);
 }
 
-void qmp_change_blockdev(const char *device, const char *filename,
-                         const char *format, Error **errp)
+void qmp_blockdev_change_medium(const char *device, const char *filename,
+                                bool has_format, const char *format,
+                                Error **errp)
 {
     BlockBackend *blk;
     BlockDriverState *medium_bs = NULL;
@@ -2165,7 +2166,7 @@ void qmp_change_blockdev(const char *device, const char *filename,
 
     bdrv_flags = blk_get_open_flags_from_root_state(blk);
 
-    if (format) {
+    if (has_format) {
         options = qdict_new();
         qdict_put(options, "driver", qstring_from_str(format));
     }
diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
index a00be94..b06a060 100644
--- a/include/sysemu/blockdev.h
+++ b/include/sysemu/blockdev.h
@@ -63,8 +63,6 @@ DriveInfo *drive_new(QemuOpts *arg, BlockInterfaceType block_default_type);
 
 /* device-hotplug */
 
-void qmp_change_blockdev(const char *device, const char *filename,
-                         const char *format, Error **errp);
 void hmp_commit(Monitor *mon, const QDict *qdict);
 void hmp_drive_del(Monitor *mon, const QDict *qdict);
 #endif
diff --git a/qapi-schema.json b/qapi-schema.json
index e18f14c..ed7fc99 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1842,8 +1842,10 @@
 #          device's password.  The behavior of reads and writes to the block
 #          device between when these calls are executed is undefined.
 #
-# Notes:  It is strongly recommended that this interface is not used especially
-#         for changing block devices.
+# Notes:  This interface is deprecated, and it is strongly recommended that you
+#         avoid using it.  For changing block devices, use
+#         blockdev-change-medium; for changing VNC parameters, use
+#         change-vnc-password.
 #
 # Since: 0.14.0
 ##
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 5c4fc72..e9fa649 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1959,6 +1959,29 @@
 
 
 ##
+# @blockdev-change-medium:
+#
+# Changes the medium inserted into a block device by ejecting the current medium
+# and loading a new image file which is inserted as the new medium (this command
+# combines blockdev-open-tray, blockdev-remove-medium, blockdev-insert-medium
+# and blockdev-close-tray).
+#
+# @device:          block device name
+#
+# @filename:        filename of the new image to be loaded
+#
+# @format:          #optional, format to open the new image with (defaults to
+#                   the probed format)
+#
+# Since: 2.5
+##
+{ 'command': 'blockdev-change-medium',
+  'data': { 'device': 'str',
+            'filename': 'str',
+            '*format': 'str' } }
+
+
+##
 # @BlockErrorAction
 #
 # An enumeration of action that has been taken when a DISK I/O occurs
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 1dfa809..d6e4d7e 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -4163,6 +4163,37 @@ Example:
 EQMP
 
     {
+        .name       = "blockdev-change-medium",
+        .args_type  = "device:B,filename:F,format:s?",
+        .mhandler.cmd_new = qmp_marshal_blockdev_change_medium,
+    },
+
+SQMP
+blockdev-change-medium
+----------------------
+
+Changes the medium inserted into a block device by ejecting the current medium
+and loading a new image file which is inserted as the new medium.
+
+Arguments:
+
+- "device": device name (json-string)
+- "filename": filename of the new image (json-string)
+- "format": format of the new image (json-string, optional)
+
+Examples:
+
+1. Change a removable medium
+
+-> { "execute": "blockdev-change-medium",
+             "arguments": { "device": "ide1-cd0",
+                            "filename": "/srv/images/Fedora-12-x86_64-DVD.iso",
+                            "format": "raw" } }
+<- { "return": {} }
+
+EQMP
+
+    {
         .name       = "query-memdev",
         .args_type  = "",
         .mhandler.cmd_new = qmp_marshal_query_memdev,
diff --git a/qmp.c b/qmp.c
index ff54e5a..4e44f98 100644
--- a/qmp.c
+++ b/qmp.c
@@ -414,7 +414,7 @@ void qmp_change(const char *device, const char *target,
     if (strcmp(device, "vnc") == 0) {
         qmp_change_vnc(target, has_arg, arg, errp);
     } else {
-        qmp_change_blockdev(device, target, arg, errp);
+        qmp_blockdev_change_medium(device, target, has_arg, arg, errp);
     }
 }
 
diff --git a/ui/cocoa.m b/ui/cocoa.m
index c0d6bb2..2d8e4e2 100644
--- a/ui/cocoa.m
+++ b/ui/cocoa.m
@@ -1113,10 +1113,12 @@ QemuCocoaView *cocoaView;
         }
 
         Error *err = NULL;
-        qmp_change_blockdev([drive cStringUsingEncoding: NSASCIIStringEncoding],
-                            [file cStringUsingEncoding: NSASCIIStringEncoding],
-                            "raw",
-                            &err);
+        qmp_blockdev_change_medium([drive cStringUsingEncoding:
+                                          NSASCIIStringEncoding],
+                                   [file cStringUsingEncoding:
+                                         NSASCIIStringEncoding],
+                                   true, "raw",
+                                   &err);
         handleAnyDeviceErrors(err);
     }
 }
-- 
1.8.3.1

[Qemu-devel] [PULL v2 13/40] hmp: Use blockdev-change-medium for change command

[Kevin Wolf]

From: Max Reitz <mreitz@redhat.com>

Use separate code paths for the two overloaded functions of the 'change'
HMP command, and invoke the 'blockdev-change-medium' QMP command if used
on a block device (by calling qmp_blockdev_change_medium()).

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hmp.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/hmp.c b/hmp.c
index a15d00c..5e5358f 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1338,22 +1338,25 @@ void hmp_change(Monitor *mon, const QDict *qdict)
     const char *arg = qdict_get_try_str(qdict, "arg");
     Error *err = NULL;
 
-    if (strcmp(device, "vnc") == 0 &&
-            (strcmp(target, "passwd") == 0 ||
-             strcmp(target, "password") == 0)) {
-        if (!arg) {
-            monitor_read_password(mon, hmp_change_read_arg, NULL);
+    if (strcmp(device, "vnc") == 0) {
+        if (strcmp(target, "passwd") == 0 ||
+            strcmp(target, "password") == 0) {
+            if (!arg) {
+                monitor_read_password(mon, hmp_change_read_arg, NULL);
+                return;
+            }
+        }
+        qmp_change("vnc", target, !!arg, arg, &err);
+    } else {
+        qmp_blockdev_change_medium(device, target, !!arg, arg, &err);
+        if (err &&
+            error_get_class(err) == ERROR_CLASS_DEVICE_ENCRYPTED) {
+            error_free(err);
+            monitor_read_block_device_key(mon, device, NULL, NULL);
             return;
         }
     }
 
-    qmp_change(device, target, !!arg, arg, &err);
-    if (err &&
-        error_get_class(err) == ERROR_CLASS_DEVICE_ENCRYPTED) {
-        error_free(err);
-        monitor_read_block_device_key(mon, device, NULL, NULL);
-        return;
-    }
     hmp_handle_error(mon, &err);
 }
 
-- 
1.8.3.1

[Qemu-devel] [PULL v2 14/40] blockdev: read-only-mode for blockdev-change-medium

[Kevin Wolf]

From: Max Reitz <mreitz@redhat.com>

Add an option to qmp_blockdev_change_medium() which allows changing the
read-only status of the block device whose medium is changed.

Some drives do not have a inherently fixed read-only status; for
instance, floppy disks can be set read-only or writable independently of
the drive. Some users may find it useful to be able to therefore change
the read-only status of a block device when changing the medium.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 blockdev.c           | 22 ++++++++++++++++++++++
 hmp.c                |  2 +-
 qapi/block-core.json | 24 +++++++++++++++++++++++-
 qmp-commands.hx      | 24 +++++++++++++++++++++++-
 qmp.c                |  3 ++-
 5 files changed, 71 insertions(+), 4 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index b3a958c..34f6e5b 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2145,6 +2145,8 @@ void qmp_blockdev_insert_medium(const char *device, const char *node_name,
 
 void qmp_blockdev_change_medium(const char *device, const char *filename,
                                 bool has_format, const char *format,
+                                bool has_read_only,
+                                BlockdevChangeReadOnlyMode read_only,
                                 Error **errp)
 {
     BlockBackend *blk;
@@ -2166,6 +2168,26 @@ void qmp_blockdev_change_medium(const char *device, const char *filename,
 
     bdrv_flags = blk_get_open_flags_from_root_state(blk);
 
+    if (!has_read_only) {
+        read_only = BLOCKDEV_CHANGE_READ_ONLY_MODE_RETAIN;
+    }
+
+    switch (read_only) {
+    case BLOCKDEV_CHANGE_READ_ONLY_MODE_RETAIN:
+        break;
+
+    case BLOCKDEV_CHANGE_READ_ONLY_MODE_READ_ONLY:
+        bdrv_flags &= ~BDRV_O_RDWR;
+        break;
+
+    case BLOCKDEV_CHANGE_READ_ONLY_MODE_READ_WRITE:
+        bdrv_flags |= BDRV_O_RDWR;
+        break;
+
+    default:
+        abort();
+    }
+
     if (has_format) {
         options = qdict_new();
         qdict_put(options, "driver", qstring_from_str(format));
diff --git a/hmp.c b/hmp.c
index 5e5358f..efa07a2 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1348,7 +1348,7 @@ void hmp_change(Monitor *mon, const QDict *qdict)
         }
         qmp_change("vnc", target, !!arg, arg, &err);
     } else {
-        qmp_blockdev_change_medium(device, target, !!arg, arg, &err);
+        qmp_blockdev_change_medium(device, target, !!arg, arg, false, 0, &err);
         if (err &&
             error_get_class(err) == ERROR_CLASS_DEVICE_ENCRYPTED) {
             error_free(err);
diff --git a/qapi/block-core.json b/qapi/block-core.json
index e9fa649..fa08ba9 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1959,6 +1959,24 @@
 
 
 ##
+# @BlockdevChangeReadOnlyMode:
+#
+# Specifies the new read-only mode of a block device subject to the
+# @blockdev-change-medium command.
+#
+# @retain:      Retains the current read-only mode
+#
+# @read-only:   Makes the device read-only
+#
+# @read-write:  Makes the device writable
+#
+# Since: 2.3
+##
+{ 'enum': 'BlockdevChangeReadOnlyMode',
+  'data': ['retain', 'read-only', 'read-write'] }
+
+
+##
 # @blockdev-change-medium:
 #
 # Changes the medium inserted into a block device by ejecting the current medium
@@ -1973,12 +1991,16 @@
 # @format:          #optional, format to open the new image with (defaults to
 #                   the probed format)
 #
+# @read-only-mode:  #optional, change the read-only mode of the device; defaults
+#                   to 'retain'
+#
 # Since: 2.5
 ##
 { 'command': 'blockdev-change-medium',
   'data': { 'device': 'str',
             'filename': 'str',
-            '*format': 'str' } }
+            '*format': 'str',
+            '*read-only-mode': 'BlockdevChangeReadOnlyMode' } }
 
 
 ##
diff --git a/qmp-commands.hx b/qmp-commands.hx
index d6e4d7e..707e0bd 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -4164,7 +4164,7 @@ EQMP
 
     {
         .name       = "blockdev-change-medium",
-        .args_type  = "device:B,filename:F,format:s?",
+        .args_type  = "device:B,filename:F,format:s?,read-only-mode:s?",
         .mhandler.cmd_new = qmp_marshal_blockdev_change_medium,
     },
 
@@ -4180,6 +4180,8 @@ Arguments:
 - "device": device name (json-string)
 - "filename": filename of the new image (json-string)
 - "format": format of the new image (json-string, optional)
+- "read-only-mode": new read-only mode (json-string, optional)
+          - Possible values: "retain" (default), "read-only", "read-write"
 
 Examples:
 
@@ -4191,6 +4193,26 @@ Examples:
                             "format": "raw" } }
 <- { "return": {} }
 
+2. Load a read-only medium into a writable drive
+
+-> { "execute": "blockdev-change-medium",
+             "arguments": { "device": "isa-fd0",
+                            "filename": "/srv/images/ro.img",
+                            "format": "raw",
+                            "read-only-mode": "retain" } }
+
+<- { "error":
+     { "class": "GenericError",
+       "desc": "Could not open '/srv/images/ro.img': Permission denied" } }
+
+-> { "execute": "blockdev-change-medium",
+             "arguments": { "device": "isa-fd0",
+                            "filename": "/srv/images/ro.img",
+                            "format": "raw",
+                            "read-only-mode": "read-only" } }
+
+<- { "return": {} }
+
 EQMP
 
     {
diff --git a/qmp.c b/qmp.c
index 4e44f98..ddc63ea 100644
--- a/qmp.c
+++ b/qmp.c
@@ -414,7 +414,8 @@ void qmp_change(const char *device, const char *target,
     if (strcmp(device, "vnc") == 0) {
         qmp_change_vnc(target, has_arg, arg, errp);
     } else {
-        qmp_blockdev_change_medium(device, target, has_arg, arg, errp);
+        qmp_blockdev_change_medium(device, target, has_arg, arg, false, 0,
+                                   errp);
     }
 }
 
-- 
1.8.3.1

[Qemu-devel] [PULL v2 15/40] hmp: Add read-only-mode option to change command

[Kevin Wolf]

From: Max Reitz <mreitz@redhat.com>

Expose the new read-only-mode option of 'blockdev-change-medium' for the
'change' HMP command.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hmp-commands.hx | 20 +++++++++++++++++---
 hmp.c           | 22 +++++++++++++++++++++-
 2 files changed, 38 insertions(+), 4 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 3a4ae39..814ea86 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -194,8 +194,8 @@ ETEXI
 
     {
         .name       = "change",
-        .args_type  = "device:B,target:F,arg:s?",
-        .params     = "device filename [format]",
+        .args_type  = "device:B,target:F,arg:s?,read-only-mode:s?",
+        .params     = "device filename [format [read-only-mode]]",
         .help       = "change a removable medium, optional format",
         .mhandler.cmd = hmp_change,
     },
@@ -206,7 +206,7 @@ STEXI
 Change the configuration of a device.
 
 @table @option
-@item change @var{diskdevice} @var{filename} [@var{format}]
+@item change @var{diskdevice} @var{filename} [@var{format} [@var{read-only-mode}]]
 Change the medium for a removable disk device to point to @var{filename}. eg
 
 @example
@@ -215,6 +215,20 @@ Change the medium for a removable disk device to point to @var{filename}. eg
 
 @var{format} is optional.
 
+@var{read-only-mode} may be used to change the read-only status of the device.
+It accepts the following values:
+
+@table @var
+@item retain
+Retains the current status; this is the default.
+
+@item read-only
+Makes the device read-only.
+
+@item read-write
+Makes the device writable.
+@end table
+
 @item change vnc @var{display},@var{options}
 Change the configuration of the VNC server. The valid syntax for @var{display}
 and @var{options} are described at @ref{sec_invocation}. eg
diff --git a/hmp.c b/hmp.c
index efa07a2..8166fd2 100644
--- a/hmp.c
+++ b/hmp.c
@@ -27,6 +27,7 @@
 #include "qapi/opts-visitor.h"
 #include "qapi/qmp/qerror.h"
 #include "qapi/string-output-visitor.h"
+#include "qapi/util.h"
 #include "qapi-visit.h"
 #include "ui/console.h"
 #include "block/qapi.h"
@@ -1336,9 +1337,16 @@ void hmp_change(Monitor *mon, const QDict *qdict)
     const char *device = qdict_get_str(qdict, "device");
     const char *target = qdict_get_str(qdict, "target");
     const char *arg = qdict_get_try_str(qdict, "arg");
+    const char *read_only = qdict_get_try_str(qdict, "read-only-mode");
+    BlockdevChangeReadOnlyMode read_only_mode = 0;
     Error *err = NULL;
 
     if (strcmp(device, "vnc") == 0) {
+        if (read_only) {
+            monitor_printf(mon,
+                           "Parameter 'read-only-mode' is invalid for VNC\n");
+            return;
+        }
         if (strcmp(target, "passwd") == 0 ||
             strcmp(target, "password") == 0) {
             if (!arg) {
@@ -1348,7 +1356,19 @@ void hmp_change(Monitor *mon, const QDict *qdict)
         }
         qmp_change("vnc", target, !!arg, arg, &err);
     } else {
-        qmp_blockdev_change_medium(device, target, !!arg, arg, false, 0, &err);
+        if (read_only) {
+            read_only_mode =
+                qapi_enum_parse(BlockdevChangeReadOnlyMode_lookup,
+                                read_only, BLOCKDEV_CHANGE_READ_ONLY_MODE_MAX,
+                                BLOCKDEV_CHANGE_READ_ONLY_MODE_RETAIN, &err);
+            if (err) {
+                hmp_handle_error(mon, &err);
+                return;
+            }
+        }
+
+        qmp_blockdev_change_medium(device, target, !!arg, arg,
+                                   !!read_only, read_only_mode, &err);
         if (err &&
             error_get_class(err) == ERROR_CLASS_DEVICE_ENCRYPTED) {
             error_free(err);
-- 
1.8.3.1

[Qemu-devel] [PULL v2 16/40] iotests: Add test for change-related QMP commands

[Kevin Wolf]

From: Max Reitz <mreitz@redhat.com>

Signed-off-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/118     | 720 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/118.out |   5 +
 tests/qemu-iotests/group   |   1 +
 3 files changed, 726 insertions(+)
 create mode 100755 tests/qemu-iotests/118
 create mode 100644 tests/qemu-iotests/118.out

diff --git a/tests/qemu-iotests/118 b/tests/qemu-iotests/118
new file mode 100755
index 0000000..a2bcd54
--- /dev/null
+++ b/tests/qemu-iotests/118
@@ -0,0 +1,720 @@
+#!/usr/bin/env python
+#
+# Test case for the QMP 'change' command and all other associated
+# commands
+#
+# Copyright (C) 2015 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+import os
+import stat
+import time
+import iotests
+from iotests import qemu_img
+
+old_img = os.path.join(iotests.test_dir, 'test0.img')
+new_img = os.path.join(iotests.test_dir, 'test1.img')
+
+class ChangeBaseClass(iotests.QMPTestCase):
+    has_opened = False
+    has_closed = False
+
+    def process_events(self):
+        for event in self.vm.get_qmp_events(wait=False):
+            if (event['event'] == 'DEVICE_TRAY_MOVED' and
+                event['data']['device'] == 'drive0'):
+                if event['data']['tray-open'] == False:
+                    self.has_closed = True
+                else:
+                    self.has_opened = True
+
+    def wait_for_open(self):
+        timeout = time.clock() + 3
+        while not self.has_opened and time.clock() < timeout:
+            self.process_events()
+        if not self.has_opened:
+            self.fail('Timeout while waiting for the tray to open')
+
+    def wait_for_close(self):
+        timeout = time.clock() + 3
+        while not self.has_closed and time.clock() < timeout:
+            self.process_events()
+        if not self.has_opened:
+            self.fail('Timeout while waiting for the tray to close')
+
+class GeneralChangeTestsBaseClass(ChangeBaseClass):
+    def test_change(self):
+        result = self.vm.qmp('change', device='drive0', target=new_img,
+                                       arg=iotests.imgfmt)
+        self.assert_qmp(result, 'return', {})
+
+        self.wait_for_open()
+        self.wait_for_close()
+
+        result = self.vm.qmp('query-block')
+        self.assert_qmp(result, 'return[0]/tray_open', False)
+        self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
+
+    def test_blockdev_change_medium(self):
+        result = self.vm.qmp('blockdev-change-medium', device='drive0',
+                                                       filename=new_img,
+                                                       format=iotests.imgfmt)
+        self.assert_qmp(result, 'return', {})
+
+        self.wait_for_open()
+        self.wait_for_close()
+
+        result = self.vm.qmp('query-block')
+        self.assert_qmp(result, 'return[0]/tray_open', False)
+        self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
+
+    def test_eject(self):
+        result = self.vm.qmp('eject', device='drive0', force=True)
+        self.assert_qmp(result, 'return', {})
+
+        self.wait_for_open()
+
+        result = self.vm.qmp('query-block')
+        self.assert_qmp(result, 'return[0]/tray_open', True)
+        self.assert_qmp_absent(result, 'return[0]/inserted')
+
+    def test_tray_eject_change(self):
+        result = self.vm.qmp('eject', device='drive0', force=True)
+        self.assert_qmp(result, 'return', {})
+
+        self.wait_for_open()
+
+        result = self.vm.qmp('query-block')
+        self.assert_qmp(result, 'return[0]/tray_open', True)
+        self.assert_qmp_absent(result, 'return[0]/inserted')
+
+        result = self.vm.qmp('blockdev-change-medium', device='drive0',
+                                                       filename=new_img,
+                                                       format=iotests.imgfmt)
+        self.assert_qmp(result, 'return', {})
+
+        self.wait_for_close()
+
+        result = self.vm.qmp('query-block')
+        self.assert_qmp(result, 'return[0]/tray_open', False)
+        self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
+
+    def test_tray_open_close(self):
+        result = self.vm.qmp('blockdev-open-tray', device='drive0', force=True)
+        self.assert_qmp(result, 'return', {})
+
+        self.wait_for_open()
+
+        result = self.vm.qmp('query-block')
+        self.assert_qmp(result, 'return[0]/tray_open', True)
+        if self.was_empty == True:
+            self.assert_qmp_absent(result, 'return[0]/inserted')
+        else:
+            self.assert_qmp(result, 'return[0]/inserted/image/filename', old_img)
+
+        result = self.vm.qmp('blockdev-close-tray', device='drive0')
+        self.assert_qmp(result, 'return', {})
+
+        if self.has_real_tray or not self.was_empty:
+            self.wait_for_close()
+
+        result = self.vm.qmp('query-block')
+        if self.has_real_tray or not self.was_empty:
+            self.assert_qmp(result, 'return[0]/tray_open', False)
+        else:
+            self.assert_qmp(result, 'return[0]/tray_open', True)
+        if self.was_empty == True:
+            self.assert_qmp_absent(result, 'return[0]/inserted')
+        else:
+            self.assert_qmp(result, 'return[0]/inserted/image/filename', old_img)
+
+    def test_tray_eject_close(self):
+        result = self.vm.qmp('eject', device='drive0', force=True)
+        self.assert_qmp(result, 'return', {})
+
+        self.wait_for_open()
+
+        result = self.vm.qmp('query-block')
+        self.assert_qmp(result, 'return[0]/tray_open', True)
+        self.assert_qmp_absent(result, 'return[0]/inserted')
+
+        result = self.vm.qmp('blockdev-close-tray', device='drive0')
+        self.assert_qmp(result, 'return', {})
+
+        if self.has_real_tray:
+            self.wait_for_close()
+
+        result = self.vm.qmp('query-block')
+        if self.has_real_tray:
+            self.assert_qmp(result, 'return[0]/tray_open', False)
+        else:
+            self.assert_qmp(result, 'return[0]/tray_open', True)
+        self.assert_qmp_absent(result, 'return[0]/inserted')
+
+    def test_tray_open_change(self):
+        result = self.vm.qmp('blockdev-open-tray', device='drive0', force=True)
+        self.assert_qmp(result, 'return', {})
+
+        self.wait_for_open()
+
+        result = self.vm.qmp('query-block')
+        self.assert_qmp(result, 'return[0]/tray_open', True)
+        if self.was_empty == True:
+            self.assert_qmp_absent(result, 'return[0]/inserted')
+        else:
+            self.assert_qmp(result, 'return[0]/inserted/image/filename', old_img)
+
+        result = self.vm.qmp('blockdev-change-medium', device='drive0',
+                                                       filename=new_img,
+                                                       format=iotests.imgfmt)
+        self.assert_qmp(result, 'return', {})
+
+        self.wait_for_close()
+
+        result = self.vm.qmp('query-block')
+        self.assert_qmp(result, 'return[0]/tray_open', False)
+        self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
+
+    def test_cycle(self):
+        result = self.vm.qmp('blockdev-add',
+                             options={'node-name': 'new',
+                                      'driver': iotests.imgfmt,
+                                      'file': {'filename': new_img,
+                                               'driver': 'file'}})
+        self.assert_qmp(result, 'return', {})
+
+        result = self.vm.qmp('blockdev-open-tray', device='drive0', force=True)
+        self.assert_qmp(result, 'return', {})
+
+        self.wait_for_open()
+
+        result = self.vm.qmp('query-block')
+        self.assert_qmp(result, 'return[0]/tray_open', True)
+        if self.was_empty == True:
+            self.assert_qmp_absent(result, 'return[0]/inserted')
+        else:
+            self.assert_qmp(result, 'return[0]/inserted/image/filename', old_img)
+
+        result = self.vm.qmp('blockdev-remove-medium', device='drive0')
+        self.assert_qmp(result, 'return', {})
+
+        result = self.vm.qmp('query-block')
+        self.assert_qmp(result, 'return[0]/tray_open', True)
+        self.assert_qmp_absent(result, 'return[0]/inserted')
+
+        result = self.vm.qmp('blockdev-insert-medium', device='drive0',
+                                                       node_name='new')
+        self.assert_qmp(result, 'return', {})
+
+        result = self.vm.qmp('query-block')
+        self.assert_qmp(result, 'return[0]/tray_open', True)
+        self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
+
+        result = self.vm.qmp('blockdev-close-tray', device='drive0')
+        self.assert_qmp(result, 'return', {})
+
+        self.wait_for_close()
+
+        result = self.vm.qmp('query-block')
+        self.assert_qmp(result, 'return[0]/tray_open', False)
+        self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
+
+    def test_close_on_closed(self):
+        result = self.vm.qmp('blockdev-close-tray', device='drive0')
+        # Should be a no-op
+        self.assert_qmp(result, 'return', {})
+        self.assertEquals(self.vm.get_qmp_events(wait=False), [])
+
+    def test_remove_on_closed(self):
+        if self.has_opened:
+            # Empty floppy drive
+            return
+
+        result = self.vm.qmp('blockdev-remove-medium', device='drive0')
+        self.assert_qmp(result, 'error/class', 'GenericError')
+
+    def test_insert_on_closed(self):
+        if self.has_opened:
+            # Empty floppy drive
+            return
+
+        result = self.vm.qmp('blockdev-add',
+                             options={'node-name': 'new',
+                                      'driver': iotests.imgfmt,
+                                      'file': {'filename': new_img,
+                                               'driver': 'file'}})
+        self.assert_qmp(result, 'return', {})
+
+        result = self.vm.qmp('blockdev-insert-medium', device='drive0',
+                                                       node_name='new')
+        self.assert_qmp(result, 'error/class', 'GenericError')
+
+class TestInitiallyFilled(GeneralChangeTestsBaseClass):
+    was_empty = False
+
+    def setUp(self, media, interface):
+        qemu_img('create', '-f', iotests.imgfmt, old_img, '1440k')
+        qemu_img('create', '-f', iotests.imgfmt, new_img, '1440k')
+        self.vm = iotests.VM().add_drive(old_img, 'media=%s' % media, interface)
+        self.vm.launch()
+
+    def tearDown(self):
+        self.vm.shutdown()
+        os.remove(old_img)
+        os.remove(new_img)
+
+    def test_insert_on_filled(self):
+        result = self.vm.qmp('blockdev-add',
+                             options={'node-name': 'new',
+                                      'driver': iotests.imgfmt,
+                                      'file': {'filename': new_img,
+                                               'driver': 'file'}})
+        self.assert_qmp(result, 'return', {})
+
+        result = self.vm.qmp('blockdev-open-tray', device='drive0')
+        self.assert_qmp(result, 'return', {})
+
+        self.wait_for_open()
+
+        result = self.vm.qmp('blockdev-insert-medium', device='drive0',
+                                                       node_name='new')
+        self.assert_qmp(result, 'error/class', 'GenericError')
+
+class TestInitiallyEmpty(GeneralChangeTestsBaseClass):
+    was_empty = True
+
+    def setUp(self, media, interface):
+        qemu_img('create', '-f', iotests.imgfmt, new_img, '1440k')
+        self.vm = iotests.VM().add_drive(None, 'media=%s' % media, interface)
+        self.vm.launch()
+
+    def tearDown(self):
+        self.vm.shutdown()
+        os.remove(new_img)
+
+    def test_remove_on_empty(self):
+        result = self.vm.qmp('blockdev-open-tray', device='drive0')
+        self.assert_qmp(result, 'return', {})
+
+        self.wait_for_open()
+
+        result = self.vm.qmp('blockdev-remove-medium', device='drive0')
+        # Should be a no-op
+        self.assert_qmp(result, 'return', {})
+
+class TestCDInitiallyFilled(TestInitiallyFilled):
+    TestInitiallyFilled = TestInitiallyFilled
+    has_real_tray = True
+
+    def setUp(self):
+        self.TestInitiallyFilled.setUp(self, 'cdrom', 'ide')
+
+class TestCDInitiallyEmpty(TestInitiallyEmpty):
+    TestInitiallyEmpty = TestInitiallyEmpty
+    has_real_tray = True
+
+    def setUp(self):
+        self.TestInitiallyEmpty.setUp(self, 'cdrom', 'ide')
+
+class TestFloppyInitiallyFilled(TestInitiallyFilled):
+    TestInitiallyFilled = TestInitiallyFilled
+    has_real_tray = False
+
+    def setUp(self):
+        self.TestInitiallyFilled.setUp(self, 'disk', 'floppy')
+
+class TestFloppyInitiallyEmpty(TestInitiallyEmpty):
+    TestInitiallyEmpty = TestInitiallyEmpty
+    has_real_tray = False
+
+    def setUp(self):
+        self.TestInitiallyEmpty.setUp(self, 'disk', 'floppy')
+        # FDDs not having a real tray and there not being a medium inside the
+        # tray at startup means the tray will be considered open
+        self.has_opened = True
+
+class TestChangeReadOnly(ChangeBaseClass):
+    def setUp(self):
+        qemu_img('create', '-f', iotests.imgfmt, old_img, '1440k')
+        qemu_img('create', '-f', iotests.imgfmt, new_img, '1440k')
+        self.vm = iotests.VM()
+
+    def tearDown(self):
+        self.vm.shutdown()
+        os.chmod(old_img, 0666)
+        os.chmod(new_img, 0666)
+        os.remove(old_img)
+        os.remove(new_img)
+
+    def test_ro_ro_retain(self):
+        os.chmod(old_img, 0444)
+        os.chmod(new_img, 0444)
+        self.vm.add_drive(old_img, 'media=disk,read-only=on', 'floppy')
+        self.vm.launch()
+
+        result = self.vm.qmp('query-block')
+        self.assert_qmp(result, 'return[0]/tray_open', False)
+        self.assert_qmp(result, 'return[0]/inserted/ro', True)
+        self.assert_qmp(result, 'return[0]/inserted/image/filename', old_img)
+
+        result = self.vm.qmp('blockdev-change-medium', device='drive0',
+                                                       filename=new_img,
+                                                       format=iotests.imgfmt,
+                                                       read_only_mode='retain')
+        self.assert_qmp(result, 'return', {})
+
+        self.wait_for_open()
+        self.wait_for_close()
+
+        result = self.vm.qmp('query-block')
+        self.assert_qmp(result, 'return[0]/tray_open', False)
+        self.assert_qmp(result, 'return[0]/inserted/ro', True)
+        self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
+
+    def test_ro_rw_retain(self):
+        os.chmod(old_img, 0444)
+        self.vm.add_drive(old_img, 'media=disk,read-only=on', 'floppy')
+        self.vm.launch()
+
+        result = self.vm.qmp('query-block')
+        self.assert_qmp(result, 'return[0]/tray_open', False)
+        self.assert_qmp(result, 'return[0]/inserted/ro', True)
+        self.assert_qmp(result, 'return[0]/inserted/image/filename', old_img)
+
+        result = self.vm.qmp('blockdev-change-medium', device='drive0',
+                                                       filename=new_img,
+                                                       format=iotests.imgfmt,
+                                                       read_only_mode='retain')
+        self.assert_qmp(result, 'return', {})
+
+        self.wait_for_open()
+        self.wait_for_close()
+
+        result = self.vm.qmp('query-block')
+        self.assert_qmp(result, 'return[0]/tray_open', False)
+        self.assert_qmp(result, 'return[0]/inserted/ro', True)
+        self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
+
+    def test_rw_ro_retain(self):
+        os.chmod(new_img, 0444)
+        self.vm.add_drive(old_img, 'media=disk', 'floppy')
+        self.vm.launch()
+
+        result = self.vm.qmp('query-block')
+        self.assert_qmp(result, 'return[0]/tray_open', False)
+        self.assert_qmp(result, 'return[0]/inserted/ro', False)
+        self.assert_qmp(result, 'return[0]/inserted/image/filename', old_img)
+
+        result = self.vm.qmp('blockdev-change-medium', device='drive0',
+                                                       filename=new_img,
+                                                       format=iotests.imgfmt,
+                                                       read_only_mode='retain')
+        self.assert_qmp(result, 'error/class', 'GenericError')
+
+        self.assertEquals(self.vm.get_qmp_events(wait=False), [])
+
+        result = self.vm.qmp('query-block')
+        self.assert_qmp(result, 'return[0]/tray_open', False)
+        self.assert_qmp(result, 'return[0]/inserted/ro', False)
+        self.assert_qmp(result, 'return[0]/inserted/image/filename', old_img)
+
+    def test_ro_rw(self):
+        os.chmod(old_img, 0444)
+        self.vm.add_drive(old_img, 'media=disk,read-only=on', 'floppy')
+        self.vm.launch()
+
+        result = self.vm.qmp('query-block')
+        self.assert_qmp(result, 'return[0]/tray_open', False)
+        self.assert_qmp(result, 'return[0]/inserted/ro', True)
+        self.assert_qmp(result, 'return[0]/inserted/image/filename', old_img)
+
+        result = self.vm.qmp('blockdev-change-medium',
+                             device='drive0',
+                             filename=new_img,
+                             format=iotests.imgfmt,
+                             read_only_mode='read-write')
+        self.assert_qmp(result, 'return', {})
+
+        self.wait_for_open()
+        self.wait_for_close()
+
+        result = self.vm.qmp('query-block')
+        self.assert_qmp(result, 'return[0]/tray_open', False)
+        self.assert_qmp(result, 'return[0]/inserted/ro', False)
+        self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
+
+    def test_rw_ro(self):
+        os.chmod(new_img, 0444)
+        self.vm.add_drive(old_img, 'media=disk', 'floppy')
+        self.vm.launch()
+
+        result = self.vm.qmp('query-block')
+        self.assert_qmp(result, 'return[0]/tray_open', False)
+        self.assert_qmp(result, 'return[0]/inserted/ro', False)
+        self.assert_qmp(result, 'return[0]/inserted/image/filename', old_img)
+
+        result = self.vm.qmp('blockdev-change-medium',
+                             device='drive0',
+                             filename=new_img,
+                             format=iotests.imgfmt,
+                             read_only_mode='read-only')
+        self.assert_qmp(result, 'return', {})
+
+        self.wait_for_open()
+        self.wait_for_close()
+
+        result = self.vm.qmp('query-block')
+        self.assert_qmp(result, 'return[0]/tray_open', False)
+        self.assert_qmp(result, 'return[0]/inserted/ro', True)
+        self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
+
+    def test_make_rw_ro(self):
+        self.vm.add_drive(old_img, 'media=disk', 'floppy')
+        self.vm.launch()
+
+        result = self.vm.qmp('query-block')
+        self.assert_qmp(result, 'return[0]/tray_open', False)
+        self.assert_qmp(result, 'return[0]/inserted/ro', False)
+        self.assert_qmp(result, 'return[0]/inserted/image/filename', old_img)
+
+        result = self.vm.qmp('blockdev-change-medium',
+                             device='drive0',
+                             filename=new_img,
+                             format=iotests.imgfmt,
+                             read_only_mode='read-only')
+        self.assert_qmp(result, 'return', {})
+
+        self.wait_for_open()
+        self.wait_for_close()
+
+        result = self.vm.qmp('query-block')
+        self.assert_qmp(result, 'return[0]/tray_open', False)
+        self.assert_qmp(result, 'return[0]/inserted/ro', True)
+        self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
+
+    def test_make_ro_rw(self):
+        os.chmod(new_img, 0444)
+        self.vm.add_drive(old_img, 'media=disk', 'floppy')
+        self.vm.launch()
+
+        result = self.vm.qmp('query-block')
+        self.assert_qmp(result, 'return[0]/tray_open', False)
+        self.assert_qmp(result, 'return[0]/inserted/ro', False)
+        self.assert_qmp(result, 'return[0]/inserted/image/filename', old_img)
+
+        result = self.vm.qmp('blockdev-change-medium',
+                             device='drive0',
+                             filename=new_img,
+                             format=iotests.imgfmt,
+                             read_only_mode='read-write')
+        self.assert_qmp(result, 'error/class', 'GenericError')
+
+        self.assertEquals(self.vm.get_qmp_events(wait=False), [])
+
+        result = self.vm.qmp('query-block')
+        self.assert_qmp(result, 'return[0]/tray_open', False)
+        self.assert_qmp(result, 'return[0]/inserted/ro', False)
+        self.assert_qmp(result, 'return[0]/inserted/image/filename', old_img)
+
+    def test_make_rw_ro_by_retain(self):
+        os.chmod(old_img, 0444)
+        self.vm.add_drive(old_img, 'media=disk,read-only=on', 'floppy')
+        self.vm.launch()
+
+        result = self.vm.qmp('query-block')
+        self.assert_qmp(result, 'return[0]/tray_open', False)
+        self.assert_qmp(result, 'return[0]/inserted/ro', True)
+        self.assert_qmp(result, 'return[0]/inserted/image/filename', old_img)
+
+        result = self.vm.qmp('blockdev-change-medium', device='drive0',
+                                                       filename=new_img,
+                                                       format=iotests.imgfmt,
+                                                       read_only_mode='retain')
+        self.assert_qmp(result, 'return', {})
+
+        self.wait_for_open()
+        self.wait_for_close()
+
+        result = self.vm.qmp('query-block')
+        self.assert_qmp(result, 'return[0]/tray_open', False)
+        self.assert_qmp(result, 'return[0]/inserted/ro', True)
+        self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
+
+    def test_make_ro_rw_by_retain(self):
+        os.chmod(new_img, 0444)
+        self.vm.add_drive(old_img, 'media=disk', 'floppy')
+        self.vm.launch()
+
+        result = self.vm.qmp('query-block')
+        self.assert_qmp(result, 'return[0]/tray_open', False)
+        self.assert_qmp(result, 'return[0]/inserted/ro', False)
+        self.assert_qmp(result, 'return[0]/inserted/image/filename', old_img)
+
+        result = self.vm.qmp('blockdev-change-medium', device='drive0',
+                                                       filename=new_img,
+                                                       format=iotests.imgfmt,
+                                                       read_only_mode='retain')
+        self.assert_qmp(result, 'error/class', 'GenericError')
+
+        self.assertEquals(self.vm.get_qmp_events(wait=False), [])
+
+        result = self.vm.qmp('query-block')
+        self.assert_qmp(result, 'return[0]/tray_open', False)
+        self.assert_qmp(result, 'return[0]/inserted/ro', False)
+        self.assert_qmp(result, 'return[0]/inserted/image/filename', old_img)
+
+    def test_rw_ro_cycle(self):
+        os.chmod(new_img, 0444)
+        self.vm.add_drive(old_img, 'media=disk', 'floppy')
+        self.vm.launch()
+
+        result = self.vm.qmp('query-block')
+        self.assert_qmp(result, 'return[0]/tray_open', False)
+        self.assert_qmp(result, 'return[0]/inserted/ro', False)
+        self.assert_qmp(result, 'return[0]/inserted/image/filename', old_img)
+
+        result = self.vm.qmp('blockdev-add',
+                             options={'node-name': 'new',
+                                      'driver': iotests.imgfmt,
+                                      'read-only': True,
+                                      'file': {'filename': new_img,
+                                               'driver': 'file'}})
+        self.assert_qmp(result, 'return', {})
+
+        result = self.vm.qmp('blockdev-open-tray', device='drive0', force=True)
+        self.assert_qmp(result, 'return', {})
+
+        self.wait_for_open()
+
+        result = self.vm.qmp('query-block')
+        self.assert_qmp(result, 'return[0]/tray_open', True)
+        self.assert_qmp(result, 'return[0]/inserted/ro', False)
+        self.assert_qmp(result, 'return[0]/inserted/image/filename', old_img)
+
+        result = self.vm.qmp('blockdev-remove-medium', device='drive0')
+        self.assert_qmp(result, 'return', {})
+
+        result = self.vm.qmp('query-block')
+        self.assert_qmp(result, 'return[0]/tray_open', True)
+        self.assert_qmp_absent(result, 'return[0]/inserted')
+
+        result = self.vm.qmp('blockdev-insert-medium', device='drive0',
+                                                       node_name='new')
+        self.assert_qmp(result, 'return', {})
+
+        result = self.vm.qmp('query-block')
+        self.assert_qmp(result, 'return[0]/tray_open', True)
+        self.assert_qmp(result, 'return[0]/inserted/ro', True)
+        self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
+
+        result = self.vm.qmp('blockdev-close-tray', device='drive0')
+        self.assert_qmp(result, 'return', {})
+
+        self.wait_for_close()
+
+        result = self.vm.qmp('query-block')
+        self.assert_qmp(result, 'return[0]/tray_open', False)
+        self.assert_qmp(result, 'return[0]/inserted/ro', True)
+        self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
+
+GeneralChangeTestsBaseClass = None
+TestInitiallyFilled = None
+TestInitiallyEmpty = None
+
+
+class TestBlockJobsAfterCycle(ChangeBaseClass):
+    def setUp(self):
+        qemu_img('create', '-f', iotests.imgfmt, old_img, '1M')
+
+        self.vm = iotests.VM()
+        self.vm.launch()
+
+        result = self.vm.qmp('blockdev-add',
+                             options={'id': 'drive0',
+                                      'driver': 'null-co'})
+        self.assert_qmp(result, 'return', {})
+
+        result = self.vm.qmp('query-block')
+        self.assert_qmp(result, 'return[0]/tray_open', False)
+        self.assert_qmp(result, 'return[0]/inserted/image/format', 'null-co')
+
+        # For device-less BBs, calling blockdev-open-tray or blockdev-close-tray
+        # is not necessary
+        result = self.vm.qmp('blockdev-remove-medium', device='drive0')
+        self.assert_qmp(result, 'return', {})
+
+        result = self.vm.qmp('query-block')
+        self.assert_qmp_absent(result, 'return[0]/inserted')
+
+        result = self.vm.qmp('blockdev-add',
+                             options={'node-name': 'node0',
+                                      'driver': iotests.imgfmt,
+                                      'file': {'filename': old_img,
+                                               'driver': 'file'}})
+        self.assert_qmp(result, 'return', {})
+
+        result = self.vm.qmp('blockdev-insert-medium', device='drive0',
+                                                       node_name='node0')
+        self.assert_qmp(result, 'return', {})
+
+        result = self.vm.qmp('query-block')
+        self.assert_qmp(result, 'return[0]/tray_open', False)
+        self.assert_qmp(result, 'return[0]/inserted/image/filename', old_img)
+
+    def tearDown(self):
+        self.vm.shutdown()
+        os.remove(old_img)
+        try:
+            os.remove(new_img)
+        except OSError:
+            pass
+
+    def test_snapshot_and_commit(self):
+        # We need backing file support
+        if iotests.imgfmt != 'qcow2' and iotests.imgfmt != 'qed':
+            return
+
+        result = self.vm.qmp('blockdev-snapshot-sync', device='drive0',
+                                                       snapshot_file=new_img,
+                                                       format=iotests.imgfmt)
+        self.assert_qmp(result, 'return', {})
+
+        result = self.vm.qmp('query-block')
+        self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
+        self.assert_qmp(result,
+                        'return[0]/inserted/image/backing-image/filename',
+                        old_img)
+
+        result = self.vm.qmp('block-commit', device='drive0')
+        self.assert_qmp(result, 'return', {})
+
+        self.vm.event_wait(name='BLOCK_JOB_READY')
+
+        result = self.vm.qmp('query-block-jobs')
+        self.assert_qmp(result, 'return[0]/device', 'drive0')
+
+        result = self.vm.qmp('block-job-complete', device='drive0')
+        self.assert_qmp(result, 'return', {})
+
+        self.vm.event_wait(name='BLOCK_JOB_COMPLETED')
+
+
+if __name__ == '__main__':
+    if iotests.qemu_default_machine != 'pc':
+        # We need floppy and IDE CD-ROM
+        iotests.notrun('not suitable for this machine type: %s' %
+                       iotests.qemu_default_machine)
+    iotests.main()
diff --git a/tests/qemu-iotests/118.out b/tests/qemu-iotests/118.out
new file mode 100644
index 0000000..6a91713
--- /dev/null
+++ b/tests/qemu-iotests/118.out
@@ -0,0 +1,5 @@
+...........................................................
+----------------------------------------------------------------------
+Ran 59 tests
+
+OK
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 30c784e..a6a61ae 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -122,6 +122,7 @@
 114 rw auto quick
 115 rw auto
 116 rw auto quick
+118 rw auto
 119 rw auto quick
 120 rw auto quick
 121 rw auto
-- 
1.8.3.1

[Qemu-devel] [PULL v2 17/40] block: check for existing device IDs in external_snapshot_prepare()

[Kevin Wolf]

From: Alberto Garcia <berto@igalia.com>

The 'snapshot-node-name' parameter of blockdev-snapshot-sync allows
setting the node name of the image that is going to be created.

Before creating the image, external_snapshot_prepare() checks that the
name is not already being used. The check is however incomplete since
it only considers existing node names, but node names must not clash
with device IDs either because they share the same namespace.

If the user attempts to create a snapshot using the name of an
existing device for the 'snapshot-node-name' parameter the operation
will eventually fail, but only after the new image has been created.

This patch replaces bdrv_find_node() with bdrv_lookup_bs() to extend
the check to existing device IDs, and thus detect possible name
clashes before the new image is created.

Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Jeff Cody <jcody@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 blockdev.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 34f6e5b..0c6c2a7 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1579,8 +1579,9 @@ static void external_snapshot_prepare(BlkTransactionState *common,
         return;
     }
 
-    if (has_snapshot_node_name && bdrv_find_node(snapshot_node_name)) {
-        error_setg(errp, "New snapshot node name already existing");
+    if (has_snapshot_node_name &&
+        bdrv_lookup_bs(snapshot_node_name, snapshot_node_name, NULL)) {
+        error_setg(errp, "New snapshot node name already in use");
         return;
     }
 
-- 
1.8.3.1

[Qemu-devel] [PULL v2 18/40] block: rename BlockdevSnapshot to BlockdevSnapshotSync

[Kevin Wolf]

From: Alberto Garcia <berto@igalia.com>

We will introduce the 'blockdev-snapshot' command that will require
its own struct for the parameters, so we need to rename this one in
order to avoid name clashes.

Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Jeff Cody <jcody@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 blockdev.c           | 2 +-
 qapi-schema.json     | 2 +-
 qapi/block-core.json | 8 ++++----
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 0c6c2a7..86a29d8 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1168,7 +1168,7 @@ void qmp_blockdev_snapshot_sync(bool has_device, const char *device,
                                 bool has_format, const char *format,
                                 bool has_mode, NewImageMode mode, Error **errp)
 {
-    BlockdevSnapshot snapshot = {
+    BlockdevSnapshotSync snapshot = {
         .has_device = has_device,
         .device = (char *) device,
         .has_node_name = has_node_name,
diff --git a/qapi-schema.json b/qapi-schema.json
index ed7fc99..42694d2 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1534,7 +1534,7 @@
 ##
 { 'union': 'TransactionAction',
   'data': {
-       'blockdev-snapshot-sync': 'BlockdevSnapshot',
+       'blockdev-snapshot-sync': 'BlockdevSnapshotSync',
        'drive-backup': 'DriveBackup',
        'blockdev-backup': 'BlockdevBackup',
        'abort': 'Abort',
diff --git a/qapi/block-core.json b/qapi/block-core.json
index fa08ba9..f592adc 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -682,7 +682,7 @@
   'data': [ 'existing', 'absolute-paths' ] }
 
 ##
-# @BlockdevSnapshot
+# @BlockdevSnapshotSync
 #
 # Either @device or @node-name must be set but not both.
 #
@@ -699,7 +699,7 @@
 # @mode: #optional whether and how QEMU should create a new image, default is
 #        'absolute-paths'.
 ##
-{ 'struct': 'BlockdevSnapshot',
+{ 'struct': 'BlockdevSnapshotSync',
   'data': { '*device': 'str', '*node-name': 'str',
             'snapshot-file': 'str', '*snapshot-node-name': 'str',
             '*format': 'str', '*mode': 'NewImageMode' } }
@@ -790,7 +790,7 @@
 #
 # Generates a synchronous snapshot of a block device.
 #
-# For the arguments, see the documentation of BlockdevSnapshot.
+# For the arguments, see the documentation of BlockdevSnapshotSync.
 #
 # Returns: nothing on success
 #          If @device is not a valid block device, DeviceNotFound
@@ -798,7 +798,7 @@
 # Since 0.14.0
 ##
 { 'command': 'blockdev-snapshot-sync',
-  'data': 'BlockdevSnapshot' }
+  'data': 'BlockdevSnapshotSync' }
 
 ##
 # @change-backing-file
-- 
1.8.3.1

[Qemu-devel] [PULL v2 19/40] block: support passing 'backing': '' to 'blockdev-add'

[Kevin Wolf]

From: Alberto Garcia <berto@igalia.com>

Passing an empty string allows opening an image but not its backing
file. This was already described in the API documentation, only the
implementation was missing.

This is useful for creating snapshots using images opened with
blockdev-add, since they are not supposed to have a backing image
before the operation.

Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Jeff Cody <jcody@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/block.c b/block.c
index a99e6d8..3493501 100644
--- a/block.c
+++ b/block.c
@@ -1393,6 +1393,7 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
     BlockDriverState *bs;
     BlockDriver *drv = NULL;
     const char *drvname;
+    const char *backing;
     Error *local_err = NULL;
     int snapshot_flags = 0;
 
@@ -1460,6 +1461,12 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
 
     assert(drvname || !(flags & BDRV_O_PROTOCOL));
 
+    backing = qdict_get_try_str(options, "backing");
+    if (backing && *backing == '\0') {
+        flags |= BDRV_O_NO_BACKING;
+        qdict_del(options, "backing");
+    }
+
     bs->open_flags = flags;
     bs->options = options;
     options = qdict_clone_shallow(options);
-- 
1.8.3.1

[Qemu-devel] [PULL v2 20/40] block: add a 'blockdev-snapshot' QMP command

[Kevin Wolf]

From: Alberto Garcia <berto@igalia.com>

One of the limitations of the 'blockdev-snapshot-sync' command is that
it does not allow passing BlockdevOptions to the newly created
snapshots, so they are always opened using the default values.

Extending the command to allow passing options is not a practical
solution because there is overlap between those options and some of
the existing parameters of the command.

This patch introduces a new 'blockdev-snapshot' command with a simpler
interface: it just takes two references to existing block devices that
will be used as the source and target for the snapshot.

Since the main difference between the two commands is that one of them
creates and opens the target image, while the other uses an already
opened one, the bulk of the implementation is shared.

Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 blockdev.c           | 165 ++++++++++++++++++++++++++++++++-------------------
 qapi-schema.json     |   2 +
 qapi/block-core.json |  28 +++++++++
 qmp-commands.hx      |  38 ++++++++++++
 4 files changed, 172 insertions(+), 61 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 86a29d8..7645d49 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1185,6 +1185,18 @@ void qmp_blockdev_snapshot_sync(bool has_device, const char *device,
                        &snapshot, errp);
 }
 
+void qmp_blockdev_snapshot(const char *node, const char *overlay,
+                           Error **errp)
+{
+    BlockdevSnapshot snapshot_data = {
+        .node = (char *) node,
+        .overlay = (char *) overlay
+    };
+
+    blockdev_do_action(TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT,
+                       &snapshot_data, errp);
+}
+
 void qmp_blockdev_snapshot_internal_sync(const char *device,
                                          const char *name,
                                          Error **errp)
@@ -1530,58 +1542,48 @@ typedef struct ExternalSnapshotState {
 static void external_snapshot_prepare(BlkTransactionState *common,
                                       Error **errp)
 {
-    int flags, ret;
-    QDict *options;
+    int flags = 0, ret;
+    QDict *options = NULL;
     Error *local_err = NULL;
-    bool has_device = false;
+    /* Device and node name of the image to generate the snapshot from */
     const char *device;
-    bool has_node_name = false;
     const char *node_name;
-    bool has_snapshot_node_name = false;
-    const char *snapshot_node_name;
+    /* Reference to the new image (for 'blockdev-snapshot') */
+    const char *snapshot_ref;
+    /* File name of the new image (for 'blockdev-snapshot-sync') */
     const char *new_image_file;
-    const char *format = "qcow2";
-    enum NewImageMode mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
     ExternalSnapshotState *state =
                              DO_UPCAST(ExternalSnapshotState, common, common);
     TransactionAction *action = common->action;
 
-    /* get parameters */
-    g_assert(action->type == TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC);
-
-    has_device = action->u.blockdev_snapshot_sync->has_device;
-    device = action->u.blockdev_snapshot_sync->device;
-    has_node_name = action->u.blockdev_snapshot_sync->has_node_name;
-    node_name = action->u.blockdev_snapshot_sync->node_name;
-    has_snapshot_node_name =
-        action->u.blockdev_snapshot_sync->has_snapshot_node_name;
-    snapshot_node_name = action->u.blockdev_snapshot_sync->snapshot_node_name;
-
-    new_image_file = action->u.blockdev_snapshot_sync->snapshot_file;
-    if (action->u.blockdev_snapshot_sync->has_format) {
-        format = action->u.blockdev_snapshot_sync->format;
-    }
-    if (action->u.blockdev_snapshot_sync->has_mode) {
-        mode = action->u.blockdev_snapshot_sync->mode;
+    /* 'blockdev-snapshot' and 'blockdev-snapshot-sync' have similar
+     * purpose but a different set of parameters */
+    switch (action->type) {
+    case TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT:
+        {
+            BlockdevSnapshot *s = action->u.blockdev_snapshot;
+            device = s->node;
+            node_name = s->node;
+            new_image_file = NULL;
+            snapshot_ref = s->overlay;
+        }
+        break;
+    case TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC:
+        {
+            BlockdevSnapshotSync *s = action->u.blockdev_snapshot_sync;
+            device = s->has_device ? s->device : NULL;
+            node_name = s->has_node_name ? s->node_name : NULL;
+            new_image_file = s->snapshot_file;
+            snapshot_ref = NULL;
+        }
+        break;
+    default:
+        g_assert_not_reached();
     }
 
     /* start processing */
-    state->old_bs = bdrv_lookup_bs(has_device ? device : NULL,
-                                   has_node_name ? node_name : NULL,
-                                   &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        return;
-    }
-
-    if (has_node_name && !has_snapshot_node_name) {
-        error_setg(errp, "New snapshot node name missing");
-        return;
-    }
-
-    if (has_snapshot_node_name &&
-        bdrv_lookup_bs(snapshot_node_name, snapshot_node_name, NULL)) {
-        error_setg(errp, "New snapshot node name already in use");
+    state->old_bs = bdrv_lookup_bs(device, node_name, errp);
+    if (!state->old_bs) {
         return;
     }
 
@@ -1612,35 +1614,70 @@ static void external_snapshot_prepare(BlkTransactionState *common,
         return;
     }
 
-    flags = state->old_bs->open_flags;
+    if (action->type == TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC) {
+        BlockdevSnapshotSync *s = action->u.blockdev_snapshot_sync;
+        const char *format = s->has_format ? s->format : "qcow2";
+        enum NewImageMode mode;
+        const char *snapshot_node_name =
+            s->has_snapshot_node_name ? s->snapshot_node_name : NULL;
 
-    /* create new image w/backing file */
-    if (mode != NEW_IMAGE_MODE_EXISTING) {
-        bdrv_img_create(new_image_file, format,
-                        state->old_bs->filename,
-                        state->old_bs->drv->format_name,
-                        NULL, -1, flags, &local_err, false);
-        if (local_err) {
-            error_propagate(errp, local_err);
+        if (node_name && !snapshot_node_name) {
+            error_setg(errp, "New snapshot node name missing");
             return;
         }
-    }
 
-    options = qdict_new();
-    if (has_snapshot_node_name) {
-        qdict_put(options, "node-name",
-                  qstring_from_str(snapshot_node_name));
+        if (snapshot_node_name &&
+            bdrv_lookup_bs(snapshot_node_name, snapshot_node_name, NULL)) {
+            error_setg(errp, "New snapshot node name already in use");
+            return;
+        }
+
+        flags = state->old_bs->open_flags;
+
+        /* create new image w/backing file */
+        mode = s->has_mode ? s->mode : NEW_IMAGE_MODE_ABSOLUTE_PATHS;
+        if (mode != NEW_IMAGE_MODE_EXISTING) {
+            bdrv_img_create(new_image_file, format,
+                            state->old_bs->filename,
+                            state->old_bs->drv->format_name,
+                            NULL, -1, flags, &local_err, false);
+            if (local_err) {
+                error_propagate(errp, local_err);
+                return;
+            }
+        }
+
+        options = qdict_new();
+        if (s->has_snapshot_node_name) {
+            qdict_put(options, "node-name",
+                      qstring_from_str(snapshot_node_name));
+        }
+        qdict_put(options, "driver", qstring_from_str(format));
+
+        flags |= BDRV_O_NO_BACKING;
     }
-    qdict_put(options, "driver", qstring_from_str(format));
 
-    /* TODO Inherit bs->options or only take explicit options with an
-     * extended QMP command? */
     assert(state->new_bs == NULL);
-    ret = bdrv_open(&state->new_bs, new_image_file, NULL, options,
-                    flags | BDRV_O_NO_BACKING, &local_err);
+    ret = bdrv_open(&state->new_bs, new_image_file, snapshot_ref, options,
+                    flags, errp);
     /* We will manually add the backing_hd field to the bs later */
     if (ret != 0) {
-        error_propagate(errp, local_err);
+        return;
+    }
+
+    if (state->new_bs->blk != NULL) {
+        error_setg(errp, "The snapshot is already in use by %s",
+                   blk_name(state->new_bs->blk));
+        return;
+    }
+
+    if (bdrv_op_is_blocked(state->new_bs, BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT,
+                           errp)) {
+        return;
+    }
+
+    if (state->new_bs->backing != NULL) {
+        error_setg(errp, "The snapshot already has a backing image");
     }
 }
 
@@ -1843,6 +1880,12 @@ static void abort_commit(BlkTransactionState *common)
 }
 
 static const BdrvActionOps actions[] = {
+    [TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT] = {
+        .instance_size = sizeof(ExternalSnapshotState),
+        .prepare  = external_snapshot_prepare,
+        .commit   = external_snapshot_commit,
+        .abort = external_snapshot_abort,
+    },
     [TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC] = {
         .instance_size = sizeof(ExternalSnapshotState),
         .prepare  = external_snapshot_prepare,
diff --git a/qapi-schema.json b/qapi-schema.json
index 42694d2..1d29525 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1531,9 +1531,11 @@
 # abort since 1.6
 # blockdev-snapshot-internal-sync since 1.7
 # blockdev-backup since 2.3
+# blockdev-snapshot since 2.5
 ##
 { 'union': 'TransactionAction',
   'data': {
+       'blockdev-snapshot': 'BlockdevSnapshot',
        'blockdev-snapshot-sync': 'BlockdevSnapshotSync',
        'drive-backup': 'DriveBackup',
        'blockdev-backup': 'BlockdevBackup',
diff --git a/qapi/block-core.json b/qapi/block-core.json
index f592adc..083d2cd 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -705,6 +705,21 @@
             '*format': 'str', '*mode': 'NewImageMode' } }
 
 ##
+# @BlockdevSnapshot
+#
+# @node: device or node name that will have a snapshot created.
+#
+# @overlay: reference to the existing block device that will become
+#           the overlay of @node, as part of creating the snapshot.
+#           It must not have a current backing file (this can be
+#           achieved by passing "backing": "" to blockdev-add).
+#
+# Since 2.5
+##
+{ 'struct': 'BlockdevSnapshot',
+  'data': { 'node': 'str', 'overlay': 'str' } }
+
+##
 # @DriveBackup
 #
 # @device: the name of the device which should be copied.
@@ -800,6 +815,19 @@
 { 'command': 'blockdev-snapshot-sync',
   'data': 'BlockdevSnapshotSync' }
 
+
+##
+# @blockdev-snapshot
+#
+# Generates a snapshot of a block device.
+#
+# For the arguments, see the documentation of BlockdevSnapshot.
+#
+# Since 2.5
+##
+{ 'command': 'blockdev-snapshot',
+  'data': 'BlockdevSnapshot' }
+
 ##
 # @change-backing-file
 #
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 707e0bd..bd4d38f 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1476,6 +1476,44 @@ Example:
 EQMP
 
     {
+        .name       = "blockdev-snapshot",
+        .args_type  = "node:s,overlay:s",
+        .mhandler.cmd_new = qmp_marshal_blockdev_snapshot,
+    },
+
+SQMP
+blockdev-snapshot
+-----------------
+Since 2.5
+
+Create a snapshot, by installing 'node' as the backing image of
+'overlay'. Additionally, if 'node' is associated with a block
+device, the block device changes to using 'overlay' as its new active
+image.
+
+Arguments:
+
+- "node": device that will have a snapshot created (json-string)
+- "overlay": device that will have 'node' as its backing image (json-string)
+
+Example:
+
+-> { "execute": "blockdev-add",
+                "arguments": { "options": { "driver": "qcow2",
+                                            "node-name": "node1534",
+                                            "file": { "driver": "file",
+                                                      "filename": "hd1.qcow2" },
+                                            "backing": "" } } }
+
+<- { "return": {} }
+
+-> { "execute": "blockdev-snapshot", "arguments": { "node": "ide-hd0",
+                                                    "overlay": "node1534" } }
+<- { "return": {} }
+
+EQMP
+
+    {
         .name       = "blockdev-snapshot-internal-sync",
         .args_type  = "device:B,name:s",
         .mhandler.cmd_new = qmp_marshal_blockdev_snapshot_internal_sync,
-- 
1.8.3.1

[Qemu-devel] [PULL v2 21/40] block: add tests for the 'blockdev-snapshot' command

[Kevin Wolf]

From: Alberto Garcia <berto@igalia.com>

Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Jeff Cody <jcody@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/085     | 102 ++++++++++++++++++++++++++++++++++++++++++---
 tests/qemu-iotests/085.out |  34 ++++++++++++++-
 2 files changed, 128 insertions(+), 8 deletions(-)

diff --git a/tests/qemu-iotests/085 b/tests/qemu-iotests/085
index 56cd6f8..9484117 100755
--- a/tests/qemu-iotests/085
+++ b/tests/qemu-iotests/085
@@ -7,6 +7,7 @@
 # snapshots are performed.
 #
 # Copyright (C) 2014 Red Hat, Inc.
+# Copyright (C) 2015 Igalia, S.L.
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -34,17 +35,17 @@ status=1	# failure is the default!
 snapshot_virt0="snapshot-v0.qcow2"
 snapshot_virt1="snapshot-v1.qcow2"
 
-MAX_SNAPSHOTS=10
+SNAPSHOTS=10
 
 _cleanup()
 {
     _cleanup_qemu
-    for i in $(seq 1 ${MAX_SNAPSHOTS})
+    for i in $(seq 1 ${SNAPSHOTS})
     do
         rm -f "${TEST_DIR}/${i}-${snapshot_virt0}"
         rm -f "${TEST_DIR}/${i}-${snapshot_virt1}"
     done
-	_cleanup_test_img
+    rm -f "${TEST_IMG}.1" "${TEST_IMG}.2"
 
 }
 trap "_cleanup; exit \$status" 0 1 2 3 15
@@ -85,18 +86,50 @@ function create_group_snapshot()
     _send_qemu_cmd $h "${cmd}" "return"
 }
 
+# ${1}: unique identifier for the snapshot filename
+# ${2}: true: open backing images; false: don't open them (default)
+function add_snapshot_image()
+{
+    if [ "${2}" = "true" ]; then
+        extra_params=""
+    else
+        extra_params="'backing': '', "
+    fi
+    base_image="${TEST_DIR}/$((${1}-1))-${snapshot_virt0}"
+    snapshot_file="${TEST_DIR}/${1}-${snapshot_virt0}"
+    _make_test_img -b "${base_image}" "$size"
+    mv "${TEST_IMG}" "${snapshot_file}"
+    cmd="{ 'execute': 'blockdev-add', 'arguments':
+           { 'options':
+             { 'driver': 'qcow2', 'node-name': 'snap_"${1}"', "${extra_params}"
+               'file':
+               { 'driver': 'file', 'filename': '"${snapshot_file}"' } } } }"
+    _send_qemu_cmd $h "${cmd}" "return"
+}
+
+# ${1}: unique identifier for the snapshot filename
+# ${2}: expected response, defaults to 'return'
+function blockdev_snapshot()
+{
+    cmd="{ 'execute': 'blockdev-snapshot',
+                      'arguments': { 'node': 'virtio0',
+                                     'overlay':'snap_"${1}"' } }"
+    _send_qemu_cmd $h "${cmd}" "${2:-return}"
+}
+
 size=128M
 
 _make_test_img $size
-mv "${TEST_IMG}" "${TEST_IMG}.orig"
+mv "${TEST_IMG}" "${TEST_IMG}.1"
 _make_test_img $size
+mv "${TEST_IMG}" "${TEST_IMG}.2"
 
 echo
 echo === Running QEMU ===
 echo
 
 qemu_comm_method="qmp"
-_launch_qemu -drive file="${TEST_IMG}.orig",if=virtio -drive file="${TEST_IMG}",if=virtio
+_launch_qemu -drive file="${TEST_IMG}.1",if=virtio -drive file="${TEST_IMG}.2",if=virtio
 h=$QEMU_HANDLE
 
 echo
@@ -105,6 +138,8 @@ echo
 
 _send_qemu_cmd $h "{ 'execute': 'qmp_capabilities' }" "return"
 
+# Tests for the blockdev-snapshot-sync command
+
 echo
 echo === Create a single snapshot on virtio0 ===
 echo
@@ -132,11 +167,66 @@ echo
 echo === Create several transactional group snapshots ===
 echo
 
-for i in $(seq 2 ${MAX_SNAPSHOTS})
+for i in $(seq 2 ${SNAPSHOTS})
 do
     create_group_snapshot ${i}
 done
 
+# Tests for the blockdev-snapshot command
+
+echo
+echo === Create a couple of snapshots using blockdev-snapshot ===
+echo
+
+SNAPSHOTS=$((${SNAPSHOTS}+1))
+add_snapshot_image ${SNAPSHOTS}
+blockdev_snapshot ${SNAPSHOTS}
+
+SNAPSHOTS=$((${SNAPSHOTS}+1))
+add_snapshot_image ${SNAPSHOTS}
+blockdev_snapshot ${SNAPSHOTS}
+
+echo
+echo === Invalid command - snapshot node used as active layer ===
+echo
+
+blockdev_snapshot ${SNAPSHOTS} error
+
+_send_qemu_cmd $h "{ 'execute': 'blockdev-snapshot',
+                     'arguments': { 'node':'virtio0',
+                                    'overlay':'virtio0' }
+                   }" "error"
+
+_send_qemu_cmd $h "{ 'execute': 'blockdev-snapshot',
+                     'arguments': { 'node':'virtio0',
+                                    'overlay':'virtio1' }
+                   }" "error"
+
+echo
+echo === Invalid command - snapshot node used as backing hd ===
+echo
+
+blockdev_snapshot $((${SNAPSHOTS}-1)) error
+
+echo
+echo === Invalid command - snapshot node has a backing image ===
+echo
+
+SNAPSHOTS=$((${SNAPSHOTS}+1))
+add_snapshot_image ${SNAPSHOTS} true
+blockdev_snapshot ${SNAPSHOTS} error
+
+echo
+echo === Invalid command - The node does not exist ===
+echo
+
+blockdev_snapshot $((${SNAPSHOTS}+1)) error
+
+_send_qemu_cmd $h "{ 'execute': 'blockdev-snapshot',
+                     'arguments': { 'node':'nodevice',
+                                    'overlay':'snap_"${SNAPSHOTS}"' }
+                   }" "error"
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/085.out b/tests/qemu-iotests/085.out
index a6cf19e..52292ea 100644
--- a/tests/qemu-iotests/085.out
+++ b/tests/qemu-iotests/085.out
@@ -11,7 +11,7 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
 
 === Create a single snapshot on virtio0 ===
 
-Formatting 'TEST_DIR/1-snapshot-v0.qcow2', fmt=qcow2 size=134217728 backing_file=TEST_DIR/t.qcow2.orig backing_fmt=qcow2 encryption=off cluster_size=65536 lazy_refcounts=off refcount_bits=16
+Formatting 'TEST_DIR/1-snapshot-v0.qcow2', fmt=qcow2 size=134217728 backing_file=TEST_DIR/t.qcow2.1 backing_fmt=qcow2 encryption=off cluster_size=65536 lazy_refcounts=off refcount_bits=16
 {"return": {}}
 
 === Invalid command - missing device and nodename ===
@@ -26,7 +26,7 @@ Formatting 'TEST_DIR/1-snapshot-v0.qcow2', fmt=qcow2 size=134217728 backing_file
 === Create several transactional group snapshots ===
 
 Formatting 'TEST_DIR/2-snapshot-v0.qcow2', fmt=qcow2 size=134217728 backing_file=TEST_DIR/1-snapshot-v0.qcow2 backing_fmt=qcow2 encryption=off cluster_size=65536 lazy_refcounts=off refcount_bits=16
-Formatting 'TEST_DIR/2-snapshot-v1.qcow2', fmt=qcow2 size=134217728 backing_file=TEST_DIR/t.qcow2 backing_fmt=qcow2 encryption=off cluster_size=65536 lazy_refcounts=off refcount_bits=16
+Formatting 'TEST_DIR/2-snapshot-v1.qcow2', fmt=qcow2 size=134217728 backing_file=TEST_DIR/t.qcow2.2 backing_fmt=qcow2 encryption=off cluster_size=65536 lazy_refcounts=off refcount_bits=16
 {"return": {}}
 Formatting 'TEST_DIR/3-snapshot-v0.qcow2', fmt=qcow2 size=134217728 backing_file=TEST_DIR/2-snapshot-v0.qcow2 backing_fmt=qcow2 encryption=off cluster_size=65536 lazy_refcounts=off refcount_bits=16
 Formatting 'TEST_DIR/3-snapshot-v1.qcow2', fmt=qcow2 size=134217728 backing_file=TEST_DIR/2-snapshot-v1.qcow2 backing_fmt=qcow2 encryption=off cluster_size=65536 lazy_refcounts=off refcount_bits=16
@@ -52,4 +52,34 @@ Formatting 'TEST_DIR/9-snapshot-v1.qcow2', fmt=qcow2 size=134217728 backing_file
 Formatting 'TEST_DIR/10-snapshot-v0.qcow2', fmt=qcow2 size=134217728 backing_file=TEST_DIR/9-snapshot-v0.qcow2 backing_fmt=qcow2 encryption=off cluster_size=65536 lazy_refcounts=off refcount_bits=16
 Formatting 'TEST_DIR/10-snapshot-v1.qcow2', fmt=qcow2 size=134217728 backing_file=TEST_DIR/9-snapshot-v1.qcow2 backing_fmt=qcow2 encryption=off cluster_size=65536 lazy_refcounts=off refcount_bits=16
 {"return": {}}
+
+=== Create a couple of snapshots using blockdev-snapshot ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 backing_file=TEST_DIR/10-snapshot-v0.IMGFMT
+{"return": {}}
+{"return": {}}
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 backing_file=TEST_DIR/11-snapshot-v0.IMGFMT
+{"return": {}}
+{"return": {}}
+
+=== Invalid command - snapshot node used as active layer ===
+
+{"error": {"class": "GenericError", "desc": "The snapshot is already in use by virtio0"}}
+{"error": {"class": "GenericError", "desc": "The snapshot is already in use by virtio0"}}
+{"error": {"class": "GenericError", "desc": "The snapshot is already in use by virtio1"}}
+
+=== Invalid command - snapshot node used as backing hd ===
+
+{"error": {"class": "GenericError", "desc": "Node 'snap_11' is busy: node is used as backing hd of 'virtio0'"}}
+
+=== Invalid command - snapshot node has a backing image ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 backing_file=TEST_DIR/12-snapshot-v0.IMGFMT
+{"return": {}}
+{"error": {"class": "GenericError", "desc": "The snapshot already has a backing image"}}
+
+=== Invalid command - The node does not exist ===
+
+{"error": {"class": "GenericError", "desc": "Cannot find device=snap_14 nor node_name=snap_14"}}
+{"error": {"class": "GenericError", "desc": "Cannot find device=nodevice nor node_name=nodevice"}}
 *** done
-- 
1.8.3.1

[Qemu-devel] [PULL v2 22/40] commit: reopen overlay_bs before base

[Kevin Wolf]

From: Alberto Garcia <berto@igalia.com>

'block-commit' needs write access to two different nodes of the chain:

- 'base', because that's where the data is written to.
- the overlay of 'top', because it needs to update the backing file
  string to point to 'base' after the operation.

Both images have to be opened in read-write mode, and commit_start()
takes care of reopening them if necessary.

With the current implementation, however, when overlay_bs is reopened
in read-write mode it has the side effect of making 'base' read-only
again, eventually making 'block-commit' fail.

This needs to be fixed in bdrv_reopen(), but until we get to that it
can be worked around simply by swapping the order of base and
overlay_bs in the reopen queue.

In order to reproduce this bug, overlay_bs needs to be initially in
read-only mode. That is: the 'top' parameter of 'block-commit' cannot
be the active layer nor its immediate backing chain.

Cc: qemu-stable@nongnu.org
Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/commit.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/block/commit.c b/block/commit.c
index fdebe87..a5d02aa 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -236,14 +236,14 @@ void commit_start(BlockDriverState *bs, BlockDriverState *base,
     orig_overlay_flags = bdrv_get_flags(overlay_bs);
 
     /* convert base & overlay_bs to r/w, if necessary */
-    if (!(orig_base_flags & BDRV_O_RDWR)) {
-        reopen_queue = bdrv_reopen_queue(reopen_queue, base, NULL,
-                                         orig_base_flags | BDRV_O_RDWR);
-    }
     if (!(orig_overlay_flags & BDRV_O_RDWR)) {
         reopen_queue = bdrv_reopen_queue(reopen_queue, overlay_bs, NULL,
                                          orig_overlay_flags | BDRV_O_RDWR);
     }
+    if (!(orig_base_flags & BDRV_O_RDWR)) {
+        reopen_queue = bdrv_reopen_queue(reopen_queue, base, NULL,
+                                         orig_base_flags | BDRV_O_RDWR);
+    }
     if (reopen_queue) {
         bdrv_reopen_multiple(reopen_queue, &local_err);
         if (local_err != NULL) {
-- 
1.8.3.1

[Qemu-devel] [PULL v2 23/40] qemu-iotests: Test the reopening of overlay_bs in 'block-commit'

[Kevin Wolf]

From: Alberto Garcia <berto@igalia.com>

The 'block-commit' command needs the overlay image of 'top' to
be opened in read-write mode in order to update the backing file
string. If 'top' is not the active layer or its backing file then its
overlay needs to be reopened during the block job.

This is a test case for that scenario.

Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/040     | 30 ++++++++++++++++++++++++++++++
 tests/qemu-iotests/040.out |  4 ++--
 2 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
index ea2f98e..5bdaf3d 100755
--- a/tests/qemu-iotests/040
+++ b/tests/qemu-iotests/040
@@ -41,6 +41,7 @@ class ImageCommitTestCase(iotests.QMPTestCase):
         while not completed:
             for event in self.vm.get_qmp_events(wait=True):
                 if event['event'] == 'BLOCK_JOB_COMPLETED':
+                    self.assert_qmp_absent(event, 'data/error')
                     self.assert_qmp(event, 'data/type', 'commit')
                     self.assert_qmp(event, 'data/device', 'drive0')
                     self.assert_qmp(event, 'data/offset', event['data']['len'])
@@ -251,5 +252,34 @@ class TestSetSpeed(ImageCommitTestCase):
 class TestActiveZeroLengthImage(TestSingleDrive):
     image_len = 0
 
+class TestReopenOverlay(ImageCommitTestCase):
+    image_len = 1024 * 1024
+    img0 = os.path.join(iotests.test_dir, '0.img')
+    img1 = os.path.join(iotests.test_dir, '1.img')
+    img2 = os.path.join(iotests.test_dir, '2.img')
+    img3 = os.path.join(iotests.test_dir, '3.img')
+
+    def setUp(self):
+        iotests.create_image(self.img0, self.image_len)
+        qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % self.img0, self.img1)
+        qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % self.img1, self.img2)
+        qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % self.img2, self.img3)
+        qemu_io('-f', iotests.imgfmt, '-c', 'write -P 0xab 0 128K', self.img1)
+        self.vm = iotests.VM().add_drive(self.img3)
+        self.vm.launch()
+
+    def tearDown(self):
+        self.vm.shutdown()
+        os.remove(self.img0)
+        os.remove(self.img1)
+        os.remove(self.img2)
+        os.remove(self.img3)
+
+    # This tests what happens when the overlay image of the 'top' node
+    # needs to be reopened in read-write mode in order to update the
+    # backing image string.
+    def test_reopen_overlay(self):
+        self.run_commit_test(self.img1, self.img0)
+
 if __name__ == '__main__':
     iotests.main(supported_fmts=['qcow2', 'qed'])
diff --git a/tests/qemu-iotests/040.out b/tests/qemu-iotests/040.out
index 42314e9..4fd1c2d 100644
--- a/tests/qemu-iotests/040.out
+++ b/tests/qemu-iotests/040.out
@@ -1,5 +1,5 @@
-........................
+.........................
 ----------------------------------------------------------------------
-Ran 24 tests
+Ran 25 tests
 
 OK
-- 
1.8.3.1

[Qemu-devel] [PULL v2 24/40] qcow2: avoid misaligned 64bit bswap

[Kevin Wolf]

From: John Snow <jsnow@redhat.com>

If we create a buffer directly on the stack by using 12 bytes, there's
no guarantee the 64bit value we want to swap will be aligned, which
could cause errors with undefined behavior.

Spotted with clang -fsanitize=undefined and observed in iotests 15, 26,
44, 115 and 121.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2-refcount.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 4b81c8d..6e0e5bd 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -560,13 +560,16 @@ static int alloc_refcount_block(BlockDriverState *bs,
     }
 
     /* Hook up the new refcount table in the qcow2 header */
-    uint8_t data[12];
-    cpu_to_be64w((uint64_t*)data, table_offset);
-    cpu_to_be32w((uint32_t*)(data + 8), table_clusters);
+    struct QEMU_PACKED {
+        uint64_t d64;
+        uint32_t d32;
+    } data;
+    cpu_to_be64w(&data.d64, table_offset);
+    cpu_to_be32w(&data.d32, table_clusters);
     BLKDBG_EVENT(bs->file, BLKDBG_REFBLOCK_ALLOC_SWITCH_TABLE);
     ret = bdrv_pwrite_sync(bs->file->bs,
                            offsetof(QCowHeader, refcount_table_offset),
-                           data, sizeof(data));
+                           &data, sizeof(data));
     if (ret < 0) {
         goto fail_table;
     }
-- 
1.8.3.1

[Qemu-devel] [PULL v2 25/40] qemu-img: add check for zero-length job len

[Kevin Wolf]

From: John Snow <jsnow@redhat.com>

The mirror job doesn't update its total length until
it has already started running, so we should translate
a zero-length job-len as meaning 0%.

Otherwise, we may get divide-by-zero faults.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Jeff Cody <jcody@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qemu-img.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/qemu-img.c b/qemu-img.c
index 3025776..9831db7 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -656,7 +656,8 @@ static void run_block_job(BlockJob *job, Error **errp)
 
     do {
         aio_poll(aio_context, true);
-        qemu_progress_print((float)job->offset / job->len * 100.f, 0);
+        qemu_progress_print(job->len ?
+                            ((float)job->offset / job->len * 100.f) : 0.0f, 0);
     } while (!job->ready);
 
     block_job_complete_sync(job, errp);
-- 
1.8.3.1

[Qemu-devel] [PULL v2 26/40] throttle: Check for pending requests in throttle_group_unregister_bs()

[Kevin Wolf]

From: Alberto Garcia <berto@igalia.com>

throttle_group_unregister_bs() removes a BlockDriverState from its
throttling group and destroys the timers. This means that there must
be no pending throttled requests at that point (because it would be
impossible to complete them), so the caller has to drain them first.

At the moment throttle_group_unregister_bs() is only called from
bdrv_io_limits_disable(), which already takes care of draining the
requests, so there's nothing to worry about, but this patch makes
this invariant explicit in the documentation and adds the relevant
assertions.

Signed-off-by: Alberto Garcia <berto@igalia.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/throttle-groups.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/block/throttle-groups.c b/block/throttle-groups.c
index 3419af7..13b5baa 100644
--- a/block/throttle-groups.c
+++ b/block/throttle-groups.c
@@ -437,6 +437,9 @@ void throttle_group_register_bs(BlockDriverState *bs, const char *groupname)
  * list, destroying the timers and setting the throttle_state pointer
  * to NULL.
  *
+ * The BlockDriverState must not have pending throttled requests, so
+ * the caller has to drain them first.
+ *
  * The group will be destroyed if it's empty after this operation.
  *
  * @bs: the BlockDriverState to remove
@@ -446,6 +449,10 @@ void throttle_group_unregister_bs(BlockDriverState *bs)
     ThrottleGroup *tg = container_of(bs->throttle_state, ThrottleGroup, ts);
     int i;
 
+    assert(bs->pending_reqs[0] == 0 && bs->pending_reqs[1] == 0);
+    assert(qemu_co_queue_empty(&bs->throttled_reqs[0]));
+    assert(qemu_co_queue_empty(&bs->throttled_reqs[1]));
+
     qemu_mutex_lock(&tg->lock);
     for (i = 0; i < 2; i++) {
         if (tg->tokens[i] == bs) {
-- 
1.8.3.1

[Qemu-devel] [PULL v2 27/40] throttle: Use bs->throttle_state instead of bs->io_limits_enabled

[Kevin Wolf]

From: Alberto Garcia <berto@igalia.com>

There are two ways to check for I/O limits in a BlockDriverState:

- bs->throttle_state: if this pointer is not NULL, it means that this
  BDS is member of a throttling group, its ThrottleTimers structure
  has been initialized and its I/O limits are ready to be applied.

- bs->io_limits_enabled: if true it means that the throttle_state
  pointer is valid _and_ the limits are currently enabled.

The latter is used in several places to check whether a BDS has I/O
limits configured, but what it really checks is whether requests
are being throttled or not. For example, io_limits_enabled can be
temporarily set to false in cases like bdrv_read_unthrottled() without
otherwise touching the throtting configuration of that BDS.

This patch replaces bs->io_limits_enabled with bs->throttle_state in
all cases where what we really want to check is the existence of I/O
limits, not whether they are currently enabled or not.

Signed-off-by: Alberto Garcia <berto@igalia.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c                   | 6 +++---
 block/qapi.c              | 2 +-
 blockdev.c                | 4 ++--
 include/block/block_int.h | 5 ++++-
 4 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/block.c b/block.c
index 3493501..cffac75 100644
--- a/block.c
+++ b/block.c
@@ -1907,7 +1907,7 @@ void bdrv_close(BlockDriverState *bs)
     }
 
     /* Disable I/O limits and drain all pending throttled requests */
-    if (bs->io_limits_enabled) {
+    if (bs->throttle_state) {
         bdrv_io_limits_disable(bs);
     }
 
@@ -3712,7 +3712,7 @@ void bdrv_detach_aio_context(BlockDriverState *bs)
         baf->detach_aio_context(baf->opaque);
     }
 
-    if (bs->io_limits_enabled) {
+    if (bs->throttle_state) {
         throttle_timers_detach_aio_context(&bs->throttle_timers);
     }
     if (bs->drv->bdrv_detach_aio_context) {
@@ -3748,7 +3748,7 @@ void bdrv_attach_aio_context(BlockDriverState *bs,
     if (bs->drv->bdrv_attach_aio_context) {
         bs->drv->bdrv_attach_aio_context(bs, new_context);
     }
-    if (bs->io_limits_enabled) {
+    if (bs->throttle_state) {
         throttle_timers_attach_aio_context(&bs->throttle_timers, new_context);
     }
 
diff --git a/block/qapi.c b/block/qapi.c
index ec0f513..89d4274 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -64,7 +64,7 @@ BlockDeviceInfo *bdrv_block_device_info(BlockDriverState *bs, Error **errp)
     info->backing_file_depth = bdrv_get_backing_file_depth(bs);
     info->detect_zeroes = bs->detect_zeroes;
 
-    if (bs->io_limits_enabled) {
+    if (bs->throttle_state) {
         ThrottleConfig cfg;
 
         throttle_group_get_config(bs, &cfg);
diff --git a/blockdev.c b/blockdev.c
index 7645d49..3598b01 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2361,14 +2361,14 @@ void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd,
     if (throttle_enabled(&cfg)) {
         /* Enable I/O limits if they're not enabled yet, otherwise
          * just update the throttling group. */
-        if (!bs->io_limits_enabled) {
+        if (!bs->throttle_state) {
             bdrv_io_limits_enable(bs, has_group ? group : device);
         } else if (has_group) {
             bdrv_io_limits_update_group(bs, group);
         }
         /* Set the new throttling configuration */
         bdrv_set_io_limits(bs, &cfg);
-    } else if (bs->io_limits_enabled) {
+    } else if (bs->throttle_state) {
         /* If all throttling settings are set to 0, disable I/O limits */
         bdrv_io_limits_disable(bs);
     }
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 6a3f64d..603145a 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -390,7 +390,10 @@ struct BlockDriverState {
     /* number of in-flight serialising requests */
     unsigned int serialising_in_flight;
 
-    /* I/O throttling */
+    /* I/O throttling.
+     * throttle_state tells us if this BDS has I/O limits configured.
+     * io_limits_enabled tells us if they are currently being
+     * enforced, but it can be temporarily set to false */
     CoQueue      throttled_reqs[2];
     bool         io_limits_enabled;
     /* The following fields are protected by the ThrottleGroup lock.
-- 
1.8.3.1

[Qemu-devel] [PULL v2 28/40] block: Disallow snapshots if the overlay doesn't support backing files

[Kevin Wolf]

From: Alberto Garcia <berto@igalia.com>

This addresses scenarios like this one:

  { 'execute': 'blockdev-add', 'arguments':
    { 'options': { 'driver': 'qcow2',
                   'node-name': 'new0',
                   'file': { 'driver': 'file',
                             'filename': 'new.qcow2',
                             'node-name': 'file0' } } } }

  { 'execute': 'blockdev-snapshot', 'arguments':
    { 'node': 'virtio0',
      'overlay': 'file0' } }

Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 blockdev.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index 3598b01..3197791 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1678,6 +1678,11 @@ static void external_snapshot_prepare(BlkTransactionState *common,
 
     if (state->new_bs->backing != NULL) {
         error_setg(errp, "The snapshot already has a backing image");
+        return;
+    }
+
+    if (!state->new_bs->drv->supports_backing) {
+        error_setg(errp, "The snapshot does not support backing images");
     }
 }
 
-- 
1.8.3.1

[Qemu-devel] [PULL v2 29/40] block: Remove inner quotation marks in iotest 085

[Kevin Wolf]

From: Alberto Garcia <berto@igalia.com>

This patch removes the inner quotation marks in all cases like this:

   cmd=" ... "${variable}" ... "

Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/085 | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/tests/qemu-iotests/085 b/tests/qemu-iotests/085
index 9484117..80e547d 100755
--- a/tests/qemu-iotests/085
+++ b/tests/qemu-iotests/085
@@ -65,7 +65,7 @@ function create_single_snapshot()
 {
     cmd="{ 'execute': 'blockdev-snapshot-sync',
                       'arguments': { 'device': 'virtio0',
-                                     'snapshot-file':'"${TEST_DIR}/${1}-${snapshot_virt0}"',
+                                     'snapshot-file':'${TEST_DIR}/${1}-${snapshot_virt0}',
                                      'format': 'qcow2' } }"
     _send_qemu_cmd $h "${cmd}" "return"
 }
@@ -77,10 +77,10 @@ function create_group_snapshot()
            {'actions': [
                { 'type': 'blockdev-snapshot-sync', 'data' :
                    { 'device': 'virtio0',
-                      'snapshot-file': '"${TEST_DIR}/${1}-${snapshot_virt0}"' } },
+                      'snapshot-file': '${TEST_DIR}/${1}-${snapshot_virt0}' } },
                { 'type': 'blockdev-snapshot-sync', 'data' :
                    { 'device': 'virtio1',
-                       'snapshot-file': '"${TEST_DIR}/${1}-${snapshot_virt1}"' } } ]
+                       'snapshot-file': '${TEST_DIR}/${1}-${snapshot_virt1}' } } ]
              } }"
 
     _send_qemu_cmd $h "${cmd}" "return"
@@ -101,9 +101,9 @@ function add_snapshot_image()
     mv "${TEST_IMG}" "${snapshot_file}"
     cmd="{ 'execute': 'blockdev-add', 'arguments':
            { 'options':
-             { 'driver': 'qcow2', 'node-name': 'snap_"${1}"', "${extra_params}"
+             { 'driver': 'qcow2', 'node-name': 'snap_${1}', ${extra_params}
                'file':
-               { 'driver': 'file', 'filename': '"${snapshot_file}"' } } } }"
+               { 'driver': 'file', 'filename': '${snapshot_file}' } } } }"
     _send_qemu_cmd $h "${cmd}" "return"
 }
 
@@ -113,7 +113,7 @@ function blockdev_snapshot()
 {
     cmd="{ 'execute': 'blockdev-snapshot',
                       'arguments': { 'node': 'virtio0',
-                                     'overlay':'snap_"${1}"' } }"
+                                     'overlay':'snap_${1}' } }"
     _send_qemu_cmd $h "${cmd}" "${2:-return}"
 }
 
@@ -152,7 +152,7 @@ echo === Invalid command - missing device and nodename ===
 echo
 
 _send_qemu_cmd $h "{ 'execute': 'blockdev-snapshot-sync',
-                         'arguments': { 'snapshot-file':'"${TEST_DIR}/1-${snapshot_virt0}"',
+                         'arguments': { 'snapshot-file':'${TEST_DIR}/1-${snapshot_virt0}',
                                      'format': 'qcow2' } }" "error"
 
 echo
@@ -224,7 +224,7 @@ blockdev_snapshot $((${SNAPSHOTS}+1)) error
 
 _send_qemu_cmd $h "{ 'execute': 'blockdev-snapshot',
                      'arguments': { 'node':'nodevice',
-                                    'overlay':'snap_"${SNAPSHOTS}"' }
+                                    'overlay':'snap_${SNAPSHOTS}' }
                    }" "error"
 
 # success, all done
-- 
1.8.3.1

[Qemu-devel] [PULL v2 30/40] block: test 'blockdev-snapshot' using a file BDS as the overlay

[Kevin Wolf]

From: Alberto Garcia <berto@igalia.com>

This test checks that it is not possible to create a snapshot if the
requested overlay node is a BDS which does not support backing images.

Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/085     | 12 +++++++++++-
 tests/qemu-iotests/085.out |  4 ++++
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/085 b/tests/qemu-iotests/085
index 80e547d..aa77eca 100755
--- a/tests/qemu-iotests/085
+++ b/tests/qemu-iotests/085
@@ -103,7 +103,8 @@ function add_snapshot_image()
            { 'options':
              { 'driver': 'qcow2', 'node-name': 'snap_${1}', ${extra_params}
                'file':
-               { 'driver': 'file', 'filename': '${snapshot_file}' } } } }"
+               { 'driver': 'file', 'filename': '${snapshot_file}',
+                 'node-name': 'file_${1}' } } } }"
     _send_qemu_cmd $h "${cmd}" "return"
 }
 
@@ -187,6 +188,15 @@ add_snapshot_image ${SNAPSHOTS}
 blockdev_snapshot ${SNAPSHOTS}
 
 echo
+echo === Invalid command - cannot create a snapshot using a file BDS ===
+echo
+
+_send_qemu_cmd $h "{ 'execute': 'blockdev-snapshot',
+                     'arguments': { 'node':'virtio0',
+                                    'overlay':'file_${SNAPSHOTS}' }
+                   }" "error"
+
+echo
 echo === Invalid command - snapshot node used as active layer ===
 echo
 
diff --git a/tests/qemu-iotests/085.out b/tests/qemu-iotests/085.out
index 52292ea..01c78d6 100644
--- a/tests/qemu-iotests/085.out
+++ b/tests/qemu-iotests/085.out
@@ -62,6 +62,10 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 backing_file=TEST_DIR/
 {"return": {}}
 {"return": {}}
 
+=== Invalid command - cannot create a snapshot using a file BDS ===
+
+{"error": {"class": "GenericError", "desc": "The snapshot does not support backing images"}}
+
 === Invalid command - snapshot node used as active layer ===
 
 {"error": {"class": "GenericError", "desc": "The snapshot is already in use by virtio0"}}
-- 
1.8.3.1

[Qemu-devel] [PULL v2 31/40] qemu-io: fix cvtnum lval types

[Kevin Wolf]

From: John Snow <jsnow@redhat.com>

cvtnum() returns int64_t: we should not be storing this
result inside of an int.

In a few cases, we need an extra sprinkling of error handling
where we expect to pass this number on towards a function that
expects something smaller than int64_t.

Reported-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qemu-io-cmds.c | 123 ++++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 87 insertions(+), 36 deletions(-)

diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index 6e5d1e4..20605f2 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -294,9 +294,10 @@ static void qemu_io_free(void *p)
     qemu_vfree(p);
 }
 
-static void dump_buffer(const void *buffer, int64_t offset, int len)
+static void dump_buffer(const void *buffer, int64_t offset, int64_t len)
 {
-    int i, j;
+    uint64_t i;
+    int j;
     const uint8_t *p;
 
     for (i = 0, p = buffer; i < len; i += 16) {
@@ -319,7 +320,7 @@ static void dump_buffer(const void *buffer, int64_t offset, int len)
 }
 
 static void print_report(const char *op, struct timeval *t, int64_t offset,
-                         int count, int total, int cnt, int Cflag)
+                         int64_t count, int64_t total, int cnt, int Cflag)
 {
     char s1[64], s2[64], ts[64];
 
@@ -327,12 +328,12 @@ static void print_report(const char *op, struct timeval *t, int64_t offset,
     if (!Cflag) {
         cvtstr((double)total, s1, sizeof(s1));
         cvtstr(tdiv((double)total, *t), s2, sizeof(s2));
-        printf("%s %d/%d bytes at offset %" PRId64 "\n",
+        printf("%s %"PRId64"/%"PRId64" bytes at offset %" PRId64 "\n",
                op, total, count, offset);
         printf("%s, %d ops; %s (%s/sec and %.4f ops/sec)\n",
                s1, cnt, ts, s2, tdiv((double)cnt, *t));
     } else {/* bytes,ops,time,bytes/sec,ops/sec */
-        printf("%d,%d,%s,%.3f,%.3f\n",
+        printf("%"PRId64",%d,%s,%.3f,%.3f\n",
             total, cnt, ts,
             tdiv((double)total, *t),
             tdiv((double)cnt, *t));
@@ -393,11 +394,15 @@ fail:
     return buf;
 }
 
-static int do_read(BlockBackend *blk, char *buf, int64_t offset, int count,
-                   int *total)
+static int do_read(BlockBackend *blk, char *buf, int64_t offset, int64_t count,
+                   int64_t *total)
 {
     int ret;
 
+    if (count >> 9 > INT_MAX) {
+        return -ERANGE;
+    }
+
     ret = blk_read(blk, offset >> 9, (uint8_t *)buf, count >> 9);
     if (ret < 0) {
         return ret;
@@ -406,11 +411,15 @@ static int do_read(BlockBackend *blk, char *buf, int64_t offset, int count,
     return 1;
 }
 
-static int do_write(BlockBackend *blk, char *buf, int64_t offset, int count,
-                    int *total)
+static int do_write(BlockBackend *blk, char *buf, int64_t offset, int64_t count,
+                    int64_t *total)
 {
     int ret;
 
+    if (count >> 9 > INT_MAX) {
+        return -ERANGE;
+    }
+
     ret = blk_write(blk, offset >> 9, (uint8_t *)buf, count >> 9);
     if (ret < 0) {
         return ret;
@@ -419,9 +428,13 @@ static int do_write(BlockBackend *blk, char *buf, int64_t offset, int count,
     return 1;
 }
 
-static int do_pread(BlockBackend *blk, char *buf, int64_t offset, int count,
-                    int *total)
+static int do_pread(BlockBackend *blk, char *buf, int64_t offset,
+                    int64_t count, int64_t *total)
 {
+    if (count > INT_MAX) {
+        return -ERANGE;
+    }
+
     *total = blk_pread(blk, offset, (uint8_t *)buf, count);
     if (*total < 0) {
         return *total;
@@ -429,9 +442,13 @@ static int do_pread(BlockBackend *blk, char *buf, int64_t offset, int count,
     return 1;
 }
 
-static int do_pwrite(BlockBackend *blk, char *buf, int64_t offset, int count,
-                     int *total)
+static int do_pwrite(BlockBackend *blk, char *buf, int64_t offset,
+                     int64_t count, int64_t *total)
 {
+    if (count > INT_MAX) {
+        return -ERANGE;
+    }
+
     *total = blk_pwrite(blk, offset, (uint8_t *)buf, count);
     if (*total < 0) {
         return *total;
@@ -442,8 +459,8 @@ static int do_pwrite(BlockBackend *blk, char *buf, int64_t offset, int count,
 typedef struct {
     BlockBackend *blk;
     int64_t offset;
-    int count;
-    int *total;
+    int64_t count;
+    int64_t *total;
     int ret;
     bool done;
 } CoWriteZeroes;
@@ -463,8 +480,8 @@ static void coroutine_fn co_write_zeroes_entry(void *opaque)
     *data->total = data->count;
 }
 
-static int do_co_write_zeroes(BlockBackend *blk, int64_t offset, int count,
-                              int *total)
+static int do_co_write_zeroes(BlockBackend *blk, int64_t offset, int64_t count,
+                              int64_t *total)
 {
     Coroutine *co;
     CoWriteZeroes data = {
@@ -475,6 +492,10 @@ static int do_co_write_zeroes(BlockBackend *blk, int64_t offset, int count,
         .done   = false,
     };
 
+    if (count >> BDRV_SECTOR_BITS > INT_MAX) {
+        return -ERANGE;
+    }
+
     co = qemu_coroutine_create(co_write_zeroes_entry);
     qemu_coroutine_enter(co, &data);
     while (!data.done) {
@@ -488,10 +509,14 @@ static int do_co_write_zeroes(BlockBackend *blk, int64_t offset, int count,
 }
 
 static int do_write_compressed(BlockBackend *blk, char *buf, int64_t offset,
-                               int count, int *total)
+                               int64_t count, int64_t *total)
 {
     int ret;
 
+    if (count >> 9 > INT_MAX) {
+        return -ERANGE;
+    }
+
     ret = blk_write_compressed(blk, offset >> 9, (uint8_t *)buf, count >> 9);
     if (ret < 0) {
         return ret;
@@ -501,8 +526,12 @@ static int do_write_compressed(BlockBackend *blk, char *buf, int64_t offset,
 }
 
 static int do_load_vmstate(BlockBackend *blk, char *buf, int64_t offset,
-                           int count, int *total)
+                           int64_t count, int64_t *total)
 {
+    if (count > INT_MAX) {
+        return -ERANGE;
+    }
+
     *total = blk_load_vmstate(blk, (uint8_t *)buf, offset, count);
     if (*total < 0) {
         return *total;
@@ -511,8 +540,12 @@ static int do_load_vmstate(BlockBackend *blk, char *buf, int64_t offset,
 }
 
 static int do_save_vmstate(BlockBackend *blk, char *buf, int64_t offset,
-                           int count, int *total)
+                           int64_t count, int64_t *total)
 {
+    if (count > INT_MAX) {
+        return -ERANGE;
+    }
+
     *total = blk_save_vmstate(blk, (uint8_t *)buf, offset, count);
     if (*total < 0) {
         return *total;
@@ -642,10 +675,11 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
     int c, cnt;
     char *buf;
     int64_t offset;
-    int count;
+    int64_t count;
     /* Some compilers get confused and warn if this is not initialized.  */
-    int total = 0;
-    int pattern = 0, pattern_offset = 0, pattern_count = 0;
+    int64_t total = 0;
+    int pattern = 0;
+    int64_t pattern_offset = 0, pattern_count = 0;
 
     while ((c = getopt(argc, argv, "bCl:pP:qs:v")) != -1) {
         switch (c) {
@@ -712,6 +746,9 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
     if (count < 0) {
         printf("non-numeric length argument -- %s\n", argv[optind]);
         return 0;
+    } else if (count > SIZE_MAX) {
+        printf("length cannot exceed %zu, given %s\n", SIZE_MAX, argv[optind]);
+        return 0;
     }
 
     if (!Pflag && (lflag || sflag)) {
@@ -734,7 +771,7 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
             return 0;
         }
         if (count & 0x1ff) {
-            printf("count %d is not sector aligned\n",
+            printf("count %"PRId64" is not sector aligned\n",
                    count);
             return 0;
         }
@@ -762,7 +799,7 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
         memset(cmp_buf, pattern, pattern_count);
         if (memcmp(buf + pattern_offset, cmp_buf, pattern_count)) {
             printf("Pattern verification failed at offset %"
-                   PRId64 ", %d bytes\n",
+                   PRId64 ", %"PRId64" bytes\n",
                    offset + pattern_offset, pattern_count);
         }
         g_free(cmp_buf);
@@ -957,9 +994,9 @@ static int write_f(BlockBackend *blk, int argc, char **argv)
     int c, cnt;
     char *buf = NULL;
     int64_t offset;
-    int count;
+    int64_t count;
     /* Some compilers get confused and warn if this is not initialized.  */
-    int total = 0;
+    int64_t total = 0;
     int pattern = 0xcd;
 
     while ((c = getopt(argc, argv, "bcCpP:qz")) != -1) {
@@ -1019,6 +1056,9 @@ static int write_f(BlockBackend *blk, int argc, char **argv)
     if (count < 0) {
         printf("non-numeric length argument -- %s\n", argv[optind]);
         return 0;
+    } else if (count > SIZE_MAX) {
+        printf("length cannot exceed %zu, given %s\n", SIZE_MAX, argv[optind]);
+        return 0;
     }
 
     if (!pflag) {
@@ -1029,7 +1069,7 @@ static int write_f(BlockBackend *blk, int argc, char **argv)
         }
 
         if (count & 0x1ff) {
-            printf("count %d is not sector aligned\n",
+            printf("count %"PRId64" is not sector aligned\n",
                    count);
             return 0;
         }
@@ -1777,8 +1817,7 @@ static int discard_f(BlockBackend *blk, int argc, char **argv)
     struct timeval t1, t2;
     int Cflag = 0, qflag = 0;
     int c, ret;
-    int64_t offset;
-    int count;
+    int64_t offset, count;
 
     while ((c = getopt(argc, argv, "Cq")) != -1) {
         switch (c) {
@@ -1808,6 +1847,11 @@ static int discard_f(BlockBackend *blk, int argc, char **argv)
     if (count < 0) {
         printf("non-numeric length argument -- %s\n", argv[optind]);
         return 0;
+    } else if (count >> BDRV_SECTOR_BITS > INT_MAX) {
+        printf("length cannot exceed %"PRIu64", given %s\n",
+               (uint64_t)INT_MAX << BDRV_SECTOR_BITS,
+               argv[optind]);
+        return 0;
     }
 
     gettimeofday(&t1, NULL);
@@ -1833,11 +1877,10 @@ out:
 static int alloc_f(BlockBackend *blk, int argc, char **argv)
 {
     BlockDriverState *bs = blk_bs(blk);
-    int64_t offset, sector_num;
-    int nb_sectors, remaining;
+    int64_t offset, sector_num, nb_sectors, remaining;
     char s1[64];
-    int num, sum_alloc;
-    int ret;
+    int num, ret;
+    int64_t sum_alloc;
 
     offset = cvtnum(argv[1]);
     if (offset < 0) {
@@ -1854,6 +1897,10 @@ static int alloc_f(BlockBackend *blk, int argc, char **argv)
         if (nb_sectors < 0) {
             printf("non-numeric length argument -- %s\n", argv[2]);
             return 0;
+        } else if (nb_sectors > INT_MAX) {
+            printf("length argument cannot exceed %d, given %s\n",
+                   INT_MAX, argv[2]);
+            return 0;
         }
     } else {
         nb_sectors = 1;
@@ -1881,7 +1928,7 @@ static int alloc_f(BlockBackend *blk, int argc, char **argv)
 
     cvtstr(offset, s1, sizeof(s1));
 
-    printf("%d/%d sectors allocated at offset %s\n",
+    printf("%"PRId64"/%"PRId64" sectors allocated at offset %s\n",
            sum_alloc, nb_sectors, s1);
     return 0;
 }
@@ -2191,10 +2238,14 @@ static const cmdinfo_t sigraise_cmd = {
 
 static int sigraise_f(BlockBackend *blk, int argc, char **argv)
 {
-    int sig = cvtnum(argv[1]);
+    int64_t sig = cvtnum(argv[1]);
     if (sig < 0) {
         printf("non-numeric signal number argument -- %s\n", argv[1]);
         return 0;
+    } else if (sig > NSIG) {
+        printf("signal argument '%s' is too large to be a valid signal\n",
+               argv[1]);
+        return 0;
     }
 
     /* Using raise() to kill this process does not necessarily flush all open
-- 
1.8.3.1

[Qemu-devel] [PULL v2 32/40] qemu-io: Check for trailing chars

[Kevin Wolf]

From: John Snow <jsnow@redhat.com>

Make sure there's not trailing garbage, e.g.
"64k-whatever-i-want-here"

Reported-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qemu-io-cmds.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index 20605f2..238b1da 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -136,7 +136,14 @@ static char **breakline(char *input, int *count)
 static int64_t cvtnum(const char *s)
 {
     char *end;
-    return qemu_strtosz_suffix(s, &end, QEMU_STRTOSZ_DEFSUFFIX_B);
+    int64_t ret;
+
+    ret = qemu_strtosz_suffix(s, &end, QEMU_STRTOSZ_DEFSUFFIX_B);
+    if (*end != '\0') {
+        /* Detritus at the end of the string */
+        return -EINVAL;
+    }
+    return ret;
 }
 
 #define EXABYTES(x)     ((long long)(x) << 60)
-- 
1.8.3.1

[Qemu-devel] [PULL v2 33/40] qemu-io: Correct error messages

[Kevin Wolf]

From: John Snow <jsnow@redhat.com>

Reported-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qemu-io-cmds.c | 53 ++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 34 insertions(+), 19 deletions(-)

diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index 238b1da..3dddae8 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -146,6 +146,21 @@ static int64_t cvtnum(const char *s)
     return ret;
 }
 
+static void print_cvtnum_err(int64_t rc, const char *arg)
+{
+    switch (rc) {
+    case -EINVAL:
+        printf("Parsing error: non-numeric argument,"
+               " or extraneous/unrecognized suffix -- %s\n", arg);
+        break;
+    case -ERANGE:
+        printf("Parsing error: argument too large -- %s\n", arg);
+        break;
+    default:
+        printf("Parsing error: %s\n", arg);
+    }
+}
+
 #define EXABYTES(x)     ((long long)(x) << 60)
 #define PETABYTES(x)    ((long long)(x) << 50)
 #define TERABYTES(x)    ((long long)(x) << 40)
@@ -367,13 +382,13 @@ create_iovec(BlockBackend *blk, QEMUIOVector *qiov, char **argv, int nr_iov,
 
         len = cvtnum(arg);
         if (len < 0) {
-            printf("non-numeric length argument -- %s\n", arg);
+            print_cvtnum_err(len, arg);
             goto fail;
         }
 
         /* should be SIZE_T_MAX, but that doesn't exist */
         if (len > INT_MAX) {
-            printf("too large length argument -- %s\n", arg);
+            printf("Argument '%s' exceeds maximum size %d\n", arg, INT_MAX);
             goto fail;
         }
 
@@ -700,7 +715,7 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
             lflag = 1;
             pattern_count = cvtnum(optarg);
             if (pattern_count < 0) {
-                printf("non-numeric length argument -- %s\n", optarg);
+                print_cvtnum_err(pattern_count, optarg);
                 return 0;
             }
             break;
@@ -721,7 +736,7 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
             sflag = 1;
             pattern_offset = cvtnum(optarg);
             if (pattern_offset < 0) {
-                printf("non-numeric length argument -- %s\n", optarg);
+                print_cvtnum_err(pattern_offset, optarg);
                 return 0;
             }
             break;
@@ -744,14 +759,14 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
 
     offset = cvtnum(argv[optind]);
     if (offset < 0) {
-        printf("non-numeric length argument -- %s\n", argv[optind]);
+        print_cvtnum_err(offset, argv[optind]);
         return 0;
     }
 
     optind++;
     count = cvtnum(argv[optind]);
     if (count < 0) {
-        printf("non-numeric length argument -- %s\n", argv[optind]);
+        print_cvtnum_err(count, argv[optind]);
         return 0;
     } else if (count > SIZE_MAX) {
         printf("length cannot exceed %zu, given %s\n", SIZE_MAX, argv[optind]);
@@ -905,7 +920,7 @@ static int readv_f(BlockBackend *blk, int argc, char **argv)
 
     offset = cvtnum(argv[optind]);
     if (offset < 0) {
-        printf("non-numeric length argument -- %s\n", argv[optind]);
+        print_cvtnum_err(offset, argv[optind]);
         return 0;
     }
     optind++;
@@ -1054,14 +1069,14 @@ static int write_f(BlockBackend *blk, int argc, char **argv)
 
     offset = cvtnum(argv[optind]);
     if (offset < 0) {
-        printf("non-numeric length argument -- %s\n", argv[optind]);
+        print_cvtnum_err(offset, argv[optind]);
         return 0;
     }
 
     optind++;
     count = cvtnum(argv[optind]);
     if (count < 0) {
-        printf("non-numeric length argument -- %s\n", argv[optind]);
+        print_cvtnum_err(count, argv[optind]);
         return 0;
     } else if (count > SIZE_MAX) {
         printf("length cannot exceed %zu, given %s\n", SIZE_MAX, argv[optind]);
@@ -1189,7 +1204,7 @@ static int writev_f(BlockBackend *blk, int argc, char **argv)
 
     offset = cvtnum(argv[optind]);
     if (offset < 0) {
-        printf("non-numeric length argument -- %s\n", argv[optind]);
+        print_cvtnum_err(offset, argv[optind]);
         return 0;
     }
     optind++;
@@ -1316,7 +1331,7 @@ static int multiwrite_f(BlockBackend *blk, int argc, char **argv)
         /* Read the offset of the request */
         offset = cvtnum(argv[optind]);
         if (offset < 0) {
-            printf("non-numeric offset argument -- %s\n", argv[optind]);
+            print_cvtnum_err(offset, argv[optind]);
             goto out;
         }
         optind++;
@@ -1543,7 +1558,7 @@ static int aio_read_f(BlockBackend *blk, int argc, char **argv)
 
     ctx->offset = cvtnum(argv[optind]);
     if (ctx->offset < 0) {
-        printf("non-numeric length argument -- %s\n", argv[optind]);
+        print_cvtnum_err(ctx->offset, argv[optind]);
         g_free(ctx);
         return 0;
     }
@@ -1638,7 +1653,7 @@ static int aio_write_f(BlockBackend *blk, int argc, char **argv)
 
     ctx->offset = cvtnum(argv[optind]);
     if (ctx->offset < 0) {
-        printf("non-numeric length argument -- %s\n", argv[optind]);
+        print_cvtnum_err(ctx->offset, argv[optind]);
         g_free(ctx);
         return 0;
     }
@@ -1698,7 +1713,7 @@ static int truncate_f(BlockBackend *blk, int argc, char **argv)
 
     offset = cvtnum(argv[1]);
     if (offset < 0) {
-        printf("non-numeric truncate argument -- %s\n", argv[1]);
+        print_cvtnum_err(offset, argv[1]);
         return 0;
     }
 
@@ -1845,14 +1860,14 @@ static int discard_f(BlockBackend *blk, int argc, char **argv)
 
     offset = cvtnum(argv[optind]);
     if (offset < 0) {
-        printf("non-numeric length argument -- %s\n", argv[optind]);
+        print_cvtnum_err(offset, argv[optind]);
         return 0;
     }
 
     optind++;
     count = cvtnum(argv[optind]);
     if (count < 0) {
-        printf("non-numeric length argument -- %s\n", argv[optind]);
+        print_cvtnum_err(count, argv[optind]);
         return 0;
     } else if (count >> BDRV_SECTOR_BITS > INT_MAX) {
         printf("length cannot exceed %"PRIu64", given %s\n",
@@ -1891,7 +1906,7 @@ static int alloc_f(BlockBackend *blk, int argc, char **argv)
 
     offset = cvtnum(argv[1]);
     if (offset < 0) {
-        printf("non-numeric offset argument -- %s\n", argv[1]);
+        print_cvtnum_err(offset, argv[1]);
         return 0;
     } else if (offset & 0x1ff) {
         printf("offset %" PRId64 " is not sector aligned\n",
@@ -1902,7 +1917,7 @@ static int alloc_f(BlockBackend *blk, int argc, char **argv)
     if (argc == 3) {
         nb_sectors = cvtnum(argv[2]);
         if (nb_sectors < 0) {
-            printf("non-numeric length argument -- %s\n", argv[2]);
+            print_cvtnum_err(nb_sectors, argv[2]);
             return 0;
         } else if (nb_sectors > INT_MAX) {
             printf("length argument cannot exceed %d, given %s\n",
@@ -2247,7 +2262,7 @@ static int sigraise_f(BlockBackend *blk, int argc, char **argv)
 {
     int64_t sig = cvtnum(argv[1]);
     if (sig < 0) {
-        printf("non-numeric signal number argument -- %s\n", argv[1]);
+        print_cvtnum_err(sig, argv[1]);
         return 0;
     } else if (sig > NSIG) {
         printf("signal argument '%s' is too large to be a valid signal\n",
-- 
1.8.3.1

[Qemu-devel] [PULL v2 34/40] qemu-iotests: fix cleanup of background processes

[Kevin Wolf]

From: Jeff Cody <jcody@redhat.com>

Commit 934659c switched the iotests to run qemu and qemu-nbd from a bash
subshell, in order to catch segfaults.  Unfortunately, this means the
process PID cannot be captured via '$!'. We stopped killing qemu and
qemu-nbd processes, leaving a lot of orphaned, running qemu processes
after executing iotests.

Since the process is using exec in the subshell, the PID is the
same as the subshell PID.

Track these PIDs for cleanup using pidfiles in the $TEST_DIR. Only
track the qemu PID, however, if requested - not all usage requires
killing the process.

Reported-by: John Snow <jsnow@redhat.com>
Signed-off-by: Jeff Cody <jcody@redhat.com>
Message-id: 9e4f958b3895b7259b98d845bb46f000ba362869.1446232490.git.jcody@redhat.com
[mreitz@redhat.com: Replaced '! -z "..."' by '-n "..."']
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/058           | 12 ++++++++----
 tests/qemu-iotests/common.config | 14 ++++++++++++--
 tests/qemu-iotests/common.qemu   | 18 ++++++++++++------
 tests/qemu-iotests/common.rc     |  8 +++++---
 4 files changed, 37 insertions(+), 15 deletions(-)

diff --git a/tests/qemu-iotests/058 b/tests/qemu-iotests/058
index f2bdd0b..63a6598 100755
--- a/tests/qemu-iotests/058
+++ b/tests/qemu-iotests/058
@@ -32,11 +32,17 @@ status=1	# failure is the default!
 
 nbd_unix_socket=$TEST_DIR/test_qemu_nbd_socket
 nbd_snapshot_img="nbd:unix:$nbd_unix_socket"
+rm -f "${TEST_DIR}/qemu-nbd.pid"
 
 _cleanup_nbd()
 {
-    if [ -n "$NBD_SNAPSHOT_PID" ]; then
-        kill "$NBD_SNAPSHOT_PID"
+    local NBD_SNAPSHOT_PID
+    if [ -f "${TEST_DIR}/qemu-nbd.pid" ]; then
+        read NBD_SNAPSHOT_PID < "${TEST_DIR}/qemu-nbd.pid"
+        rm -f "${TEST_DIR}/qemu-nbd.pid"
+        if [ -n "$NBD_SNAPSHOT_PID" ]; then
+            kill "$NBD_SNAPSHOT_PID"
+        fi
     fi
     rm -f "$nbd_unix_socket"
 }
@@ -60,7 +66,6 @@ _export_nbd_snapshot()
 {
     _cleanup_nbd
     $QEMU_NBD -v -t -k "$nbd_unix_socket" "$TEST_IMG" -l $1 &
-    NBD_SNAPSHOT_PID=$!
     _wait_for_nbd
 }
 
@@ -68,7 +73,6 @@ _export_nbd_snapshot1()
 {
     _cleanup_nbd
     $QEMU_NBD -v -t -k "$nbd_unix_socket" "$TEST_IMG" -l snapshot.name=$1 &
-    NBD_SNAPSHOT_PID=$!
     _wait_for_nbd
 }
 
diff --git a/tests/qemu-iotests/common.config b/tests/qemu-iotests/common.config
index 596bb2b..be99730 100644
--- a/tests/qemu-iotests/common.config
+++ b/tests/qemu-iotests/common.config
@@ -44,6 +44,8 @@ export HOST_OPTIONS=${HOST_OPTIONS:=local.config}
 export CHECK_OPTIONS=${CHECK_OPTIONS:="-g auto"}
 export PWD=`pwd`
 
+export _QEMU_HANDLE=0
+
 # $1 = prog to look for, $2* = default pathnames if not found in $PATH
 set_prog_path()
 {
@@ -105,7 +107,12 @@ fi
 
 _qemu_wrapper()
 {
-    (exec "$QEMU_PROG" $QEMU_OPTIONS "$@")
+    (
+        if [ -n "${QEMU_NEED_PID}" ]; then
+            echo $BASHPID > "${TEST_DIR}/qemu-${_QEMU_HANDLE}.pid"
+        fi
+        exec "$QEMU_PROG" $QEMU_OPTIONS "$@"
+    )
 }
 
 _qemu_img_wrapper()
@@ -120,7 +127,10 @@ _qemu_io_wrapper()
 
 _qemu_nbd_wrapper()
 {
-    (exec "$QEMU_NBD_PROG" $QEMU_NBD_OPTIONS "$@")
+    (
+        echo $BASHPID > "${TEST_DIR}/qemu-nbd.pid"
+        exec "$QEMU_NBD_PROG" $QEMU_NBD_OPTIONS "$@"
+    )
 }
 
 export QEMU=_qemu_wrapper
diff --git a/tests/qemu-iotests/common.qemu b/tests/qemu-iotests/common.qemu
index e3faa53..8bf3969 100644
--- a/tests/qemu-iotests/common.qemu
+++ b/tests/qemu-iotests/common.qemu
@@ -30,8 +30,6 @@ QEMU_COMM_TIMEOUT=10
 QEMU_FIFO_IN="${TEST_DIR}/qmp-in-$$"
 QEMU_FIFO_OUT="${TEST_DIR}/qmp-out-$$"
 
-QEMU_PID=
-_QEMU_HANDLE=0
 QEMU_HANDLE=0
 
 # If bash version is >= 4.1, these will be overwritten and dynamic
@@ -153,11 +151,11 @@ function _launch_qemu()
     mkfifo "${fifo_out}"
     mkfifo "${fifo_in}"
 
+    QEMU_NEED_PID='y'\
     ${QEMU} -nographic -serial none ${comm} -machine accel=qtest "${@}" \
                                                                 >"${fifo_out}" \
                                                                 2>&1 \
                                                                 <"${fifo_in}" &
-    QEMU_PID[${_QEMU_HANDLE}]=$!
 
     if [[ "${BASH_VERSINFO[0]}" -ge "5" ||
         ("${BASH_VERSINFO[0]}" -ge "4"  &&  "${BASH_VERSINFO[1]}" -ge "1") ]]
@@ -196,10 +194,18 @@ function _cleanup_qemu()
     # QEMU_PID[], QEMU_IN[], QEMU_OUT[] all use same indices
     for i in "${!QEMU_OUT[@]}"
     do
-        if [ -z "${wait}" ]; then
-            kill -KILL ${QEMU_PID[$i]} 2>/dev/null
+        local QEMU_PID
+        if [ -f "${TEST_DIR}/qemu-${i}.pid" ]; then
+            read QEMU_PID < "${TEST_DIR}/qemu-${i}.pid"
+            rm -f "${TEST_DIR}/qemu-${i}.pid"
+            if [ -z "${wait}" ] && [ -n "${QEMU_PID}" ]; then
+                kill -KILL ${QEMU_PID} 2>/dev/null
+            fi
+            if [ -n "${QEMU_PID}" ]; then
+                wait ${QEMU_PID} 2>/dev/null # silent kill
+            fi
         fi
-        wait ${QEMU_PID[$i]} 2>/dev/null # silent kill
+
         if [ -n "${wait}" ]; then
             cat <&${QEMU_OUT[$i]} | _filter_testdir | _filter_qemu \
                                   | _filter_qemu_io | _filter_qmp
diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 28e4bea..4878e99 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -154,7 +154,6 @@ _make_test_img()
     # Start an NBD server on the image file, which is what we'll be talking to
     if [ $IMGPROTO = "nbd" ]; then
         eval "$QEMU_NBD -v -t -b 127.0.0.1 -p 10810 -f $IMGFMT  $TEST_IMG_FILE &"
-        QEMU_NBD_PID=$!
         sleep 1 # FIXME: qemu-nbd needs to be listening before we continue
     fi
 }
@@ -175,8 +174,11 @@ _cleanup_test_img()
     case "$IMGPROTO" in
 
         nbd)
-            if [ -n "$QEMU_NBD_PID" ]; then
-                kill $QEMU_NBD_PID
+            if [ -f "${TEST_DIR}/qemu-nbd.pid" ]; then
+                local QEMU_NBD_PID
+                read QEMU_NBD_PID < "${TEST_DIR}/qemu-nbd.pid"
+                kill ${QEMU_NBD_PID}
+                rm -f "${TEST_DIR}/qemu-nbd.pid"
             fi
             rm -f "$TEST_IMG_FILE"
             ;;
-- 
1.8.3.1

[Qemu-devel] [PULL v2 35/40] qemu-iotests: fix -valgrind option for check

[Kevin Wolf]

From: Jeff Cody <jcody@redhat.com>

Commit 934659c switched the iotests to run qemu-io from a bash subshell,
in order to catch segfaults.  This method is incompatible with the
current valgrind_qemu_io() bash function.

Move the valgrind usage into the exec subshell in _qemu_io_wrapper(),
while making sure the original return value is passed back to the
caller.

Update test output for tests 039, 061, and 137 as it looks for the
specific subshell command when the process is terminated.

Reported-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Jeff Cody <jcody@redhat.com>
Message-id: 0066fd85d26ca641a1c25135ff2479b7985701cf.1446232490.git.jcody@redhat.com
Reviewed-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/039.out       | 30 +++++++++++++++++++++++++-----
 tests/qemu-iotests/061.out       | 12 ++++++++++--
 tests/qemu-iotests/137.out       |  6 +++++-
 tests/qemu-iotests/common        |  9 ++-------
 tests/qemu-iotests/common.config | 18 +++++++++++++++++-
 tests/qemu-iotests/common.rc     | 10 ----------
 6 files changed, 59 insertions(+), 26 deletions(-)

diff --git a/tests/qemu-iotests/039.out b/tests/qemu-iotests/039.out
index 03a31c5..32c8846 100644
--- a/tests/qemu-iotests/039.out
+++ b/tests/qemu-iotests/039.out
@@ -11,7 +11,11 @@ No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
 wrote 512/512 bytes at offset 0
 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-./common.config: Killed                  ( exec "$QEMU_IO_PROG" $QEMU_IO_OPTIONS "$@" )
+./common.config: Killed                  ( if [ "${VALGRIND_QEMU}" == "y" ]; then
+    exec valgrind --log-file="${VALGRIND_LOGFILE}" --error-exitcode=99 "$QEMU_IO_PROG" $QEMU_IO_OPTIONS "$@";
+else
+    exec "$QEMU_IO_PROG" $QEMU_IO_OPTIONS "$@";
+fi )
 incompatible_features     0x1
 ERROR cluster 5 refcount=0 reference=1
 ERROR OFLAG_COPIED data cluster: l2_entry=8000000000050000 refcount=0
@@ -46,7 +50,11 @@ read 512/512 bytes at offset 0
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
 wrote 512/512 bytes at offset 0
 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-./common.config: Killed                  ( exec "$QEMU_IO_PROG" $QEMU_IO_OPTIONS "$@" )
+./common.config: Killed                  ( if [ "${VALGRIND_QEMU}" == "y" ]; then
+    exec valgrind --log-file="${VALGRIND_LOGFILE}" --error-exitcode=99 "$QEMU_IO_PROG" $QEMU_IO_OPTIONS "$@";
+else
+    exec "$QEMU_IO_PROG" $QEMU_IO_OPTIONS "$@";
+fi )
 incompatible_features     0x1
 ERROR cluster 5 refcount=0 reference=1
 Rebuilding refcount structure
@@ -60,7 +68,11 @@ incompatible_features     0x0
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
 wrote 512/512 bytes at offset 0
 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-./common.config: Killed                  ( exec "$QEMU_IO_PROG" $QEMU_IO_OPTIONS "$@" )
+./common.config: Killed                  ( if [ "${VALGRIND_QEMU}" == "y" ]; then
+    exec valgrind --log-file="${VALGRIND_LOGFILE}" --error-exitcode=99 "$QEMU_IO_PROG" $QEMU_IO_OPTIONS "$@";
+else
+    exec "$QEMU_IO_PROG" $QEMU_IO_OPTIONS "$@";
+fi )
 incompatible_features     0x0
 No errors were found on the image.
 
@@ -79,7 +91,11 @@ No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
 wrote 512/512 bytes at offset 0
 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-./common.config: Killed                  ( exec "$QEMU_IO_PROG" $QEMU_IO_OPTIONS "$@" )
+./common.config: Killed                  ( if [ "${VALGRIND_QEMU}" == "y" ]; then
+    exec valgrind --log-file="${VALGRIND_LOGFILE}" --error-exitcode=99 "$QEMU_IO_PROG" $QEMU_IO_OPTIONS "$@";
+else
+    exec "$QEMU_IO_PROG" $QEMU_IO_OPTIONS "$@";
+fi )
 incompatible_features     0x1
 ERROR cluster 5 refcount=0 reference=1
 ERROR OFLAG_COPIED data cluster: l2_entry=8000000000050000 refcount=0
@@ -89,7 +105,11 @@ Data may be corrupted, or further writes to the image may corrupt it.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
 wrote 512/512 bytes at offset 0
 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-./common.config: Killed                  ( exec "$QEMU_IO_PROG" $QEMU_IO_OPTIONS "$@" )
+./common.config: Killed                  ( if [ "${VALGRIND_QEMU}" == "y" ]; then
+    exec valgrind --log-file="${VALGRIND_LOGFILE}" --error-exitcode=99 "$QEMU_IO_PROG" $QEMU_IO_OPTIONS "$@";
+else
+    exec "$QEMU_IO_PROG" $QEMU_IO_OPTIONS "$@";
+fi )
 incompatible_features     0x0
 No errors were found on the image.
 *** done
diff --git a/tests/qemu-iotests/061.out b/tests/qemu-iotests/061.out
index b16bea9..f2598a8 100644
--- a/tests/qemu-iotests/061.out
+++ b/tests/qemu-iotests/061.out
@@ -57,7 +57,11 @@ No errors were found on the image.
 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)
-./common.config: Killed                  ( exec "$QEMU_IO_PROG" $QEMU_IO_OPTIONS "$@" )
+./common.config: Killed                  ( if [ "${VALGRIND_QEMU}" == "y" ]; then
+    exec valgrind --log-file="${VALGRIND_LOGFILE}" --error-exitcode=99 "$QEMU_IO_PROG" $QEMU_IO_OPTIONS "$@";
+else
+    exec "$QEMU_IO_PROG" $QEMU_IO_OPTIONS "$@";
+fi )
 magic                     0x514649fb
 version                   3
 backing_file_offset       0x0
@@ -215,7 +219,11 @@ No errors were found on the image.
 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)
-./common.config: Killed                  ( exec "$QEMU_IO_PROG" $QEMU_IO_OPTIONS "$@" )
+./common.config: Killed                  ( if [ "${VALGRIND_QEMU}" == "y" ]; then
+    exec valgrind --log-file="${VALGRIND_LOGFILE}" --error-exitcode=99 "$QEMU_IO_PROG" $QEMU_IO_OPTIONS "$@";
+else
+    exec "$QEMU_IO_PROG" $QEMU_IO_OPTIONS "$@";
+fi )
 magic                     0x514649fb
 version                   3
 backing_file_offset       0x0
diff --git a/tests/qemu-iotests/137.out b/tests/qemu-iotests/137.out
index cf55a41..88c702c 100644
--- a/tests/qemu-iotests/137.out
+++ b/tests/qemu-iotests/137.out
@@ -31,7 +31,11 @@ Cache clean interval too big
 Unsupported value 'blubb' for qcow2 option 'overlap-check'. Allowed are any of the following: none, constant, cached, all
 wrote 512/512 bytes at offset 0
 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-./common.config: Killed                  ( exec "$QEMU_IO_PROG" $QEMU_IO_OPTIONS "$@" )
+./common.config: Killed                  ( if [ "${VALGRIND_QEMU}" == "y" ]; then
+    exec valgrind --log-file="${VALGRIND_LOGFILE}" --error-exitcode=99 "$QEMU_IO_PROG" $QEMU_IO_OPTIONS "$@";
+else
+    exec "$QEMU_IO_PROG" $QEMU_IO_OPTIONS "$@";
+fi )
 incompatible_features     0x0
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
 wrote 65536/65536 bytes at offset 0
diff --git a/tests/qemu-iotests/common b/tests/qemu-iotests/common
index 25c351b..ff84f4b 100644
--- a/tests/qemu-iotests/common
+++ b/tests/qemu-iotests/common
@@ -41,7 +41,6 @@ sortme=false
 expunge=true
 have_test_arg=false
 randomize=false
-valgrind=false
 cachemode=false
 rm -f $tmp.list $tmp.tmp $tmp.sed
 
@@ -53,6 +52,7 @@ export CACHEMODE="writeback"
 export QEMU_IO_OPTIONS=""
 export CACHEMODE_IS_DEFAULT=true
 export QEMU_OPTIONS="-nodefaults"
+export VALGRIND_QEMU=
 
 for r
 do
@@ -278,7 +278,7 @@ testlist options
             ;;
 
         -valgrind)
-            valgrind=true
+            VALGRIND_QEMU='y'
             xpand=false
             ;;
 
@@ -436,8 +436,3 @@ fi
 if [ "$IMGPROTO" = "nbd" ] ; then
     [ "$QEMU_NBD" = "" ] && _fatal "qemu-nbd not found"
 fi
-
-if $valgrind; then
-    export REAL_QEMU_IO="$QEMU_IO_PROG"
-    export QEMU_IO_PROG=valgrind_qemu_io
-fi
diff --git a/tests/qemu-iotests/common.config b/tests/qemu-iotests/common.config
index be99730..3ed51b8 100644
--- a/tests/qemu-iotests/common.config
+++ b/tests/qemu-iotests/common.config
@@ -122,7 +122,23 @@ _qemu_img_wrapper()
 
 _qemu_io_wrapper()
 {
-    (exec "$QEMU_IO_PROG" $QEMU_IO_OPTIONS "$@")
+    local VALGRIND_LOGFILE=/tmp/$$.valgrind
+    local RETVAL
+    (
+        if [ "${VALGRIND_QEMU}" == "y" ]; then
+            exec valgrind --log-file="${VALGRIND_LOGFILE}" --error-exitcode=99 "$QEMU_IO_PROG" $QEMU_IO_OPTIONS "$@"
+        else
+            exec "$QEMU_IO_PROG" $QEMU_IO_OPTIONS "$@"
+        fi
+    )
+    RETVAL=$?
+    if [ "${VALGRIND_QEMU}" == "y" ]; then
+        if [ $RETVAL == 99 ]; then
+            cat "${VALGRIND_LOGFILE}"
+        fi
+        rm -f "${VALGRIND_LOGFILE}"
+    fi
+    (exit $RETVAL)
 }
 
 _qemu_nbd_wrapper()
diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 4878e99..d9913f8 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -70,16 +70,6 @@ else
     TEST_IMG=$IMGPROTO:$TEST_DIR/t.$IMGFMT
 fi
 
-function valgrind_qemu_io()
-{
-    valgrind --log-file=/tmp/$$.valgrind --error-exitcode=99 $REAL_QEMU_IO "$@"
-    if [ $? != 0 ]; then
-        cat /tmp/$$.valgrind
-    fi
-    rm -f /tmp/$$.valgrind
-}
-
-
 _optstr_add()
 {
     if [ -n "$1" ]; then
-- 
1.8.3.1

[Qemu-devel] [PULL v2 36/40] mirror: block all operations on the target image during the job

[Kevin Wolf]

From: Alberto Garcia <berto@igalia.com>

There's nothing preventing the target image from being used by other
operations during the 'drive-mirror' job, so we should block them all
until the job is done.

Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Message-id: 82b88fd04cde918a08a6f9a4ab85626d7fd7b502.1446475331.git.berto@igalia.com
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/mirror.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/block/mirror.c b/block/mirror.c
index b1252a1..60f1cb5 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -384,6 +384,7 @@ static void mirror_exit(BlockJob *job, void *opaque)
         aio_context_release(replace_aio_context);
     }
     g_free(s->replaces);
+    bdrv_op_unblock_all(s->target, s->common.blocker);
     bdrv_unref(s->target);
     block_job_completed(&s->common, data->ret);
     g_free(data);
@@ -744,6 +745,9 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
         block_job_release(bs);
         return;
     }
+
+    bdrv_op_block_all(s->target, s->common.blocker);
+
     bdrv_set_enable_write_cache(s->target, true);
     if (s->target->blk) {
         blk_set_on_error(s->target->blk, on_target_error, on_target_error);
-- 
1.8.3.1

[Qemu-devel] [PULL v2 37/40] block: Add blk_get_refcnt()

[Kevin Wolf]

From: Alberto Garcia <berto@igalia.com>

This function returns the reference count of a given BlockBackend.
For convenience, it returns 0 if the BlockBackend pointer is NULL.

Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Message-id: dfdd8a17dbe3288842840636d2cfe5bb895abcb0.1446475331.git.berto@igalia.com
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/block-backend.c          | 5 +++++
 include/sysemu/block-backend.h | 1 +
 2 files changed, 6 insertions(+)

diff --git a/block/block-backend.c b/block/block-backend.c
index 1ac6982..6f9309f 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -189,6 +189,11 @@ static void drive_info_del(DriveInfo *dinfo)
     g_free(dinfo);
 }
 
+int blk_get_refcnt(BlockBackend *blk)
+{
+    return blk ? blk->refcnt : 0;
+}
+
 /*
  * Increment @blk's reference count.
  * @blk must not be null.
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 40e315b..f4a68e2 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -65,6 +65,7 @@ BlockBackend *blk_new_with_bs(const char *name, Error **errp);
 BlockBackend *blk_new_open(const char *name, const char *filename,
                            const char *reference, QDict *options, int flags,
                            Error **errp);
+int blk_get_refcnt(BlockBackend *blk);
 void blk_ref(BlockBackend *blk);
 void blk_unref(BlockBackend *blk);
 const char *blk_name(BlockBackend *blk);
-- 
1.8.3.1

[Qemu-devel] [PULL v2 38/40] block: Add 'x-blockdev-del' QMP command

[Kevin Wolf]

From: Alberto Garcia <berto@igalia.com>

This command is still experimental, hence the name.

This is the companion to 'blockdev-add'. It allows deleting a
BlockBackend with its associated BlockDriverState tree, or a
BlockDriverState that is not attached to any backend.

In either case, the command fails if the reference count is greater
than 1 or the BlockDriverState has any parents.

Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Message-id: 6cfc148c77aca1da942b094d811bfa3fcf7ac7bb.1446475331.git.berto@igalia.com
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 blockdev.c           | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi/block-core.json | 32 +++++++++++++++++++++++--
 qmp-commands.hx      | 61 ++++++++++++++++++++++++++++++++++++++++++++++--
 3 files changed, 155 insertions(+), 4 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 3197791..8607df9 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3479,6 +3479,72 @@ fail:
     qmp_output_visitor_cleanup(ov);
 }
 
+void qmp_x_blockdev_del(bool has_id, const char *id,
+                        bool has_node_name, const char *node_name, Error **errp)
+{
+    AioContext *aio_context;
+    BlockBackend *blk;
+    BlockDriverState *bs;
+
+    if (has_id && has_node_name) {
+        error_setg(errp, "Only one of id and node-name must be specified");
+        return;
+    } else if (!has_id && !has_node_name) {
+        error_setg(errp, "No block device specified");
+        return;
+    }
+
+    if (has_id) {
+        blk = blk_by_name(id);
+        if (!blk) {
+            error_setg(errp, "Cannot find block backend %s", id);
+            return;
+        }
+        if (blk_get_refcnt(blk) > 1) {
+            error_setg(errp, "Block backend %s is in use", id);
+            return;
+        }
+        bs = blk_bs(blk);
+        aio_context = blk_get_aio_context(blk);
+    } else {
+        bs = bdrv_find_node(node_name);
+        if (!bs) {
+            error_setg(errp, "Cannot find node %s", node_name);
+            return;
+        }
+        blk = bs->blk;
+        if (blk) {
+            error_setg(errp, "Node %s is in use by %s",
+                       node_name, blk_name(blk));
+            return;
+        }
+        aio_context = bdrv_get_aio_context(bs);
+    }
+
+    aio_context_acquire(aio_context);
+
+    if (bs) {
+        if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_DRIVE_DEL, errp)) {
+            goto out;
+        }
+
+        if (bs->refcnt > 1 || !QLIST_EMPTY(&bs->parents)) {
+            error_setg(errp, "Block device %s is in use",
+                       bdrv_get_device_or_node_name(bs));
+            goto out;
+        }
+    }
+
+    if (blk) {
+        blk_unref(blk);
+    } else {
+        bdrv_unref(bs);
+    }
+
+out:
+    aio_context_release(aio_context);
+}
+
 BlockJobInfoList *qmp_query_block_jobs(Error **errp)
 {
     BlockJobInfoList *head = NULL, **p_next = &head;
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 083d2cd..470e86c 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1895,8 +1895,8 @@
 # level and no BlockBackend will be created.
 #
 # This command is still a work in progress.  It doesn't support all
-# block drivers, it lacks a matching blockdev-del, and more.  Stay
-# away from it unless you want to help with its development.
+# block drivers among other things.  Stay away from it unless you want
+# to help with its development.
 #
 # @options: block device options for the new device
 #
@@ -1905,6 +1905,34 @@
 { 'command': 'blockdev-add', 'data': { 'options': 'BlockdevOptions' } }
 
 ##
+# @x-blockdev-del:
+#
+# Deletes a block device that has been added using blockdev-add.
+# The selected device can be either a block backend or a graph node.
+#
+# In the former case the backend will be destroyed, along with its
+# inserted medium if there's any. The command will fail if the backend
+# or its medium are in use.
+#
+# In the latter case the node will be destroyed. The command will fail
+# if the node is attached to a block backend or is otherwise being
+# used.
+#
+# One of @id or @node-name must be specified, but not both.
+#
+# This command is still a work in progress and is considered
+# experimental. Stay away from it unless you want to help with its
+# development.
+#
+# @id: #optional Name of the block backend device to delete.
+#
+# @node-name: #optional Name of the graph node to delete.
+#
+# Since: 2.5
+##
+{ 'command': 'x-blockdev-del', 'data': { '*id': 'str', '*node-name': 'str' } }
+
+##
 # @blockdev-open-tray:
 #
 # Opens a block device's tray. If there is a block driver state tree inserted as
diff --git a/qmp-commands.hx b/qmp-commands.hx
index bd4d38f..658e7da 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -3927,8 +3927,8 @@ blockdev-add
 Add a block device.
 
 This command is still a work in progress.  It doesn't support all
-block drivers, it lacks a matching blockdev-del, and more.  Stay away
-from it unless you want to help with its development.
+block drivers among other things.  Stay away from it unless you want
+to help with its development.
 
 Arguments:
 
@@ -3974,6 +3974,63 @@ Example (2):
 EQMP
 
     {
+        .name       = "x-blockdev-del",
+        .args_type  = "id:s?,node-name:s?",
+        .mhandler.cmd_new = qmp_marshal_x_blockdev_del,
+    },
+
+SQMP
+x-blockdev-del
+------------
+Since 2.5
+
+Deletes a block device thas has been added using blockdev-add.
+The selected device can be either a block backend or a graph node.
+
+In the former case the backend will be destroyed, along with its
+inserted medium if there's any. The command will fail if the backend
+or its medium are in use.
+
+In the latter case the node will be destroyed. The command will fail
+if the node is attached to a block backend or is otherwise being
+used.
+
+One of "id" or "node-name" must be specified, but not both.
+
+This command is still a work in progress and is considered
+experimental. Stay away from it unless you want to help with its
+development.
+
+Arguments:
+
+- "id": Name of the block backend device to delete (json-string, optional)
+- "node-name": Name of the graph node to delete (json-string, optional)
+
+Example:
+
+-> { "execute": "blockdev-add",
+     "arguments": {
+         "options": {
+             "driver": "qcow2",
+             "id": "drive0",
+             "file": {
+                 "driver": "file",
+                 "filename": "test.qcow2"
+             }
+         }
+     }
+   }
+
+<- { "return": {} }
+
+-> { "execute": "x-blockdev-del",
+     "arguments": { "id": "drive0" }
+   }
+<- { "return": {} }
+
+EQMP
+
+    {
         .name       = "blockdev-open-tray",
         .args_type  = "device:s,force:b?",
         .mhandler.cmd_new = qmp_marshal_blockdev_open_tray,
-- 
1.8.3.1

[Qemu-devel] [PULL v2 39/40] iotests: Add tests for the x-blockdev-del command

[Kevin Wolf]

From: Alberto Garcia <berto@igalia.com>

Signed-off-by: Alberto Garcia <berto@igalia.com>
Message-id: 57c3b0d4d0c73ddadd19e5bded9492c359cc4568.1446475331.git.berto@igalia.com
Reviewed-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/139     | 414 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/139.out |   5 +
 tests/qemu-iotests/group   |   1 +
 3 files changed, 420 insertions(+)
 create mode 100644 tests/qemu-iotests/139
 create mode 100644 tests/qemu-iotests/139.out

diff --git a/tests/qemu-iotests/139 b/tests/qemu-iotests/139
new file mode 100644
index 0000000..b5470f7
--- /dev/null
+++ b/tests/qemu-iotests/139
@@ -0,0 +1,414 @@
+#!/usr/bin/env python
+#
+# Test cases for the QMP 'x-blockdev-del' command
+#
+# Copyright (C) 2015 Igalia, S.L.
+# Author: Alberto Garcia <berto@igalia.com>
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+import os
+import iotests
+import time
+
+base_img = os.path.join(iotests.test_dir, 'base.img')
+new_img = os.path.join(iotests.test_dir, 'new.img')
+
+class TestBlockdevDel(iotests.QMPTestCase):
+
+    def setUp(self):
+        iotests.qemu_img('create', '-f', iotests.imgfmt, base_img, '1M')
+        self.vm = iotests.VM()
+        self.vm.launch()
+
+    def tearDown(self):
+        self.vm.shutdown()
+        os.remove(base_img)
+        if os.path.isfile(new_img):
+            os.remove(new_img)
+
+    # Check whether a BlockBackend exists
+    def checkBlockBackend(self, backend, node, must_exist = True):
+        result = self.vm.qmp('query-block')
+        backends = filter(lambda x: x['device'] == backend, result['return'])
+        self.assertLessEqual(len(backends), 1)
+        self.assertEqual(must_exist, len(backends) == 1)
+        if must_exist:
+            if node:
+                self.assertEqual(backends[0]['inserted']['node-name'], node)
+            else:
+                self.assertFalse(backends[0].has_key('inserted'))
+
+    # Check whether a BlockDriverState exists
+    def checkBlockDriverState(self, node, must_exist = True):
+        result = self.vm.qmp('query-named-block-nodes')
+        nodes = filter(lambda x: x['node-name'] == node, result['return'])
+        self.assertLessEqual(len(nodes), 1)
+        self.assertEqual(must_exist, len(nodes) == 1)
+
+    # Add a new BlockBackend (with its attached BlockDriverState)
+    def addBlockBackend(self, backend, node):
+        file_node = '%s_file' % node
+        self.checkBlockBackend(backend, node, False)
+        self.checkBlockDriverState(node, False)
+        self.checkBlockDriverState(file_node, False)
+        opts = {'driver': iotests.imgfmt,
+                'id': backend,
+                'node-name': node,
+                'file': {'driver': 'file',
+                         'node-name': file_node,
+                         'filename': base_img}}
+        result = self.vm.qmp('blockdev-add', conv_keys = False, options = opts)
+        self.assert_qmp(result, 'return', {})
+        self.checkBlockBackend(backend, node)
+        self.checkBlockDriverState(node)
+        self.checkBlockDriverState(file_node)
+
+    # Add a BlockDriverState without a BlockBackend
+    def addBlockDriverState(self, node):
+        file_node = '%s_file' % node
+        self.checkBlockDriverState(node, False)
+        self.checkBlockDriverState(file_node, False)
+        opts = {'driver': iotests.imgfmt,
+                'node-name': node,
+                'file': {'driver': 'file',
+                         'node-name': file_node,
+                         'filename': base_img}}
+        result = self.vm.qmp('blockdev-add', conv_keys = False, options = opts)
+        self.assert_qmp(result, 'return', {})
+        self.checkBlockDriverState(node)
+        self.checkBlockDriverState(file_node)
+
+    # Add a BlockDriverState that will be used as overlay for the base_img BDS
+    def addBlockDriverStateOverlay(self, node):
+        self.checkBlockDriverState(node, False)
+        iotests.qemu_img('create', '-f', iotests.imgfmt,
+                         '-b', base_img, new_img, '1M')
+        opts = {'driver': iotests.imgfmt,
+                'node-name': node,
+                'backing': '',
+                'file': {'driver': 'file',
+                         'filename': new_img}}
+        result = self.vm.qmp('blockdev-add', conv_keys = False, options = opts)
+        self.assert_qmp(result, 'return', {})
+        self.checkBlockDriverState(node)
+
+    # Delete a BlockBackend
+    def delBlockBackend(self, backend, node, expect_error = False,
+                        destroys_media = True):
+        self.checkBlockBackend(backend, node)
+        if node:
+            self.checkBlockDriverState(node)
+        result = self.vm.qmp('x-blockdev-del', id = backend)
+        if expect_error:
+            self.assert_qmp(result, 'error/class', 'GenericError')
+            if node:
+                self.checkBlockDriverState(node)
+        else:
+            self.assert_qmp(result, 'return', {})
+            if node:
+                self.checkBlockDriverState(node, not destroys_media)
+        self.checkBlockBackend(backend, node, must_exist = expect_error)
+
+    # Delete a BlockDriverState
+    def delBlockDriverState(self, node, expect_error = False):
+        self.checkBlockDriverState(node)
+        result = self.vm.qmp('x-blockdev-del', node_name = node)
+        if expect_error:
+            self.assert_qmp(result, 'error/class', 'GenericError')
+        else:
+            self.assert_qmp(result, 'return', {})
+        self.checkBlockDriverState(node, expect_error)
+
+    # Add a device model
+    def addDeviceModel(self, device, backend):
+        result = self.vm.qmp('device_add', id = device,
+                             driver = 'virtio-blk-pci', drive = backend)
+        self.assert_qmp(result, 'return', {})
+
+    # Delete a device model
+    def delDeviceModel(self, device):
+        result = self.vm.qmp('device_del', id = device)
+        self.assert_qmp(result, 'return', {})
+
+        result = self.vm.qmp('system_reset')
+        self.assert_qmp(result, 'return', {})
+
+        device_path = '/machine/peripheral/%s/virtio-backend' % device
+        event = self.vm.event_wait(name="DEVICE_DELETED",
+                                   match={'data': {'path': device_path}})
+        self.assertNotEqual(event, None)
+
+        event = self.vm.event_wait(name="DEVICE_DELETED",
+                                   match={'data': {'device': device}})
+        self.assertNotEqual(event, None)
+
+    # Remove a BlockDriverState
+    def ejectDrive(self, backend, node, expect_error = False,
+                   destroys_media = True):
+        self.checkBlockBackend(backend, node)
+        self.checkBlockDriverState(node)
+        result = self.vm.qmp('eject', device = backend)
+        if expect_error:
+            self.assert_qmp(result, 'error/class', 'GenericError')
+            self.checkBlockDriverState(node)
+            self.checkBlockBackend(backend, node)
+        else:
+            self.assert_qmp(result, 'return', {})
+            self.checkBlockDriverState(node, not destroys_media)
+            self.checkBlockBackend(backend, None)
+
+    # Insert a BlockDriverState
+    def insertDrive(self, backend, node):
+        self.checkBlockBackend(backend, None)
+        self.checkBlockDriverState(node)
+        result = self.vm.qmp('blockdev-insert-medium',
+                             device = backend, node_name = node)
+        self.assert_qmp(result, 'return', {})
+        self.checkBlockBackend(backend, node)
+        self.checkBlockDriverState(node)
+
+    # Create a snapshot using 'blockdev-snapshot-sync'
+    def createSnapshotSync(self, node, overlay):
+        self.checkBlockDriverState(node)
+        self.checkBlockDriverState(overlay, False)
+        opts = {'node-name': node,
+                'snapshot-file': new_img,
+                'snapshot-node-name': overlay,
+                'format': iotests.imgfmt}
+        result = self.vm.qmp('blockdev-snapshot-sync', conv_keys=False, **opts)
+        self.assert_qmp(result, 'return', {})
+        self.checkBlockDriverState(node)
+        self.checkBlockDriverState(overlay)
+
+    # Create a snapshot using 'blockdev-snapshot'
+    def createSnapshot(self, node, overlay):
+        self.checkBlockDriverState(node)
+        self.checkBlockDriverState(overlay)
+        result = self.vm.qmp('blockdev-snapshot',
+                             node = node, overlay = overlay)
+        self.assert_qmp(result, 'return', {})
+        self.checkBlockDriverState(node)
+        self.checkBlockDriverState(overlay)
+
+    # Create a mirror
+    def createMirror(self, backend, node, new_node):
+        self.checkBlockBackend(backend, node)
+        self.checkBlockDriverState(new_node, False)
+        opts = {'device': backend,
+                'target': new_img,
+                'node-name': new_node,
+                'sync': 'top',
+                'format': iotests.imgfmt}
+        result = self.vm.qmp('drive-mirror', conv_keys=False, **opts)
+        self.assert_qmp(result, 'return', {})
+        self.checkBlockBackend(backend, node)
+        self.checkBlockDriverState(new_node)
+
+    # Complete an existing block job
+    def completeBlockJob(self, backend, node_before, node_after):
+        self.checkBlockBackend(backend, node_before)
+        result = self.vm.qmp('block-job-complete', device=backend)
+        self.assert_qmp(result, 'return', {})
+        self.wait_until_completed(backend)
+        self.checkBlockBackend(backend, node_after)
+
+    # Add a BlkDebug node
+    # Note that the purpose of this is to test the x-blockdev-del
+    # sanity checks, not to create a usable blkdebug drive
+    def addBlkDebug(self, debug, node):
+        self.checkBlockDriverState(node, False)
+        self.checkBlockDriverState(debug, False)
+        image = {'driver': iotests.imgfmt,
+                 'node-name': node,
+                 'file': {'driver': 'file',
+                          'filename': base_img}}
+        opts = {'driver': 'blkdebug',
+                'node-name': debug,
+                'image': image}
+        result = self.vm.qmp('blockdev-add', conv_keys = False, options = opts)
+        self.assert_qmp(result, 'return', {})
+        self.checkBlockDriverState(node)
+        self.checkBlockDriverState(debug)
+
+    # Add a BlkVerify node
+    # Note that the purpose of this is to test the x-blockdev-del
+    # sanity checks, not to create a usable blkverify drive
+    def addBlkVerify(self, blkverify, test, raw):
+        self.checkBlockDriverState(test, False)
+        self.checkBlockDriverState(raw, False)
+        self.checkBlockDriverState(blkverify, False)
+        iotests.qemu_img('create', '-f', iotests.imgfmt, new_img, '1M')
+        node_0 = {'driver': iotests.imgfmt,
+                  'node-name': test,
+                  'file': {'driver': 'file',
+                           'filename': base_img}}
+        node_1 = {'driver': iotests.imgfmt,
+                  'node-name': raw,
+                  'file': {'driver': 'file',
+                           'filename': new_img}}
+        opts = {'driver': 'blkverify',
+                'node-name': blkverify,
+                'test': node_0,
+                'raw': node_1}
+        result = self.vm.qmp('blockdev-add', conv_keys = False, options = opts)
+        self.assert_qmp(result, 'return', {})
+        self.checkBlockDriverState(test)
+        self.checkBlockDriverState(raw)
+        self.checkBlockDriverState(blkverify)
+
+    # Add a Quorum node
+    def addQuorum(self, quorum, child0, child1):
+        self.checkBlockDriverState(child0, False)
+        self.checkBlockDriverState(child1, False)
+        self.checkBlockDriverState(quorum, False)
+        iotests.qemu_img('create', '-f', iotests.imgfmt, new_img, '1M')
+        child_0 = {'driver': iotests.imgfmt,
+                   'node-name': child0,
+                   'file': {'driver': 'file',
+                            'filename': base_img}}
+        child_1 = {'driver': iotests.imgfmt,
+                   'node-name': child1,
+                   'file': {'driver': 'file',
+                            'filename': new_img}}
+        opts = {'driver': 'quorum',
+                'node-name': quorum,
+                'vote-threshold': 1,
+                'children': [ child_0, child_1 ]}
+        result = self.vm.qmp('blockdev-add', conv_keys = False, options = opts)
+        self.assert_qmp(result, 'return', {})
+        self.checkBlockDriverState(child0)
+        self.checkBlockDriverState(child1)
+        self.checkBlockDriverState(quorum)
+
+    ########################
+    # The tests start here #
+    ########################
+
+    def testWrongParameters(self):
+        self.addBlockBackend('drive0', 'node0')
+        result = self.vm.qmp('x-blockdev-del')
+        self.assert_qmp(result, 'error/class', 'GenericError')
+        result = self.vm.qmp('x-blockdev-del', id='drive0', node_name='node0')
+        self.assert_qmp(result, 'error/class', 'GenericError')
+        self.delBlockBackend('drive0', 'node0')
+
+    def testBlockBackend(self):
+        self.addBlockBackend('drive0', 'node0')
+        # You cannot delete a BDS that is attached to a backend
+        self.delBlockDriverState('node0', expect_error = True)
+        self.delBlockBackend('drive0', 'node0')
+
+    def testBlockDriverState(self):
+        self.addBlockDriverState('node0')
+        # You cannot delete a file BDS directly
+        self.delBlockDriverState('node0_file', expect_error = True)
+        self.delBlockDriverState('node0')
+
+    def testEject(self):
+        self.addBlockBackend('drive0', 'node0')
+        self.ejectDrive('drive0', 'node0')
+        self.delBlockBackend('drive0', None)
+
+    def testDeviceModel(self):
+        self.addBlockBackend('drive0', 'node0')
+        self.addDeviceModel('device0', 'drive0')
+        self.ejectDrive('drive0', 'node0', expect_error = True)
+        self.delBlockBackend('drive0', 'node0', expect_error = True)
+        self.delDeviceModel('device0')
+        self.delBlockBackend('drive0', 'node0')
+
+    def testAttachMedia(self):
+        # This creates a BlockBackend and removes its media
+        self.addBlockBackend('drive0', 'node0')
+        self.ejectDrive('drive0', 'node0')
+        # This creates a new BlockDriverState and inserts it into the backend
+        self.addBlockDriverState('node1')
+        self.insertDrive('drive0', 'node1')
+        # The backend can't be removed: the new BDS has an extra reference
+        self.delBlockBackend('drive0', 'node1', expect_error = True)
+        self.delBlockDriverState('node1', expect_error = True)
+        # The BDS still exists after being ejected, but now it can be removed
+        self.ejectDrive('drive0', 'node1', destroys_media = False)
+        self.delBlockDriverState('node1')
+        self.delBlockBackend('drive0', None)
+
+    def testSnapshotSync(self):
+        self.addBlockBackend('drive0', 'node0')
+        self.createSnapshotSync('node0', 'overlay0')
+        # This fails because node0 is now being used as a backing image
+        self.delBlockDriverState('node0', expect_error = True)
+        # This succeeds because overlay0 only has the backend reference
+        self.delBlockBackend('drive0', 'overlay0')
+        self.checkBlockDriverState('node0', False)
+
+    def testSnapshot(self):
+        self.addBlockBackend('drive0', 'node0')
+        self.addBlockDriverStateOverlay('overlay0')
+        self.createSnapshot('node0', 'overlay0')
+        self.delBlockBackend('drive0', 'overlay0', expect_error = True)
+        self.delBlockDriverState('node0', expect_error = True)
+        self.delBlockDriverState('overlay0', expect_error = True)
+        self.ejectDrive('drive0', 'overlay0', destroys_media = False)
+        self.delBlockBackend('drive0', None)
+        self.delBlockDriverState('node0', expect_error = True)
+        self.delBlockDriverState('overlay0')
+        self.checkBlockDriverState('node0', False)
+
+    def testMirror(self):
+        self.addBlockBackend('drive0', 'node0')
+        self.createMirror('drive0', 'node0', 'mirror0')
+        # The block job prevents removing the device
+        self.delBlockBackend('drive0', 'node0', expect_error = True)
+        self.delBlockDriverState('node0', expect_error = True)
+        self.delBlockDriverState('mirror0', expect_error = True)
+        self.wait_ready('drive0')
+        self.completeBlockJob('drive0', 'node0', 'mirror0')
+        self.assert_no_active_block_jobs()
+        self.checkBlockDriverState('node0', False)
+        # This succeeds because the backend now points to mirror0
+        self.delBlockBackend('drive0', 'mirror0')
+
+    def testBlkDebug(self):
+        self.addBlkDebug('debug0', 'node0')
+        # 'node0' is used by the blkdebug node
+        self.delBlockDriverState('node0', expect_error = True)
+        # But we can remove the blkdebug node directly
+        self.delBlockDriverState('debug0')
+        self.checkBlockDriverState('node0', False)
+
+    def testBlkVerify(self):
+        self.addBlkVerify('verify0', 'node0', 'node1')
+        # We cannot remove the children of a blkverify device
+        self.delBlockDriverState('node0', expect_error = True)
+        self.delBlockDriverState('node1', expect_error = True)
+        # But we can remove the blkverify node directly
+        self.delBlockDriverState('verify0')
+        self.checkBlockDriverState('node0', False)
+        self.checkBlockDriverState('node1', False)
+
+    def testQuorum(self):
+        self.addQuorum('quorum0', 'node0', 'node1')
+        # We cannot remove the children of a Quorum device
+        self.delBlockDriverState('node0', expect_error = True)
+        self.delBlockDriverState('node1', expect_error = True)
+        # But we can remove the Quorum node directly
+        self.delBlockDriverState('quorum0')
+        self.checkBlockDriverState('node0', False)
+        self.checkBlockDriverState('node1', False)
+
+
+if __name__ == '__main__':
+    iotests.main(supported_fmts=["qcow2"])
diff --git a/tests/qemu-iotests/139.out b/tests/qemu-iotests/139.out
new file mode 100644
index 0000000..281b69e
--- /dev/null
+++ b/tests/qemu-iotests/139.out
@@ -0,0 +1,5 @@
+............
+----------------------------------------------------------------------
+Ran 12 tests
+
+OK
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index a6a61ae..c69265d 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -138,3 +138,4 @@
 135 rw auto
 137 rw auto
 138 rw auto quick
+139 rw auto quick
-- 
1.8.3.1

Re: [Qemu-devel] [Qemu-block] [PULL v2 39/40] iotests: Add tests for the x-blockdev-del command

[Stefan Hajnoczi]

On Tue, Nov 10, 2015 at 2:09 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> From: Alberto Garcia <berto@igalia.com>
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> Message-id: 57c3b0d4d0c73ddadd19e5bded9492c359cc4568.1446475331.git.berto@igalia.com
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/139     | 414 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/139.out |   5 +
>  tests/qemu-iotests/group   |   1 +
>  3 files changed, 420 insertions(+)
>  create mode 100644 tests/qemu-iotests/139
>  create mode 100644 tests/qemu-iotests/139.out

I'm seeing the following failure:

 ./check -qcow2 139
QEMU          -- "./qemu" -nodefaults
QEMU_IMG      -- "./qemu-img"
QEMU_IO       -- "./qemu-io"  -f qcow2 --cache writeback
QEMU_NBD      -- "./qemu-nbd"
IMGFMT        -- qcow2 (compat=1.1)
IMGPROTO      -- file
PLATFORM      -- Linux/x86_64 stefanha-x1 4.2.5-300.fc23.x86_64
TEST_DIR      -- /home/stefanha/qemu/tests/qemu-iotests/scratch
SOCKET_SCM_HELPER -- /home/stefanha/qemu/tests/qemu-iotests/socket_scm_helper

139         [failed, exit status 1] - output mismatch (see 139.out.bad)
--- /home/stefanha/qemu/tests/qemu-iotests/139.out    2015-11-10
14:24:03.728322694 +0000
+++ 139.out.bad    2015-11-10 14:54:51.617899443 +0000
@@ -1,5 +1,19 @@
-............
+........F...
+======================================================================
+FAIL: testQuorum (__main__.TestBlockdevDel)
+----------------------------------------------------------------------
+Traceback (most recent call last):
+  File "139", line 403, in testQuorum
+    self.addQuorum('quorum0', 'node0', 'node1')
+  File "139", line 291, in addQuorum
+    self.assert_qmp(result, 'return', {})
+  File "/home/stefanha/qemu/tests/qemu-iotests/iotests.py", line 314,
in assert_qmp
+    result = self.dictpath(d, path)
+  File "/home/stefanha/qemu/tests/qemu-iotests/iotests.py", line 293,
in dictpath
+    self.fail('failed path traversal for "%s" in "%s"' % (path, str(d)))
+AssertionError: failed path traversal for "return" in "{u'error':
{u'class': u'GenericError', u'desc': u"Unknown driver 'quorum'"}}"
+
 ----------------------------------------------------------------------
 Ran 12 tests

-OK
+FAILED (failures=1)
Failures: 139
Failed 1 of 1 tests

Re: [Qemu-devel] [Qemu-block] [PULL v2 39/40] iotests: Add tests for the x-blockdev-del command

[Kevin Wolf]

Am 10.11.2015 um 15:59 hat Stefan Hajnoczi geschrieben:
> On Tue, Nov 10, 2015 at 2:09 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> > From: Alberto Garcia <berto@igalia.com>
> >
> > Signed-off-by: Alberto Garcia <berto@igalia.com>
> > Message-id: 57c3b0d4d0c73ddadd19e5bded9492c359cc4568.1446475331.git.berto@igalia.com
> > Reviewed-by: Max Reitz <mreitz@redhat.com>
> > Signed-off-by: Max Reitz <mreitz@redhat.com>
> > ---
> >  tests/qemu-iotests/139     | 414 +++++++++++++++++++++++++++++++++++++++++++++
> >  tests/qemu-iotests/139.out |   5 +
> >  tests/qemu-iotests/group   |   1 +
> >  3 files changed, 420 insertions(+)
> >  create mode 100644 tests/qemu-iotests/139
> >  create mode 100644 tests/qemu-iotests/139.out
> 
> I'm seeing the following failure:
> 
>  ./check -qcow2 139
> QEMU          -- "./qemu" -nodefaults
> QEMU_IMG      -- "./qemu-img"
> QEMU_IO       -- "./qemu-io"  -f qcow2 --cache writeback
> QEMU_NBD      -- "./qemu-nbd"
> IMGFMT        -- qcow2 (compat=1.1)
> IMGPROTO      -- file
> PLATFORM      -- Linux/x86_64 stefanha-x1 4.2.5-300.fc23.x86_64
> TEST_DIR      -- /home/stefanha/qemu/tests/qemu-iotests/scratch
> SOCKET_SCM_HELPER -- /home/stefanha/qemu/tests/qemu-iotests/socket_scm_helper
> 
> 139         [failed, exit status 1] - output mismatch (see 139.out.bad)
> --- /home/stefanha/qemu/tests/qemu-iotests/139.out    2015-11-10
> 14:24:03.728322694 +0000
> +++ 139.out.bad    2015-11-10 14:54:51.617899443 +0000
> @@ -1,5 +1,19 @@
> -............
> +........F...
> +======================================================================
> +FAIL: testQuorum (__main__.TestBlockdevDel)
> +----------------------------------------------------------------------
> +Traceback (most recent call last):
> +  File "139", line 403, in testQuorum
> +    self.addQuorum('quorum0', 'node0', 'node1')
> +  File "139", line 291, in addQuorum
> +    self.assert_qmp(result, 'return', {})
> +  File "/home/stefanha/qemu/tests/qemu-iotests/iotests.py", line 314,
> in assert_qmp
> +    result = self.dictpath(d, path)
> +  File "/home/stefanha/qemu/tests/qemu-iotests/iotests.py", line 293,
> in dictpath
> +    self.fail('failed path traversal for "%s" in "%s"' % (path, str(d)))
> +AssertionError: failed path traversal for "return" in "{u'error':
> {u'class': u'GenericError', u'desc': u"Unknown driver 'quorum'"}}"
> +

I guess we need a follow-up patch for -rc1 that simply skips the test
case if quorum isn't available.

Kevin

[Qemu-devel] [PULL v2 40/40] qcow2: Fix qcow2_get_cluster_offset() for zero clusters

[Kevin Wolf]

When searching for contiguous zero clusters, we only need to check the
cluster type. Before this patch, an increasing offset (L2E_OFFSET_MASK)
was expected, so that the function never returned more than a single
zero cluster in practice. This patch fixes it to actually return as many
contiguous zero clusters as it can.

Cc: qemu-stable@nongnu.org
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-id: 1446657384-5907-1-git-send-email-kwolf@redhat.com
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2-cluster.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 67be0ce..24a60e2 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -312,7 +312,7 @@ static int count_contiguous_clusters(int nb_clusters, int cluster_size,
     if (!offset)
         return 0;
 
-    assert(qcow2_get_cluster_type(first_entry) != QCOW2_CLUSTER_COMPRESSED);
+    assert(qcow2_get_cluster_type(first_entry) == QCOW2_CLUSTER_NORMAL);
 
     for (i = 0; i < nb_clusters; i++) {
         uint64_t l2_entry = be64_to_cpu(l2_table[i]) & mask;
@@ -324,14 +324,16 @@ static int count_contiguous_clusters(int nb_clusters, int cluster_size,
 	return i;
 }
 
-static int count_contiguous_free_clusters(int nb_clusters, uint64_t *l2_table)
+static int count_contiguous_clusters_by_type(int nb_clusters,
+                                             uint64_t *l2_table,
+                                             int wanted_type)
 {
     int i;
 
     for (i = 0; i < nb_clusters; i++) {
         int type = qcow2_get_cluster_type(be64_to_cpu(l2_table[i]));
 
-        if (type != QCOW2_CLUSTER_UNALLOCATED) {
+        if (type != wanted_type) {
             break;
         }
     }
@@ -554,13 +556,14 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset,
             ret = -EIO;
             goto fail;
         }
-        c = count_contiguous_clusters(nb_clusters, s->cluster_size,
-                &l2_table[l2_index], QCOW_OFLAG_ZERO);
+        c = count_contiguous_clusters_by_type(nb_clusters, &l2_table[l2_index],
+                                              QCOW2_CLUSTER_ZERO);
         *cluster_offset = 0;
         break;
     case QCOW2_CLUSTER_UNALLOCATED:
         /* how many empty clusters ? */
-        c = count_contiguous_free_clusters(nb_clusters, &l2_table[l2_index]);
+        c = count_contiguous_clusters_by_type(nb_clusters, &l2_table[l2_index],
+                                              QCOW2_CLUSTER_UNALLOCATED);
         *cluster_offset = 0;
         break;
     case QCOW2_CLUSTER_NORMAL:
-- 
1.8.3.1

Re: [Qemu-devel] [PULL v2 00/40] Block layer patches

[Peter Maydell]

On 10 November 2015 at 14:09, Kevin Wolf <kwolf@redhat.com> wrote:
> The following changes since commit a8b4f9585a0bf5186fca793ce2c5d754cd8ec49a:
>
>   Merge remote-tracking branch 'remotes/armbru/tags/pull-qapi-2015-11-10' into staging (2015-11-10 09:39:24 +0000)
>
> are available in the git repository at:
>
>
>   git://repo.or.cz/qemu/kevin.git tags/for-upstream
>
> for you to fetch changes up to c400bddb916268394e352f82809eb4728424a5b1:
>
>   Merge remote-tracking branch 'mreitz/tags/pull-block-for-kevin-2015-11-10' into queue-block (2015-11-10 14:59:26 +0100)
>
> ----------------------------------------------------------------
>
> Block layer patches

Fails to build on OSX :-(

/Users/pm215/src/qemu-for-merges/ui/cocoa.m:1121:40: error: too few
arguments to function call, expected 7, have 5
                                   &err);
                                       ^
./qmp-commands.h:61:1: note: 'qmp_blockdev_change_medium' declared here
void qmp_blockdev_change_medium(const char *device, const char
*filename, bool has_format, const char *format, bool
has_read_only_mode, BlockdevChangeReadOnlyMode read_only_mode, Error
**errp);
^
1 error generated.

Also some warnings:

/Users/pm215/src/qemu-for-merges/qemu-io-cmds.c:772:56: warning:
format specifies type 'size_t' (aka 'unsigned long') but the argument
has type 'unsigned long lo
ng' [-Wformat]
        printf("length cannot exceed %zu, given %s\n", SIZE_MAX, argv[optind]);
                                     ~~~               ^~~~~~~~
                                     %llu
/usr/include/stdint.h:153:20: note: expanded from macro 'SIZE_MAX'
#define SIZE_MAX          UINT64_MAX
                          ^~~~~~~~~~
/usr/include/stdint.h:87:27: note: expanded from macro 'UINT64_MAX'
#define UINT64_MAX        18446744073709551615ULL
                          ^~~~~~~~~~~~~~~~~~~~~~~
/Users/pm215/src/qemu-for-merges/qemu-io-cmds.c:1082:56: warning:
format specifies type 'size_t' (aka 'unsigned long') but the argument
has type 'unsigned long l
ong' [-Wformat]
        printf("length cannot exceed %zu, given %s\n", SIZE_MAX, argv[optind]);
                                     ~~~               ^~~~~~~~
                                     %llu
/usr/include/stdint.h:153:20: note: expanded from macro 'SIZE_MAX'
#define SIZE_MAX          UINT64_MAX
                          ^~~~~~~~~~
/usr/include/stdint.h:87:27: note: expanded from macro 'UINT64_MAX'
#define UINT64_MAX        18446744073709551615ULL
                          ^~~~~~~~~~~~~~~~~~~~~~~

thanks
-- PMM

Re: [Qemu-devel] [PULL v2 00/40] Block layer patches

[Kevin Wolf]

Am 10.11.2015 um 18:10 hat Peter Maydell geschrieben:
> On 10 November 2015 at 14:09, Kevin Wolf <kwolf@redhat.com> wrote:
> > The following changes since commit a8b4f9585a0bf5186fca793ce2c5d754cd8ec49a:
> >
> >   Merge remote-tracking branch 'remotes/armbru/tags/pull-qapi-2015-11-10' into staging (2015-11-10 09:39:24 +0000)
> >
> > are available in the git repository at:
> >
> >
> >   git://repo.or.cz/qemu/kevin.git tags/for-upstream
> >
> > for you to fetch changes up to c400bddb916268394e352f82809eb4728424a5b1:
> >
> >   Merge remote-tracking branch 'mreitz/tags/pull-block-for-kevin-2015-11-10' into queue-block (2015-11-10 14:59:26 +0100)
> >
> > ----------------------------------------------------------------
> >
> > Block layer patches
> 
> Fails to build on OSX :-(
> 
> /Users/pm215/src/qemu-for-merges/ui/cocoa.m:1121:40: error: too few
> arguments to function call, expected 7, have 5
>                                    &err);
>                                        ^
> ./qmp-commands.h:61:1: note: 'qmp_blockdev_change_medium' declared here
> void qmp_blockdev_change_medium(const char *device, const char
> *filename, bool has_format, const char *format, bool
> has_read_only_mode, BlockdevChangeReadOnlyMode read_only_mode, Error
> **errp);
> ^
> 1 error generated.
> 
> Also some warnings:
> 
> /Users/pm215/src/qemu-for-merges/qemu-io-cmds.c:772:56: warning:
> format specifies type 'size_t' (aka 'unsigned long') but the argument
> has type 'unsigned long lo
> ng' [-Wformat]
>         printf("length cannot exceed %zu, given %s\n", SIZE_MAX, argv[optind]);
>                                      ~~~               ^~~~~~~~
>                                      %llu
> /usr/include/stdint.h:153:20: note: expanded from macro 'SIZE_MAX'
> #define SIZE_MAX          UINT64_MAX
>                           ^~~~~~~~~~
> /usr/include/stdint.h:87:27: note: expanded from macro 'UINT64_MAX'
> #define UINT64_MAX        18446744073709551615ULL
>                           ^~~~~~~~~~~~~~~~~~~~~~~

Isn't that actually a bug in the system headers? If I understand the
spec correctly, SIZE_MAX should be size_t. ("this expression shall have
the same type as would an expression that is an object of the
corresponding type converted according to the integer promotions.")

Anyway, I'll squash in a workaround that explicitly casts to uint64_t.

Kevin

Re: [Qemu-devel] [PULL v2 00/40] Block layer patches

[Eric Blake]

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

On 11/11/2015 08:35 AM, Kevin Wolf wrote:

>> Also some warnings:
>>
>> /Users/pm215/src/qemu-for-merges/qemu-io-cmds.c:772:56: warning:
>> format specifies type 'size_t' (aka 'unsigned long') but the argument
>> has type 'unsigned long lo
>> ng' [-Wformat]
>>         printf("length cannot exceed %zu, given %s\n", SIZE_MAX, argv[optind]);
>>                                      ~~~               ^~~~~~~~
>>                                      %llu
>> /usr/include/stdint.h:153:20: note: expanded from macro 'SIZE_MAX'
>> #define SIZE_MAX          UINT64_MAX
>>                           ^~~~~~~~~~
>> /usr/include/stdint.h:87:27: note: expanded from macro 'UINT64_MAX'
>> #define UINT64_MAX        18446744073709551615ULL
>>                           ^~~~~~~~~~~~~~~~~~~~~~~
> 
> Isn't that actually a bug in the system headers? If I understand the
> spec correctly, SIZE_MAX should be size_t. ("this expression shall have
> the same type as would an expression that is an object of the
> corresponding type converted according to the integer promotions.")

You are correct; the system headers are buggy.

-- 
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: 604 bytes --]