Series comparison

-[Qemu-devel] [PULL 00/12] Block patches
+[PULL v2 00/13] Block patches
-The following changes since commit bfec359afba088aaacc7d316f43302f28c6e642a:
+The following changes since commit 42f6c9179be4401974dd3a75ee72defd16b5092d:
-  Merge remote-tracking branch 'remotes/armbru/tags/pull-qdev-2017-04-21' into staging (2017-04-21 11:42:03 +0100)
+  Merge tag 'pull-ppc-20211112' of https://github.com/legoater/qemu into staging (2021-11-12 12:28:25 +0100)
-are available in the git repository at:
+are available in the Git repository at:
-  git://github.com/codyprime/qemu-kvm-jtc.git tags/block-pull-request
+  https://gitlab.com/hreitz/qemu.git tags/pull-block-2021-11-16
-for you to fetch changes up to 1507631e438930bc07f776f303af127a9cdb4d41:
+for you to fetch changes up to 5dbd0ce115c7720268e653fe928bb6a0c1314a80:
-  qemu-iotests: _cleanup_qemu must be called on exit (2017-04-21 08:32:44 -0400)
+  file-posix: Fix alignment after reopen changing O_DIRECT (2021-11-16 11:30:29 +0100)
 ----------------------------------------------------------------
+Block patches for 6.2.0-rc1:
-Block patches for 2.10
+- Fixes to image streaming job and block layer reconfiguration to make
   iotest 030 pass again
 - docs: Deprecate incorrectly typed device_add arguments
 - file-posix: Fix alignment after reopen changing O_DIRECT
 ----------------------------------------------------------------
+v2:
+- Fixed iotest 142 (modified by "file-posix: Fix alignment after reopen
+  changing O_DIRECT") -- at least I hope so: for me, it now passes on a
+k block device, and the gitlab pipeline passed, too
-Ashish Mittal (2):
+- Note that because I had to modify Kevin's pull request, I did not want
-  block/vxhs.c: Add support for a new block device type called "vxhs"
+  to merge it partially (with a merge commit), but instead decided to
-  block/vxhs.c: Add qemu-iotests for new block device type "vxhs"
+  apply all patches from the pull request mails (including their message
   IDs)
-Jeff Cody (10):
+----------------------------------------------------------------
-  qemu-iotests: exclude vxhs from image creation via protocol
+Hanna Reitz (10):
-  block: add bdrv_set_read_only() helper function
+  stream: Traverse graph after modification
-  block: do not set BDS read_only if copy_on_read enabled
+  block: Manipulate children list in .attach/.detach
-  block: honor BDRV_O_ALLOW_RDWR when clearing bs->read_only
+  block: Unite remove_empty_child and child_free
-  block: code movement
+  block: Drop detached child from ignore list
-  block: introduce bdrv_can_set_read_only()
+  block: Pass BdrvChild ** to replace_child_noperm
-  block: use bdrv_can_set_read_only() during reopen
+  block: Restructure remove_file_or_backing_child()
-  block/rbd - update variable names to more apt names
+  transactions: Invoke clean() after everything else
-  block/rbd: Add support for reopen()
+  block: Let replace_child_tran keep indirect pointer
-  qemu-iotests: _cleanup_qemu must be called on exit
+  block: Let replace_child_noperm free children
   iotests/030: Unthrottle parallel jobs in reverse
- block.c                          |  56 +++-
+Kevin Wolf (2):
- block/Makefile.objs              |   2 +
+  docs: Deprecate incorrectly typed device_add arguments
- block/bochs.c                    |   5 +-
+  file-posix: Fix alignment after reopen changing O_DIRECT
- block/cloop.c                    |   5 +-
- block/dmg.c                      |   6 +-
+Stefan Hajnoczi (1):
- block/rbd.c                      |  65 +++--
+  softmmu/qdev-monitor: fix use-after-free in qdev_set_id()
- block/trace-events               |  17 ++
- block/vvfat.c                    |  19 +-
+ docs/about/deprecated.rst   |  14 +++
- block/vxhs.c                     | 575 +++++++++++++++++++++++++++++++++++++++
+ include/qemu/transactions.h |   3 +
- configure                        |  39 +++
+ block.c                     | 233 +++++++++++++++++++++++++++---------
- include/block/block.h            |   2 +
+ block/file-posix.c          |  20 +++-
- qapi/block-core.json             |  23 +-
+ block/stream.c              |   7 +-
- tests/qemu-iotests/017           |   1 +
+ softmmu/qdev-monitor.c      |   2 +-
- tests/qemu-iotests/020           |   1 +
+ util/transactions.c         |   8 +-
- tests/qemu-iotests/028           |   1 +
+ tests/qemu-iotests/030      |  11 +-
- tests/qemu-iotests/029           |   1 +
+ tests/qemu-iotests/142      |  29 +++++
- tests/qemu-iotests/073           |   1 +
+ tests/qemu-iotests/142.out  |  18 +++
- tests/qemu-iotests/094           |  11 +-
+files changed, 279 insertions(+), 66 deletions(-)
  tests/qemu-iotests/102           |   5 +-
  tests/qemu-iotests/109           |   1 +
  tests/qemu-iotests/114           |   1 +
  tests/qemu-iotests/117           |   1 +
  tests/qemu-iotests/130           |   2 +
  tests/qemu-iotests/134           |   1 +
  tests/qemu-iotests/140           |   1 +
  tests/qemu-iotests/141           |   1 +
  tests/qemu-iotests/143           |   1 +
  tests/qemu-iotests/156           |   2 +
  tests/qemu-iotests/158           |   1 +
  tests/qemu-iotests/common        |   6 +
  tests/qemu-iotests/common.config |  13 +
  tests/qemu-iotests/common.filter |   1 +
  tests/qemu-iotests/common.rc     |  19 ++
 files changed, 844 insertions(+), 42 deletions(-)
  create mode 100644 block/vxhs.c
 --
-.9.3
+.33.1

-[Qemu-devel] [PULL 12/12] qemu-iotests: _cleanup_qemu must be called on exit
+[PULL v2 01/13] stream: Traverse graph after modification
-For the tests that use the common.qemu functions for running a QEMU
+bdrv_cor_filter_drop() modifies the block graph.  That means that other
-process, _cleanup_qemu must be called in the exit function.
+parties can also modify the block graph before it returns.  Therefore,
 we cannot assume that the result of a graph traversal we did before
 remains valid afterwards.
-If it is not, if the qemu process aborts, then not all of the droppings
+We should thus fetch `base` and `unfiltered_base` afterwards instead of
-are cleaned up (e.g. pidfile, fifos).
+before.
-This updates those tests that did not have a cleanup in qemu-iotests.
+Signed-off-by: Hanna Reitz <hreitz@redhat.com>
 Reviewed-by: Kevin Wolf <kwolf@redhat.com>
 Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
 Message-Id: <20211111120829.81329-2-hreitz@redhat.com>
 Signed-off-by: Kevin Wolf <kwolf@redhat.com>
 Message-Id: <20211115145409.176785-2-kwolf@redhat.com>
 Signed-off-by: Hanna Reitz <hreitz@redhat.com>
 ---
  block/stream.c | 7 +++++--
 file changed, 5 insertions(+), 2 deletions(-)
-(I swapped spaces for tabs in test 102 as well)
+diff --git a/block/stream.c b/block/stream.c
+index XXXXXXX..XXXXXXX 100644
-Reported-by: Eric Blake <eblake@redhat.com>
+--- a/block/stream.c
-Reviewed-by: Eric Blake <eblake@redhat.com>
++++ b/block/stream.c
-Signed-off-by: Jeff Cody <jcody@redhat.com>
+@@ -XXX,XX +XXX,XX @@ static int stream_prepare(Job *job)
 Message-id: d59c2f6ad6c1da8b9b3c7f357c94a7122ccfc55a.1492544096.git.jcody@redhat.com
 ---
  tests/qemu-iotests/028 |  1 +
  tests/qemu-iotests/094 | 11 ++++++++---
  tests/qemu-iotests/102 |  5 +++--
  tests/qemu-iotests/109 |  1 +
  tests/qemu-iotests/117 |  1 +
  tests/qemu-iotests/130 |  1 +
  tests/qemu-iotests/140 |  1 +
  tests/qemu-iotests/141 |  1 +
  tests/qemu-iotests/143 |  1 +
  tests/qemu-iotests/156 |  1 +
 files changed, 19 insertions(+), 5 deletions(-)
 diff --git a/tests/qemu-iotests/028 b/tests/qemu-iotests/028
 index XXXXXXX..XXXXXXX 100755
 --- a/tests/qemu-iotests/028
 +++ b/tests/qemu-iotests/028
@@ -XXX,XX +XXX,XX @@ status=1    # failure is the default!
  _cleanup()
  {
-+    _cleanup_qemu
+     StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
-     rm -f "${TEST_IMG}.copy"
+     BlockDriverState *unfiltered_bs = bdrv_skip_filters(s->target_bs);
-     _cleanup_test_img
+-    BlockDriverState *base = bdrv_filter_or_cow_bs(s->above_base);
- }
+-    BlockDriverState *unfiltered_base = bdrv_skip_filters(base);
-diff --git a/tests/qemu-iotests/094 b/tests/qemu-iotests/094
++    BlockDriverState *base;
-index XXXXXXX..XXXXXXX 100755
++    BlockDriverState *unfiltered_base;
---- a/tests/qemu-iotests/094
+     Error *local_err = NULL;
-+++ b/tests/qemu-iotests/094
+     int ret = 0;
-@@ -XXX,XX +XXX,XX @@ echo "QA output created by $seq"
- here="$PWD"
+@@ -XXX,XX +XXX,XX @@ static int stream_prepare(Job *job)
- status=1    # failure is the default!
+     bdrv_cor_filter_drop(s->cor_filter_bs);
+     s->cor_filter_bs = NULL;
--trap "exit \$status" 0 1 2 3 15
-+_cleanup()
++    base = bdrv_filter_or_cow_bs(s->above_base);
-+{
++    unfiltered_base = bdrv_skip_filters(base);
 +    _cleanup_qemu
 +    _cleanup_test_img
 +    rm -f "$TEST_DIR/source.$IMGFMT"
 +}
 +
-+trap "_cleanup; exit \$status" 0 1 2 3 15
+     if (bdrv_cow_child(unfiltered_bs)) {
+         const char *base_id = NULL, *base_fmt = NULL;
- # get standard environment, filters and checks
+         if (unfiltered_base) {
  . ./common.rc
@@ -XXX,XX +XXX,XX @@ _send_qemu_cmd $QEMU_HANDLE \
  wait=1 _cleanup_qemu
 -_cleanup_test_img
 -rm -f "$TEST_DIR/source.$IMGFMT"
  # success, all done
  echo '*** done'
 diff --git a/tests/qemu-iotests/102 b/tests/qemu-iotests/102
 index XXXXXXX..XXXXXXX 100755
 --- a/tests/qemu-iotests/102
 +++ b/tests/qemu-iotests/102
@@ -XXX,XX +XXX,XX @@ seq=$(basename $0)
  echo "QA output created by $seq"
  here=$PWD
 -status=1    # failure is the default!
 +status=1    # failure is the default!
  _cleanup()
  {
 -    _cleanup_test_img
 +    _cleanup_qemu
 +    _cleanup_test_img
  }
  trap "_cleanup; exit \$status" 0 1 2 3 15
 diff --git a/tests/qemu-iotests/109 b/tests/qemu-iotests/109
 index XXXXXXX..XXXXXXX 100755
 --- a/tests/qemu-iotests/109
 +++ b/tests/qemu-iotests/109
@@ -XXX,XX +XXX,XX @@ status=1    # failure is the default!
  _cleanup()
  {
 +    _cleanup_qemu
      rm -f $TEST_IMG.src
      _cleanup_test_img
  }
 diff --git a/tests/qemu-iotests/117 b/tests/qemu-iotests/117
 index XXXXXXX..XXXXXXX 100755
 --- a/tests/qemu-iotests/117
 +++ b/tests/qemu-iotests/117
@@ -XXX,XX +XXX,XX @@ status=1    # failure is the default!
  _cleanup()
  {
 +    _cleanup_qemu
      _cleanup_test_img
  }
  trap "_cleanup; exit \$status" 0 1 2 3 15
 diff --git a/tests/qemu-iotests/130 b/tests/qemu-iotests/130
 index XXXXXXX..XXXXXXX 100755
 --- a/tests/qemu-iotests/130
 +++ b/tests/qemu-iotests/130
@@ -XXX,XX +XXX,XX @@ status=1    # failure is the default!
  _cleanup()
  {
 +    _cleanup_qemu
      _cleanup_test_img
  }
  trap "_cleanup; exit \$status" 0 1 2 3 15
 diff --git a/tests/qemu-iotests/140 b/tests/qemu-iotests/140
 index XXXXXXX..XXXXXXX 100755
 --- a/tests/qemu-iotests/140
 +++ b/tests/qemu-iotests/140
@@ -XXX,XX +XXX,XX @@ status=1    # failure is the default!
  _cleanup()
  {
 +    _cleanup_qemu
      _cleanup_test_img
      rm -f "$TEST_DIR/nbd"
  }
 diff --git a/tests/qemu-iotests/141 b/tests/qemu-iotests/141
 index XXXXXXX..XXXXXXX 100755
 --- a/tests/qemu-iotests/141
 +++ b/tests/qemu-iotests/141
@@ -XXX,XX +XXX,XX @@ status=1    # failure is the default!
  _cleanup()
  {
 +    _cleanup_qemu
      _cleanup_test_img
      rm -f "$TEST_DIR/{b,m,o}.$IMGFMT"
  }
 diff --git a/tests/qemu-iotests/143 b/tests/qemu-iotests/143
 index XXXXXXX..XXXXXXX 100755
 --- a/tests/qemu-iotests/143
 +++ b/tests/qemu-iotests/143
@@ -XXX,XX +XXX,XX @@ status=1    # failure is the default!
  _cleanup()
  {
 +    _cleanup_qemu
      rm -f "$TEST_DIR/nbd"
  }
  trap "_cleanup; exit \$status" 0 1 2 3 15
 diff --git a/tests/qemu-iotests/156 b/tests/qemu-iotests/156
 index XXXXXXX..XXXXXXX 100755
 --- a/tests/qemu-iotests/156
 +++ b/tests/qemu-iotests/156
@@ -XXX,XX +XXX,XX @@ status=1    # failure is the default!
  _cleanup()
  {
 +    _cleanup_qemu
      rm -f "$TEST_IMG{,.target}{,.backing,.overlay}"
  }
  trap "_cleanup; exit \$status" 0 1 2 3 15
 --
-.9.3
+.33.1

-[Qemu-devel] [PULL 07/12] block: code movement
+[PULL v2 02/13] block: Manipulate children list in .attach/.detach
-Move bdrv_is_read_only() up with its friends.
+The children list is specific to BDS parents.  We should not modify it
 in the general children modification code, but let BDS parents deal with
 it in their .attach() and .detach() methods.
-Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
+This also has the advantage that a BdrvChild is removed from the
-Reviewed-by: John Snow <jsnow@redhat.com>
+children list before its .bs pointer can become NULL.  BDS parents
-Signed-off-by: Jeff Cody <jcody@redhat.com>
+generally assume that their children's .bs pointer is never NULL, so
-Message-id: 73b2399459760c32506f9407efb9dddb3a2789de.1491597120.git.jcody@redhat.com
+this is actually a bug fix.
 Signed-off-by: Hanna Reitz <hreitz@redhat.com>
 Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
 Message-Id: <20211111120829.81329-3-hreitz@redhat.com>
 Signed-off-by: Kevin Wolf <kwolf@redhat.com>
 Message-Id: <20211115145409.176785-3-kwolf@redhat.com>
 Signed-off-by: Hanna Reitz <hreitz@redhat.com>
 ---
- block.c | 10 +++++-----
+ block.c | 14 +++++---------
-file changed, 5 insertions(+), 5 deletions(-)
+file changed, 5 insertions(+), 9 deletions(-)
 diff --git a/block.c b/block.c
 index XXXXXXX..XXXXXXX 100644
 --- a/block.c
 +++ b/block.c
-@@ -XXX,XX +XXX,XX @@ void path_combine(char *dest, int dest_size,
+@@ -XXX,XX +XXX,XX @@ static void bdrv_child_cb_attach(BdrvChild *child)
  {
      BlockDriverState *bs = child->opaque;
 +    QLIST_INSERT_HEAD(&bs->children, child, next);
 +
      if (child->role & BDRV_CHILD_COW) {
          bdrv_backing_attach(child);
      }
+@@ -XXX,XX +XXX,XX @@ static void bdrv_child_cb_detach(BdrvChild *child)
+     }
+     bdrv_unapply_subtree_drain(child, bs);
++
++    QLIST_REMOVE(child, next);
  }
-+bool bdrv_is_read_only(BlockDriverState *bs)
+ static int bdrv_child_cb_update_filename(BdrvChild *c, BlockDriverState *base,
-+{
+@@ -XXX,XX +XXX,XX @@ static void bdrv_child_free(void *opaque)
-+    return bs->read_only;
+ static void bdrv_remove_empty_child(BdrvChild *child)
 +}
 +
  int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
  {
-     /* Do not set read_only if copy_on_read is enabled */
+     assert(!child->bs);
-@@ -XXX,XX +XXX,XX @@ void bdrv_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr)
+-    QLIST_SAFE_REMOVE(child, next);
-     *nb_sectors_ptr = nb_sectors < 0 ? 0 : nb_sectors;
++    assert(!child->next.le_prev); /* not in children list */
      bdrv_child_free(child);
  }
--bool bdrv_is_read_only(BlockDriverState *bs)
+@@ -XXX,XX +XXX,XX @@ static int bdrv_attach_child_noperm(BlockDriverState *parent_bs,
--{
+         return ret;
--    return bs->read_only;
+     }
--}
 -    QLIST_INSERT_HEAD(&parent_bs->children, *child, next);
 -    /*
 -     * child is removed in bdrv_attach_child_common_abort(), so don't care to
 -     * abort this change separately.
 -     */
 -
- bool bdrv_is_sg(BlockDriverState *bs)
+     return 0;
- {
+ }
-     return bs->sg;
@@ -XXX,XX +XXX,XX @@ static void bdrv_remove_filter_or_cow_child_abort(void *opaque)
      BdrvRemoveFilterOrCowChild *s = opaque;
      BlockDriverState *parent_bs = s->child->opaque;
 -    QLIST_INSERT_HEAD(&parent_bs->children, s->child, next);
      if (s->is_backing) {
          parent_bs->backing = s->child;
      } else {
@@ -XXX,XX +XXX,XX @@ static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,
      };
      tran_add(tran, &bdrv_remove_filter_or_cow_child_drv, s);
 -    QLIST_SAFE_REMOVE(child, next);
      if (s->is_backing) {
          bs->backing = NULL;
      } else {
 --
-.9.3
+.33.1

-[Qemu-devel] [PULL 05/12] block: do not set BDS read_only if copy_on_read enabled
+[PULL v2 03/13] block: Unite remove_empty_child and child_free
-A few block drivers will set the BDS read_only flag from their
+Now that bdrv_remove_empty_child() no longer removes the child from the
-.bdrv_open() function.  This means the bs->read_only flag could
+parent's children list but only checks that it is not in such a list, it
-be set after we enable copy_on_read, as the BDRV_O_COPY_ON_READ
+is only a wrapper around bdrv_child_free() that checks that the child is
-flag check occurs prior to the call to bdrv->bdrv_open().
+empty and unused.  That should apply to all children that we free, so
 put those checks into bdrv_child_free() and drop
 bdrv_remove_empty_child().
-This adds an error return to bdrv_set_read_only(), and an error will be
+Signed-off-by: Hanna Reitz <hreitz@redhat.com>
-return if we try to set the BDS to read_only while copy_on_read is
+Reviewed-by: Kevin Wolf <kwolf@redhat.com>
-enabled.
+Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
+Message-Id: <20211111120829.81329-4-hreitz@redhat.com>
-This patch also changes the behavior of vvfat.  Before, vvfat could
+Signed-off-by: Kevin Wolf <kwolf@redhat.com>
-override the drive 'readonly' flag with its own, internal 'rw' flag.
+Message-Id: <20211115145409.176785-4-kwolf@redhat.com>
+Signed-off-by: Hanna Reitz <hreitz@redhat.com>
 For instance, this -drive parameter would result in a writable image:
 "-drive format=vvfat,dir=/tmp/vvfat,rw,if=virtio,readonly=on"
 This is not correct.  Now, attempting to use the above -drive parameter
 will result in an error (i.e., 'rw' is incompatible with 'readonly=on').
 Signed-off-by: Jeff Cody <jcody@redhat.com>
 Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
 Reviewed-by: John Snow <jsnow@redhat.com>
 Message-id: 0c5b4c1cc2c651471b131f21376dfd5ea24d2196.1491597120.git.jcody@redhat.com
 ---
- block.c               | 10 +++++++++-
+ block.c | 26 +++++++++++++-------------
- block/bochs.c         |  5 ++++-
+file changed, 13 insertions(+), 13 deletions(-)
  block/cloop.c         |  5 ++++-
  block/dmg.c           |  6 +++++-
  block/rbd.c           | 11 ++++++++++-
  block/vvfat.c         | 19 +++++++++++++++----
  include/block/block.h |  2 +-
 files changed, 48 insertions(+), 10 deletions(-)
 diff --git a/block.c b/block.c
 index XXXXXXX..XXXXXXX 100644
 --- a/block.c
 +++ b/block.c
-@@ -XXX,XX +XXX,XX @@ void path_combine(char *dest, int dest_size,
+@@ -XXX,XX +XXX,XX @@ static void bdrv_replace_child_noperm(BdrvChild *child,
      }
  }
--void bdrv_set_read_only(BlockDriverState *bs, bool read_only)
+-static void bdrv_child_free(void *opaque)
-+int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
+-{
 -    BdrvChild *c = opaque;
 -
 -    g_free(c->name);
 -    g_free(c);
 -}
 -
 -static void bdrv_remove_empty_child(BdrvChild *child)
 +/**
 + * Free the given @child.
 + *
 + * The child must be empty (i.e. `child->bs == NULL`) and it must be
 + * unused (i.e. not in a children list).
 + */
 +static void bdrv_child_free(BdrvChild *child)
  {
-+    /* Do not set read_only if copy_on_read is enabled */
+     assert(!child->bs);
-+    if (bs->copy_on_read && read_only) {
+     assert(!child->next.le_prev); /* not in children list */
-+        error_setg(errp, "Can't set node '%s' to r/o with copy-on-read enabled",
+-    bdrv_child_free(child);
 +                   bdrv_get_device_or_node_name(bs));
 +        return -EINVAL;
 +    }
 +
-     bs->read_only = read_only;
++    g_free(child->name);
-+    return 0;
++    g_free(child);
  }
- void bdrv_get_full_backing_filename_from_filename(const char *backed,
+ typedef struct BdrvAttachChildCommonState {
-diff --git a/block/bochs.c b/block/bochs.c
+@@ -XXX,XX +XXX,XX @@ static void bdrv_attach_child_common_abort(void *opaque)
 index XXXXXXX..XXXXXXX 100644
 --- a/block/bochs.c
 +++ b/block/bochs.c
@@ -XXX,XX +XXX,XX @@ static int bochs_open(BlockDriverState *bs, QDict *options, int flags,
          return -EINVAL;
      }
--    bdrv_set_read_only(bs, true); /* no write support yet */
+     bdrv_unref(bs);
-+    ret = bdrv_set_read_only(bs, true, errp); /* no write support yet */
+-    bdrv_remove_empty_child(child);
-+    if (ret < 0) {
++    bdrv_child_free(child);
-+        return ret;
+     *s->child = NULL;
-+    }
+ }
-     ret = bdrv_pread(bs->file, 0, &bochs, sizeof(bochs));
+@@ -XXX,XX +XXX,XX @@ static int bdrv_attach_child_common(BlockDriverState *child_bs,
-     if (ret < 0) {
-diff --git a/block/cloop.c b/block/cloop.c
+         if (ret < 0) {
-index XXXXXXX..XXXXXXX 100644
+             error_propagate(errp, local_err);
---- a/block/cloop.c
+-            bdrv_remove_empty_child(new_child);
-+++ b/block/cloop.c
++            bdrv_child_free(new_child);
-@@ -XXX,XX +XXX,XX @@ static int cloop_open(BlockDriverState *bs, QDict *options, int flags,
+             return ret;
-         return -EINVAL;
+         }
      }
+@@ -XXX,XX +XXX,XX @@ static void bdrv_detach_child(BdrvChild *child)
--    bdrv_set_read_only(bs, true);
+     BlockDriverState *old_bs = child->bs;
-+    ret = bdrv_set_read_only(bs, true, errp);
-+    if (ret < 0) {
+     bdrv_replace_child_noperm(child, NULL);
-+        return ret;
+-    bdrv_remove_empty_child(child);
-+    }
++    bdrv_child_free(child);
-     /* read header */
+     if (old_bs) {
-     ret = bdrv_pread(bs->file, 128, &s->block_size, 4);
+         /*
 diff --git a/block/dmg.c b/block/dmg.c
 index XXXXXXX..XXXXXXX 100644
 --- a/block/dmg.c
 +++ b/block/dmg.c
@@ -XXX,XX +XXX,XX @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
          return -EINVAL;
      }
 +    ret = bdrv_set_read_only(bs, true, errp);
 +    if (ret < 0) {
 +        return ret;
 +    }
 +
      block_module_load_one("dmg-bz2");
 -    bdrv_set_read_only(bs, true);
      s->n_chunks = 0;
      s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL;
 diff --git a/block/rbd.c b/block/rbd.c
 index XXXXXXX..XXXXXXX 100644
 --- a/block/rbd.c
 +++ b/block/rbd.c
@@ -XXX,XX +XXX,XX @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
          goto failed_shutdown;
      }
 +    /* rbd_open is always r/w */
      r = rbd_open(s->io_ctx, s->name, &s->image, s->snap);
      if (r < 0) {
          error_setg_errno(errp, -r, "error reading header from %s", s->name);
          goto failed_open;
      }
 -    bdrv_set_read_only(bs, (s->snap != NULL));
 +    /* If we are using an rbd snapshot, we must be r/o, otherwise
 +     * leave as-is */
 +    if (s->snap != NULL) {
 +        r = bdrv_set_read_only(bs, true, &local_err);
 +        if (r < 0) {
 +            error_propagate(errp, local_err);
 +            goto failed_open;
 +        }
 +    }
      qemu_opts_del(opts);
      return 0;
 diff --git a/block/vvfat.c b/block/vvfat.c
 index XXXXXXX..XXXXXXX 100644
 --- a/block/vvfat.c
 +++ b/block/vvfat.c
@@ -XXX,XX +XXX,XX @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
      s->current_cluster=0xffffffff;
 -    /* read only is the default for safety */
 -    bdrv_set_read_only(bs, true);
      s->qcow = NULL;
      s->qcow_filename = NULL;
      s->fat2 = NULL;
@@ -XXX,XX +XXX,XX @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
      s->sector_count = cyls * heads * secs - (s->first_sectors_number - 1);
      if (qemu_opt_get_bool(opts, "rw", false)) {
 -        ret = enable_write_target(bs, errp);
 +        if (!bdrv_is_read_only(bs)) {
 +            ret = enable_write_target(bs, errp);
 +            if (ret < 0) {
 +                goto fail;
 +            }
 +        } else {
 +            ret = -EPERM;
 +            error_setg(errp,
 +                       "Unable to set VVFAT to 'rw' when drive is read-only");
 +            goto fail;
 +        }
 +    } else  {
 +        /* read only is the default for safety */
 +        ret = bdrv_set_read_only(bs, true, &local_err);
          if (ret < 0) {
 +            error_propagate(errp, local_err);
              goto fail;
          }
 -        bdrv_set_read_only(bs, false);
      }
      bs->total_sectors = cyls * heads * secs;
 diff --git a/include/block/block.h b/include/block/block.h
 index XXXXXXX..XXXXXXX 100644
 --- a/include/block/block.h
 +++ b/include/block/block.h
@@ -XXX,XX +XXX,XX @@ int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
                              int64_t sector_num, int nb_sectors, int *pnum);
  bool bdrv_is_read_only(BlockDriverState *bs);
 -void bdrv_set_read_only(BlockDriverState *bs, bool read_only);
 +int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
  bool bdrv_is_sg(BlockDriverState *bs);
  bool bdrv_is_inserted(BlockDriverState *bs);
  int bdrv_media_changed(BlockDriverState *bs);
 --
-.9.3
+.33.1

-[Qemu-devel] [PULL 09/12] block: use bdrv_can_set_read_only() during reopen
+[PULL v2 04/13] block: Drop detached child from ignore list
-Signed-off-by: Jeff Cody <jcody@redhat.com>
+bdrv_attach_child_common_abort() restores the parent's AioContext.  To
-Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
+do so, the child (which was supposed to be attached, but is now detached
-Reviewed-by: John Snow <jsnow@redhat.com>
+again by this abort handler) is added to the ignore list for the
-Message-id: 00aed7ffdd7be4b9ed9ce1007d50028a72b34ebe.1491597120.git.jcody@redhat.com
+AioContext changing functions.
 However, since we modify a BDS's children list in the BdrvChildClass's
 .attach and .detach handlers, the child is already effectively detached
 from the parent by this point.  We do not need to put it into the ignore
 list.
 Use this opportunity to clean up the empty line structure: Keep setting
 the ignore list, invoking the AioContext function, and freeing the
 ignore list in blocks separated by empty lines.
 Signed-off-by: Hanna Reitz <hreitz@redhat.com>
 Reviewed-by: Kevin Wolf <kwolf@redhat.com>
 Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
 Message-Id: <20211111120829.81329-5-hreitz@redhat.com>
 Signed-off-by: Kevin Wolf <kwolf@redhat.com>
 Message-Id: <20211115145409.176785-5-kwolf@redhat.com>
 Signed-off-by: Hanna Reitz <hreitz@redhat.com>
 ---
- block.c | 14 ++++++++------
+ block.c | 8 +++++---
-file changed, 8 insertions(+), 6 deletions(-)
+file changed, 5 insertions(+), 3 deletions(-)
 diff --git a/block.c b/block.c
 index XXXXXXX..XXXXXXX 100644
 --- a/block.c
 +++ b/block.c
-@@ -XXX,XX +XXX,XX @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
+@@ -XXX,XX +XXX,XX @@ static void bdrv_attach_child_common_abort(void *opaque)
      BlockDriver *drv;
      QemuOpts *opts;
      const char *value;
 +    bool read_only;
      assert(reopen_state != NULL);
      assert(reopen_state->bs->drv != NULL);
@@ -XXX,XX +XXX,XX @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
          qdict_put(reopen_state->options, "driver", qstring_from_str(value));
      }
--    /* if we are to stay read-only, do not allow permission change
+     if (bdrv_child_get_parent_aio_context(child) != s->old_parent_ctx) {
--     * to r/w */
+-        GSList *ignore = g_slist_prepend(NULL, child);
--    if (!(reopen_state->bs->open_flags & BDRV_O_ALLOW_RDWR) &&
++        GSList *ignore;
--        reopen_state->flags & BDRV_O_RDWR) {
--        error_setg(errp, "Node '%s' is read only",
++        /* No need to ignore `child`, because it has been detached already */
--                   bdrv_get_device_or_node_name(reopen_state->bs));
++        ignore = NULL;
-+    /* If we are to stay read-only, do not allow permission change
+         child->klass->can_set_aio_ctx(child, s->old_parent_ctx, &ignore,
-+     * to r/w. Attempting to set to r/w may fail if either BDRV_O_ALLOW_RDWR is
+                                       &error_abort);
-+     * not set, or if the BDS still has copy_on_read enabled */
+         g_slist_free(ignore);
-+    read_only = !(reopen_state->flags & BDRV_O_RDWR);
+-        ignore = g_slist_prepend(NULL, child);
-+    ret = bdrv_can_set_read_only(reopen_state->bs, read_only, &local_err);
+-        child->klass->set_aio_ctx(child, s->old_parent_ctx, &ignore);
-+    if (local_err) {
-+        error_propagate(errp, local_err);
++        ignore = NULL;
-         goto error;
++        child->klass->set_aio_ctx(child, s->old_parent_ctx, &ignore);
          g_slist_free(ignore);
      }
 --
-.9.3
+.33.1

-[Qemu-devel] [PULL 08/12] block: introduce bdrv_can_set_read_only()
+[PULL v2 05/13] block: Pass BdrvChild ** to replace_child_noperm
-Introduce check function for setting read_only flags.  Will return < 0 on
+bdrv_replace_child_noperm() modifies BdrvChild.bs, and can potentially
-error, with appropriate Error value set.  Does not alter any flags.
+set it to NULL.  That is dangerous, because BDS parents generally assume
 that their children's .bs pointer is never NULL.  We therefore want to
 let bdrv_replace_child_noperm() set the corresponding BdrvChild pointer
 to NULL, too.
-Signed-off-by: Jeff Cody <jcody@redhat.com>
+This patch lays the foundation for it by passing a BdrvChild ** pointer
-Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
+to bdrv_replace_child_noperm() so that it can later use it to NULL the
-Reviewed-by: John Snow <jsnow@redhat.com>
+BdrvChild pointer immediately after setting BdrvChild.bs to NULL.
-Message-id: e2bba34ac3bc76a0c42adc390413f358ae0566e8.1491597120.git.jcody@redhat.com
 (We will still need to undertake some intermediate steps, though.)
 Signed-off-by: Hanna Reitz <hreitz@redhat.com>
 Message-Id: <20211111120829.81329-6-hreitz@redhat.com>
 Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
 Signed-off-by: Kevin Wolf <kwolf@redhat.com>
 Message-Id: <20211115145409.176785-6-kwolf@redhat.com>
 Signed-off-by: Hanna Reitz <hreitz@redhat.com>
 ---
- block.c               | 14 +++++++++++++-
+ block.c | 23 ++++++++++++-----------
- include/block/block.h |  1 +
+file changed, 12 insertions(+), 11 deletions(-)
 files changed, 14 insertions(+), 1 deletion(-)
 diff --git a/block.c b/block.c
 index XXXXXXX..XXXXXXX 100644
 --- a/block.c
 +++ b/block.c
-@@ -XXX,XX +XXX,XX @@ bool bdrv_is_read_only(BlockDriverState *bs)
+@@ -XXX,XX +XXX,XX @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
-     return bs->read_only;
+ static bool bdrv_recurse_has_child(BlockDriverState *bs,
                                     BlockDriverState *child);
 -static void bdrv_replace_child_noperm(BdrvChild *child,
 +static void bdrv_replace_child_noperm(BdrvChild **child,
                                        BlockDriverState *new_bs);
  static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,
                                                BdrvChild *child,
@@ -XXX,XX +XXX,XX @@ static void bdrv_replace_child_abort(void *opaque)
      BlockDriverState *new_bs = s->child->bs;
      /* old_bs reference is transparently moved from @s to @s->child */
 -    bdrv_replace_child_noperm(s->child, s->old_bs);
 +    bdrv_replace_child_noperm(&s->child, s->old_bs);
      bdrv_unref(new_bs);
  }
--int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
+@@ -XXX,XX +XXX,XX @@ static void bdrv_replace_child_tran(BdrvChild *child, BlockDriverState *new_bs,
-+int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
+     if (new_bs) {
          bdrv_ref(new_bs);
      }
 -    bdrv_replace_child_noperm(child, new_bs);
 +    bdrv_replace_child_noperm(&child, new_bs);
      /* old_bs reference is transparently moved from @child to @s */
  }
@@ -XXX,XX +XXX,XX @@ uint64_t bdrv_qapi_perm_to_blk_perm(BlockPermission qapi_perm)
      return permissions[qapi_perm];
  }
 -static void bdrv_replace_child_noperm(BdrvChild *child,
 +static void bdrv_replace_child_noperm(BdrvChild **childp,
                                        BlockDriverState *new_bs)
  {
-     /* Do not set read_only if copy_on_read is enabled */
++    BdrvChild *child = *childp;
-     if (bs->copy_on_read && read_only) {
+     BlockDriverState *old_bs = child->bs;
-@@ -XXX,XX +XXX,XX @@ int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
+     int new_bs_quiesce_counter;
-         return -EPERM;
+     int drain_saldo;
@@ -XXX,XX +XXX,XX @@ static void bdrv_attach_child_common_abort(void *opaque)
      BdrvChild *child = *s->child;
      BlockDriverState *bs = child->bs;
 -    bdrv_replace_child_noperm(child, NULL);
 +    bdrv_replace_child_noperm(s->child, NULL);
      if (bdrv_get_aio_context(bs) != s->old_child_ctx) {
          bdrv_try_set_aio_context(bs, s->old_child_ctx, &error_abort);
@@ -XXX,XX +XXX,XX @@ static int bdrv_attach_child_common(BlockDriverState *child_bs,
      }
-+    return 0;
+     bdrv_ref(child_bs);
-+}
+-    bdrv_replace_child_noperm(new_child, child_bs);
-+
++    bdrv_replace_child_noperm(&new_child, child_bs);
-+int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
-+{
+     *child = new_child;
-+    int ret = 0;
-+
+@@ -XXX,XX +XXX,XX @@ static int bdrv_attach_child_noperm(BlockDriverState *parent_bs,
 +    ret = bdrv_can_set_read_only(bs, read_only, errp);
 +    if (ret < 0) {
 +        return ret;
 +    }
 +
      bs->read_only = read_only;
      return 0;
  }
-diff --git a/include/block/block.h b/include/block/block.h
-index XXXXXXX..XXXXXXX 100644
+-static void bdrv_detach_child(BdrvChild *child)
---- a/include/block/block.h
++static void bdrv_detach_child(BdrvChild **childp)
-+++ b/include/block/block.h
+ {
-@@ -XXX,XX +XXX,XX @@ int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
+-    BlockDriverState *old_bs = child->bs;
-                             int64_t sector_num, int nb_sectors, int *pnum);
++    BlockDriverState *old_bs = (*childp)->bs;
- bool bdrv_is_read_only(BlockDriverState *bs);
+-    bdrv_replace_child_noperm(child, NULL);
-+int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
+-    bdrv_child_free(child);
- int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
++    bdrv_replace_child_noperm(childp, NULL);
- bool bdrv_is_sg(BlockDriverState *bs);
++    bdrv_child_free(*childp);
- bool bdrv_is_inserted(BlockDriverState *bs);
      if (old_bs) {
          /*
@@ -XXX,XX +XXX,XX @@ void bdrv_root_unref_child(BdrvChild *child)
      BlockDriverState *child_bs;
      child_bs = child->bs;
 -    bdrv_detach_child(child);
 +    bdrv_detach_child(&child);
      bdrv_unref(child_bs);
  }
 --
-.9.3
+.33.1

-[Qemu-devel] [PULL 06/12] block: honor BDRV_O_ALLOW_RDWR when clearing bs->read_only
+[PULL v2 06/13] block: Restructure remove_file_or_backing_child()
-The BDRV_O_ALLOW_RDWR flag allows / prohibits the changing of
+As of a future patch, bdrv_replace_child_tran() will take a BdrvChild **
-the BDS 'read_only' state, but there are a few places where it
+pointer.  Prepare for that by getting such a pointer and using it where
-is ignored.  In the bdrv_set_read_only() helper, make sure to
+applicable, and (dereferenced) as a parameter for
-honor the flag.
+bdrv_replace_child_tran().
-Signed-off-by: Jeff Cody <jcody@redhat.com>
+Signed-off-by: Hanna Reitz <hreitz@redhat.com>
-Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
+Message-Id: <20211111120829.81329-7-hreitz@redhat.com>
-Reviewed-by: John Snow <jsnow@redhat.com>
+Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
-Message-id: be2e5fb2d285cbece2b6d06bed54a6f56520d251.1491597120.git.jcody@redhat.com
+Signed-off-by: Kevin Wolf <kwolf@redhat.com>
 Message-Id: <20211115145409.176785-7-kwolf@redhat.com>
 Signed-off-by: Hanna Reitz <hreitz@redhat.com>
 ---
- block.c | 7 +++++++
+ block.c | 21 ++++++++++++---------
-file changed, 7 insertions(+)
+file changed, 12 insertions(+), 9 deletions(-)
 diff --git a/block.c b/block.c
 index XXXXXXX..XXXXXXX 100644
 --- a/block.c
 +++ b/block.c
-@@ -XXX,XX +XXX,XX @@ int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
+@@ -XXX,XX +XXX,XX @@ static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,
-         return -EINVAL;
+                                               BdrvChild *child,
                                                Transaction *tran)
  {
 +    BdrvChild **childp;
      BdrvRemoveFilterOrCowChild *s;
 -    assert(child == bs->backing || child == bs->file);
 -
      if (!child) {
          return;
      }
-+    /* Do not clear read_only if it is prohibited */
++    if (child == bs->backing) {
-+    if (!read_only && !(bs->open_flags & BDRV_O_ALLOW_RDWR)) {
++        childp = &bs->backing;
-+        error_setg(errp, "Node '%s' is read only",
++    } else if (child == bs->file) {
-+                   bdrv_get_device_or_node_name(bs));
++        childp = &bs->file;
-+        return -EPERM;
++    } else {
 +        g_assert_not_reached();
 +    }
 +
-     bs->read_only = read_only;
+     if (child->bs) {
-     return 0;
+-        bdrv_replace_child_tran(child, NULL, tran);
 +        bdrv_replace_child_tran(*childp, NULL, tran);
      }
      s = g_new(BdrvRemoveFilterOrCowChild, 1);
      *s = (BdrvRemoveFilterOrCowChild) {
          .child = child,
 -        .is_backing = (child == bs->backing),
 +        .is_backing = (childp == &bs->backing),
      };
      tran_add(tran, &bdrv_remove_filter_or_cow_child_drv, s);
 -    if (s->is_backing) {
 -        bs->backing = NULL;
 -    } else {
 -        bs->file = NULL;
 -    }
 +    *childp = NULL;
  }
+ /*
 --
-.9.3
+.33.1

-[Qemu-devel] [PULL 02/12] block/vxhs.c: Add qemu-iotests for new block device type "vxhs"
+[PULL v2 07/13] transactions: Invoke clean() after everything else
-From: Ashish Mittal <ashmit602@gmail.com>
+Invoke the transaction drivers' .clean() methods only after all
 .commit() or .abort() handlers are done.
-These changes use a vxhs test server that is a part of the following
+This makes it easier to have nested transactions where the top-level
-repository:
+transactions pass objects to lower transactions that the latter can
-https://github.com/VeritasHyperScale/libqnio.git
+still use throughout their commit/abort phases, while the top-level
 transaction keeps a reference that is released in its .clean() method.
-Signed-off-by: Ashish Mittal <Ashish.Mittal@veritas.com>
+(Before this commit, that is also possible, but the top-level
-Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
+transaction would need to take care to invoke tran_add() before the
-Reviewed-by: Jeff Cody <jcody@redhat.com>
+lower-level transaction does.  This commit makes the ordering
-Signed-off-by: Jeff Cody <jcody@redhat.com>
+irrelevant, which is just a bit nicer.)
-Message-id: 1491277689-24949-3-git-send-email-Ashish.Mittal@veritas.com
 Signed-off-by: Hanna Reitz <hreitz@redhat.com>
 Message-Id: <20211111120829.81329-8-hreitz@redhat.com>
 Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
 Signed-off-by: Kevin Wolf <kwolf@redhat.com>
 Message-Id: <20211115145409.176785-8-kwolf@redhat.com>
 Signed-off-by: Hanna Reitz <hreitz@redhat.com>
 ---
- tests/qemu-iotests/common        |  6 ++++++
+ include/qemu/transactions.h | 3 +++
- tests/qemu-iotests/common.config | 13 +++++++++++++
+ util/transactions.c         | 8 ++++++--
- tests/qemu-iotests/common.filter |  1 +
+files changed, 9 insertions(+), 2 deletions(-)
  tests/qemu-iotests/common.rc     | 19 +++++++++++++++++++
 files changed, 39 insertions(+)
-diff --git a/tests/qemu-iotests/common b/tests/qemu-iotests/common
+diff --git a/include/qemu/transactions.h b/include/qemu/transactions.h
 index XXXXXXX..XXXXXXX 100644
---- a/tests/qemu-iotests/common
+--- a/include/qemu/transactions.h
-+++ b/tests/qemu-iotests/common
++++ b/include/qemu/transactions.h
-@@ -XXX,XX +XXX,XX @@ check options
+@@ -XXX,XX +XXX,XX @@
-     -ssh                test ssh
+  * tran_create(), call your "prepare" functions on it, and finally call
-     -nfs                test nfs
+  * tran_abort() or tran_commit() to finalize the transaction by corresponding
-     -luks               test luks
+  * finalization actions in reverse order.
-+    -vxhs               test vxhs
++ *
-     -xdiff              graphical mode diff
++ * The clean() functions registered by the drivers in a transaction are called
-     -nocache            use O_DIRECT on backing file
++ * last, after all abort() or commit() functions have been called.
-     -misalign           misalign memory allocations
+  */
-@@ -XXX,XX +XXX,XX @@ testlist options
-             xpand=false
+ #ifndef QEMU_TRANSACTIONS_H
-             ;;
+diff --git a/util/transactions.c b/util/transactions.c
 +        -vxhs)
 +            IMGPROTO=vxhs
 +            xpand=false
 +            ;;
 +
          -ssh)
              IMGPROTO=ssh
              xpand=false
 diff --git a/tests/qemu-iotests/common.config b/tests/qemu-iotests/common.config
 index XXXXXXX..XXXXXXX 100644
---- a/tests/qemu-iotests/common.config
+--- a/util/transactions.c
-+++ b/tests/qemu-iotests/common.config
++++ b/util/transactions.c
-@@ -XXX,XX +XXX,XX @@ if [ -z "$QEMU_NBD_PROG" ]; then
+@@ -XXX,XX +XXX,XX @@ void tran_abort(Transaction *tran)
      export QEMU_NBD_PROG="`set_prog_path qemu-nbd`"
  fi
 +if [ -z "$QEMU_VXHS_PROG" ]; then
 +    export QEMU_VXHS_PROG="`set_prog_path qnio_server`"
 +fi
 +
  _qemu_wrapper()
  {
-     (
+     TransactionAction *act, *next;
-@@ -XXX,XX +XXX,XX @@ _qemu_nbd_wrapper()
-     )
+-    QSLIST_FOREACH_SAFE(act, &tran->actions, entry, next) {
- }
++    QSLIST_FOREACH(act, &tran->actions, entry) {
+         if (act->drv->abort) {
-+_qemu_vxhs_wrapper()
+             act->drv->abort(act->opaque);
-+{
+         }
-+    (
++    }
-+        echo $BASHPID > "${TEST_DIR}/qemu-vxhs.pid"
-+        exec "$QEMU_VXHS_PROG" $QEMU_VXHS_OPTIONS "$@"
++    QSLIST_FOREACH_SAFE(act, &tran->actions, entry, next) {
-+    )
+         if (act->drv->clean) {
-+}
+             act->drv->clean(act->opaque);
-+
+         }
- export QEMU=_qemu_wrapper
+@@ -XXX,XX +XXX,XX @@ void tran_commit(Transaction *tran)
- export QEMU_IMG=_qemu_img_wrapper
+ {
- export QEMU_IO=_qemu_io_wrapper
+     TransactionAction *act, *next;
- export QEMU_NBD=_qemu_nbd_wrapper
-+export QEMU_VXHS=_qemu_vxhs_wrapper
+-    QSLIST_FOREACH_SAFE(act, &tran->actions, entry, next) {
++    QSLIST_FOREACH(act, &tran->actions, entry) {
- QEMU_IMG_EXTRA_ARGS=
+         if (act->drv->commit) {
- if [ "$IMGOPTSSYNTAX" = "true" ]; then
+             act->drv->commit(act->opaque);
-diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
+         }
-index XXXXXXX..XXXXXXX 100644
++    }
---- a/tests/qemu-iotests/common.filter
-+++ b/tests/qemu-iotests/common.filter
++    QSLIST_FOREACH_SAFE(act, &tran->actions, entry, next) {
-@@ -XXX,XX +XXX,XX @@ _filter_img_info()
+         if (act->drv->clean) {
-         -e "s#$TEST_DIR#TEST_DIR#g" \
+             act->drv->clean(act->opaque);
-         -e "s#$IMGFMT#IMGFMT#g" \
+         }
          -e 's#nbd://127.0.0.1:10810$#TEST_DIR/t.IMGFMT#g' \
 +        -e 's#json.*vdisk-id.*vxhs"}}#TEST_DIR/t.IMGFMT#' \
          -e "/encrypted: yes/d" \
          -e "/cluster_size: [0-9]\\+/d" \
          -e "/table_size: [0-9]\\+/d" \
 diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
 index XXXXXXX..XXXXXXX 100644
 --- a/tests/qemu-iotests/common.rc
 +++ b/tests/qemu-iotests/common.rc
@@ -XXX,XX +XXX,XX @@ else
      elif [ "$IMGPROTO" = "nfs" ]; then
          TEST_DIR="nfs://127.0.0.1/$TEST_DIR"
          TEST_IMG=$TEST_DIR/t.$IMGFMT
 +    elif [ "$IMGPROTO" = "vxhs" ]; then
 +        TEST_IMG_FILE=$TEST_DIR/t.$IMGFMT
 +        TEST_IMG="vxhs://127.0.0.1:9999/t.$IMGFMT"
      else
          TEST_IMG=$IMGPROTO:$TEST_DIR/t.$IMGFMT
      fi
@@ -XXX,XX +XXX,XX @@ _make_test_img()
          eval "$QEMU_NBD -v -t -b 127.0.0.1 -p 10810 -f $IMGFMT  $TEST_IMG_FILE >/dev/null &"
          sleep 1 # FIXME: qemu-nbd needs to be listening before we continue
      fi
 +
 +    # Start QNIO server on image directory for vxhs protocol
 +    if [ $IMGPROTO = "vxhs" ]; then
 +        eval "$QEMU_VXHS -d  $TEST_DIR > /dev/null &"
 +        sleep 1 # Wait for server to come up.
 +    fi
  }
  _rm_test_img()
@@ -XXX,XX +XXX,XX @@ _cleanup_test_img()
              fi
              rm -f "$TEST_IMG_FILE"
              ;;
 +        vxhs)
 +            if [ -f "${TEST_DIR}/qemu-vxhs.pid" ]; then
 +                local QEMU_VXHS_PID
 +                read QEMU_VXHS_PID < "${TEST_DIR}/qemu-vxhs.pid"
 +                kill ${QEMU_VXHS_PID} >/dev/null 2>&1
 +                rm -f "${TEST_DIR}/qemu-vxhs.pid"
 +            fi
 +            rm -f "$TEST_IMG_FILE"
 +            ;;
 +
          file)
              _rm_test_img "$TEST_DIR/t.$IMGFMT"
              _rm_test_img "$TEST_DIR/t.$IMGFMT.orig"
 --
-.9.3
+.33.1

-[Qemu-devel] [PULL 04/12] block: add bdrv_set_read_only() helper function
+[PULL v2 08/13] block: Let replace_child_tran keep indirect pointer
-We have a helper wrapper for checking for the BDS read_only flag,
+As of a future commit, bdrv_replace_child_noperm() will clear the
-add a helper wrapper to set the read_only flag as well.
+indirect BdrvChild pointer passed to it if the new child BDS is NULL.
+bdrv_replace_child_tran() will want to let it do that, but revert this
-Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
+change in its abort handler.  For that, we need to have it receive a
-Signed-off-by: Jeff Cody <jcody@redhat.com>
+BdrvChild ** pointer, too, and keep it stored in the
-Reviewed-by: John Snow <jsnow@redhat.com>
+BdrvReplaceChildState object that we attach to the transaction.
-Message-id: 9b18972d05f5fa2ac16c014f0af98d680553048d.1491597120.git.jcody@redhat.com
 Note that we do not need to store it in the BdrvReplaceChildState when
 new_bs is not NULL, because then there is nothing to revert.  This is
 important so that bdrv_replace_node_noperm() can pass a pointer to a
 loop-local variable to bdrv_replace_child_tran() without worrying that
 this pointer will outlive one loop iteration.
 (Of course, for that to work, bdrv_replace_node_noperm() and in turn
 bdrv_replace_node() and its relatives may not be called with a NULL @to
 node.  Luckily, they already are not, but now we should assert this.)
 bdrv_remove_file_or_backing_child() on the other hand needs to ensure
 that the indirect pointer it passes will stay valid for the duration of
 the transaction.  Ensure this by keeping a strong reference to the BDS
 whose &bs->backing or &bs->file it passes to bdrv_replace_child_tran(),
 and giving up that reference only in the transaction .clean() handler.
 Signed-off-by: Hanna Reitz <hreitz@redhat.com>
 Message-Id: <20211111120829.81329-9-hreitz@redhat.com>
 Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
 Signed-off-by: Kevin Wolf <kwolf@redhat.com>
 Message-Id: <20211115145409.176785-9-kwolf@redhat.com>
 Signed-off-by: Hanna Reitz <hreitz@redhat.com>
 ---
- block.c               | 5 +++++
+ block.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++-------
- block/bochs.c         | 2 +-
+file changed, 73 insertions(+), 10 deletions(-)
  block/cloop.c         | 2 +-
  block/dmg.c           | 2 +-
  block/rbd.c           | 2 +-
  block/vvfat.c         | 4 ++--
  include/block/block.h | 1 +
 files changed, 12 insertions(+), 6 deletions(-)
 diff --git a/block.c b/block.c
 index XXXXXXX..XXXXXXX 100644
 --- a/block.c
 +++ b/block.c
-@@ -XXX,XX +XXX,XX @@ void path_combine(char *dest, int dest_size,
+@@ -XXX,XX +XXX,XX @@ static int bdrv_drv_set_perm(BlockDriverState *bs, uint64_t perm,
-     }
- }
+ typedef struct BdrvReplaceChildState {
+     BdrvChild *child;
-+void bdrv_set_read_only(BlockDriverState *bs, bool read_only)
++    BdrvChild **childp;
      BlockDriverState *old_bs;
  } BdrvReplaceChildState;
@@ -XXX,XX +XXX,XX @@ static void bdrv_replace_child_abort(void *opaque)
      BdrvReplaceChildState *s = opaque;
      BlockDriverState *new_bs = s->child->bs;
 -    /* old_bs reference is transparently moved from @s to @s->child */
 +    /*
 +     * old_bs reference is transparently moved from @s to s->child.
 +     *
 +     * Pass &s->child here instead of s->childp, because:
 +     * (1) s->old_bs must be non-NULL, so bdrv_replace_child_noperm() will not
 +     *     modify the BdrvChild * pointer we indirectly pass to it, i.e. it
 +     *     will not modify s->child.  From that perspective, it does not matter
 +     *     whether we pass s->childp or &s->child.
 +     *     (TODO: Right now, bdrv_replace_child_noperm() never modifies that
 +     *     pointer anyway (though it will in the future), so at this point it
 +     *     absolutely does not matter whether we pass s->childp or &s->child.)
 +     * (2) If new_bs is not NULL, s->childp will be NULL.  We then cannot use
 +     *     it here.
 +     * (3) If new_bs is NULL, *s->childp will have been NULLed by
 +     *     bdrv_replace_child_tran()'s bdrv_replace_child_noperm() call, and we
 +     *     must not pass a NULL *s->childp here.
 +     *     (TODO: In its current state, bdrv_replace_child_noperm() will not
 +     *     have NULLed *s->childp, so this does not apply yet.  It will in the
 +     *     future.)
 +     *
 +     * So whether new_bs was NULL or not, we cannot pass s->childp here; and in
 +     * any case, there is no reason to pass it anyway.
 +     */
      bdrv_replace_child_noperm(&s->child, s->old_bs);
      bdrv_unref(new_bs);
  }
@@ -XXX,XX +XXX,XX @@ static TransactionActionDrv bdrv_replace_child_drv = {
   * Note: real unref of old_bs is done only on commit.
   *
   * The function doesn't update permissions, caller is responsible for this.
 + *
 + * Note that if new_bs == NULL, @childp is stored in a state object attached
 + * to @tran, so that the old child can be reinstated in the abort handler.
 + * Therefore, if @new_bs can be NULL, @childp must stay valid until the
 + * transaction is committed or aborted.
 + *
 + * (TODO: The reinstating does not happen yet, but it will once
 + * bdrv_replace_child_noperm() NULLs *childp when new_bs is NULL.)
   */
 -static void bdrv_replace_child_tran(BdrvChild *child, BlockDriverState *new_bs,
 +static void bdrv_replace_child_tran(BdrvChild **childp,
 +                                    BlockDriverState *new_bs,
                                      Transaction *tran)
  {
      BdrvReplaceChildState *s = g_new(BdrvReplaceChildState, 1);
      *s = (BdrvReplaceChildState) {
 -        .child = child,
 -        .old_bs = child->bs,
 +        .child = *childp,
 +        .childp = new_bs == NULL ? childp : NULL,
 +        .old_bs = (*childp)->bs,
      };
      tran_add(tran, &bdrv_replace_child_drv, s);
      if (new_bs) {
          bdrv_ref(new_bs);
      }
 -    bdrv_replace_child_noperm(&child, new_bs);
 -    /* old_bs reference is transparently moved from @child to @s */
 +    bdrv_replace_child_noperm(childp, new_bs);
 +    /* old_bs reference is transparently moved from *childp to @s */
  }
  /*
@@ -XXX,XX +XXX,XX @@ static bool should_update_child(BdrvChild *c, BlockDriverState *to)
  typedef struct BdrvRemoveFilterOrCowChild {
      BdrvChild *child;
 +    BlockDriverState *bs;
      bool is_backing;
  } BdrvRemoveFilterOrCowChild;
@@ -XXX,XX +XXX,XX @@ static void bdrv_remove_filter_or_cow_child_commit(void *opaque)
      bdrv_child_free(s->child);
  }
 +static void bdrv_remove_filter_or_cow_child_clean(void *opaque)
 +{
-+    bs->read_only = read_only;
++    BdrvRemoveFilterOrCowChild *s = opaque;
 +
 +    /* Drop the bs reference after the transaction is done */
 +    bdrv_unref(s->bs);
 +    g_free(s);
 +}
 +
- void bdrv_get_full_backing_filename_from_filename(const char *backed,
+ static TransactionActionDrv bdrv_remove_filter_or_cow_child_drv = {
-                                                   const char *backing,
+     .abort = bdrv_remove_filter_or_cow_child_abort,
-                                                   char *dest, size_t sz,
+     .commit = bdrv_remove_filter_or_cow_child_commit,
-diff --git a/block/bochs.c b/block/bochs.c
+-    .clean = g_free,
-index XXXXXXX..XXXXXXX 100644
++    .clean = bdrv_remove_filter_or_cow_child_clean,
---- a/block/bochs.c
+ };
-+++ b/block/bochs.c
-@@ -XXX,XX +XXX,XX @@ static int bochs_open(BlockDriverState *bs, QDict *options, int flags,
+ /*
-         return -EINVAL;
+@@ -XXX,XX +XXX,XX @@ static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,
-     }
+         return;
+     }
--    bs->read_only = true; /* no write support yet */
-+    bdrv_set_read_only(bs, true); /* no write support yet */
++    /*
++     * Keep a reference to @bs so @childp will stay valid throughout the
-     ret = bdrv_pread(bs->file, 0, &bochs, sizeof(bochs));
++     * transaction (required by bdrv_replace_child_tran())
-     if (ret < 0) {
++     */
-diff --git a/block/cloop.c b/block/cloop.c
++    bdrv_ref(bs);
-index XXXXXXX..XXXXXXX 100644
+     if (child == bs->backing) {
---- a/block/cloop.c
+         childp = &bs->backing;
-+++ b/block/cloop.c
+     } else if (child == bs->file) {
-@@ -XXX,XX +XXX,XX @@ static int cloop_open(BlockDriverState *bs, QDict *options, int flags,
+@@ -XXX,XX +XXX,XX @@ static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,
-         return -EINVAL;
+     }
-     }
+     if (child->bs) {
--    bs->read_only = true;
+-        bdrv_replace_child_tran(*childp, NULL, tran);
-+    bdrv_set_read_only(bs, true);
++        bdrv_replace_child_tran(childp, NULL, tran);
+     }
-     /* read header */
-     ret = bdrv_pread(bs->file, 128, &s->block_size, 4);
+     s = g_new(BdrvRemoveFilterOrCowChild, 1);
-diff --git a/block/dmg.c b/block/dmg.c
+     *s = (BdrvRemoveFilterOrCowChild) {
-index XXXXXXX..XXXXXXX 100644
+         .child = child,
---- a/block/dmg.c
++        .bs = bs,
-+++ b/block/dmg.c
+         .is_backing = (childp == &bs->backing),
-@@ -XXX,XX +XXX,XX @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
+     };
-     }
+     tran_add(tran, &bdrv_remove_filter_or_cow_child_drv, s);
+@@ -XXX,XX +XXX,XX @@ static int bdrv_replace_node_noperm(BlockDriverState *from,
-     block_module_load_one("dmg-bz2");
+ {
--    bs->read_only = true;
+     BdrvChild *c, *next;
-+    bdrv_set_read_only(bs, true);
++    assert(to != NULL);
-     s->n_chunks = 0;
++
-     s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL;
+     QLIST_FOREACH_SAFE(c, &from->parents, next_parent, next) {
-diff --git a/block/rbd.c b/block/rbd.c
+         assert(c->bs == from);
-index XXXXXXX..XXXXXXX 100644
+         if (!should_update_child(c, to)) {
---- a/block/rbd.c
+@@ -XXX,XX +XXX,XX @@ static int bdrv_replace_node_noperm(BlockDriverState *from,
-+++ b/block/rbd.c
+                        c->name, from->node_name);
-@@ -XXX,XX +XXX,XX @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
+             return -EPERM;
-         goto failed_open;
+         }
-     }
+-        bdrv_replace_child_tran(c, to, tran);
++
--    bs->read_only = (s->snap != NULL);
++        /*
-+    bdrv_set_read_only(bs, (s->snap != NULL));
++         * Passing a pointer to the local variable @c is fine here, because
++         * @to is not NULL, and so &c will not be attached to the transaction.
-     qemu_opts_del(opts);
++         */
 +        bdrv_replace_child_tran(&c, to, tran);
      }
      return 0;
-diff --git a/block/vvfat.c b/block/vvfat.c
+@@ -XXX,XX +XXX,XX @@ static int bdrv_replace_node_noperm(BlockDriverState *from,
-index XXXXXXX..XXXXXXX 100644
+  *
---- a/block/vvfat.c
+  * With @detach_subchain=true @to must be in a backing chain of @from. In this
-+++ b/block/vvfat.c
+  * case backing link of the cow-parent of @to is removed.
-@@ -XXX,XX +XXX,XX @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
++ *
-     s->current_cluster=0xffffffff;
++ * @to must not be NULL.
+  */
-     /* read only is the default for safety */
+ static int bdrv_replace_node_common(BlockDriverState *from,
--    bs->read_only = true;
+                                     BlockDriverState *to,
-+    bdrv_set_read_only(bs, true);
+@@ -XXX,XX +XXX,XX @@ static int bdrv_replace_node_common(BlockDriverState *from,
-     s->qcow = NULL;
+     BlockDriverState *to_cow_parent = NULL;
-     s->qcow_filename = NULL;
+     int ret;
-     s->fat2 = NULL;
-@@ -XXX,XX +XXX,XX @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
++    assert(to != NULL);
-         if (ret < 0) {
++
-             goto fail;
+     if (detach_subchain) {
-         }
+         assert(bdrv_chain_contains(from, to));
--        bs->read_only = false;
+         assert(from != to);
-+        bdrv_set_read_only(bs, false);
+@@ -XXX,XX +XXX,XX @@ out:
-     }
+     return ret;
+ }
-     bs->total_sectors = cyls * heads * secs;
-diff --git a/include/block/block.h b/include/block/block.h
++/**
-index XXXXXXX..XXXXXXX 100644
++ * Replace node @from by @to (where neither may be NULL).
---- a/include/block/block.h
++ */
-+++ b/include/block/block.h
+ int bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
-@@ -XXX,XX +XXX,XX @@ int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
+                       Error **errp)
-                             int64_t sector_num, int nb_sectors, int *pnum);
+ {
+@@ -XXX,XX +XXX,XX @@ int bdrv_replace_child_bs(BdrvChild *child, BlockDriverState *new_bs,
- bool bdrv_is_read_only(BlockDriverState *bs);
+     bdrv_drained_begin(old_bs);
-+void bdrv_set_read_only(BlockDriverState *bs, bool read_only);
+     bdrv_drained_begin(new_bs);
- bool bdrv_is_sg(BlockDriverState *bs);
- bool bdrv_is_inserted(BlockDriverState *bs);
+-    bdrv_replace_child_tran(child, new_bs, tran);
- int bdrv_media_changed(BlockDriverState *bs);
++    bdrv_replace_child_tran(&child, new_bs, tran);
      found = g_hash_table_new(NULL, NULL);
      refresh_list = bdrv_topological_dfs(refresh_list, found, old_bs);
 --
-.9.3
+.33.1

-New patch
+[PULL v2 09/13] block: Let replace_child_noperm free children
+In most of the block layer, especially when traversing down from other
 BlockDriverStates, we assume that BdrvChild.bs can never be NULL.  When
 it becomes NULL, it is expected that the corresponding BdrvChild pointer
 also becomes NULL and the BdrvChild object is freed.
 Therefore, once bdrv_replace_child_noperm() sets the BdrvChild.bs
 pointer to NULL, it should also immediately set the corresponding
 BdrvChild pointer (like bs->file or bs->backing) to NULL.
 In that context, it also makes sense for this function to free the
 child.  Sometimes we cannot do so, though, because it is called in a
 transactional context where the caller might still want to reinstate the
 child in the abort branch (and free it only on commit), so this behavior
 has to remain optional.
 In bdrv_replace_child_tran()'s abort handler, we now rely on the fact
 that the BdrvChild passed to bdrv_replace_child_tran() must have had a
 non-NULL .bs pointer initially.  Make a note of that and assert it.
 Signed-off-by: Hanna Reitz <hreitz@redhat.com>
 Message-Id: <20211111120829.81329-10-hreitz@redhat.com>
 Signed-off-by: Kevin Wolf <kwolf@redhat.com>
 Message-Id: <20211115145409.176785-10-kwolf@redhat.com>
 Signed-off-by: Hanna Reitz <hreitz@redhat.com>
 ---
  block.c | 102 +++++++++++++++++++++++++++++++++++++++++++-------------
 file changed, 79 insertions(+), 23 deletions(-)
 diff --git a/block.c b/block.c
 index XXXXXXX..XXXXXXX 100644
 --- a/block.c
 +++ b/block.c
@@ -XXX,XX +XXX,XX @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
  static bool bdrv_recurse_has_child(BlockDriverState *bs,
                                     BlockDriverState *child);
 +static void bdrv_child_free(BdrvChild *child);
  static void bdrv_replace_child_noperm(BdrvChild **child,
 -                                      BlockDriverState *new_bs);
 +                                      BlockDriverState *new_bs,
 +                                      bool free_empty_child);
  static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,
                                                BdrvChild *child,
                                                Transaction *tran);
@@ -XXX,XX +XXX,XX @@ typedef struct BdrvReplaceChildState {
      BdrvChild *child;
      BdrvChild **childp;
      BlockDriverState *old_bs;
 +    bool free_empty_child;
  } BdrvReplaceChildState;
  static void bdrv_replace_child_commit(void *opaque)
  {
      BdrvReplaceChildState *s = opaque;
 +    if (s->free_empty_child && !s->child->bs) {
 +        bdrv_child_free(s->child);
 +    }
      bdrv_unref(s->old_bs);
  }
@@ -XXX,XX +XXX,XX @@ static void bdrv_replace_child_abort(void *opaque)
       *     modify the BdrvChild * pointer we indirectly pass to it, i.e. it
       *     will not modify s->child.  From that perspective, it does not matter
       *     whether we pass s->childp or &s->child.
 -     *     (TODO: Right now, bdrv_replace_child_noperm() never modifies that
 -     *     pointer anyway (though it will in the future), so at this point it
 -     *     absolutely does not matter whether we pass s->childp or &s->child.)
       * (2) If new_bs is not NULL, s->childp will be NULL.  We then cannot use
       *     it here.
       * (3) If new_bs is NULL, *s->childp will have been NULLed by
       *     bdrv_replace_child_tran()'s bdrv_replace_child_noperm() call, and we
       *     must not pass a NULL *s->childp here.
 -     *     (TODO: In its current state, bdrv_replace_child_noperm() will not
 -     *     have NULLed *s->childp, so this does not apply yet.  It will in the
 -     *     future.)
       *
       * So whether new_bs was NULL or not, we cannot pass s->childp here; and in
       * any case, there is no reason to pass it anyway.
       */
 -    bdrv_replace_child_noperm(&s->child, s->old_bs);
 +    bdrv_replace_child_noperm(&s->child, s->old_bs, true);
 +    /*
 +     * The child was pre-existing, so s->old_bs must be non-NULL, and
 +     * s->child thus must not have been freed
 +     */
 +    assert(s->child != NULL);
 +    if (!new_bs) {
 +        /* As described above, *s->childp was cleared, so restore it */
 +        assert(s->childp != NULL);
 +        *s->childp = s->child;
 +    }
      bdrv_unref(new_bs);
  }
@@ -XXX,XX +XXX,XX @@ static TransactionActionDrv bdrv_replace_child_drv = {
   *
   * The function doesn't update permissions, caller is responsible for this.
   *
 + * (*childp)->bs must not be NULL.
 + *
   * Note that if new_bs == NULL, @childp is stored in a state object attached
   * to @tran, so that the old child can be reinstated in the abort handler.
   * Therefore, if @new_bs can be NULL, @childp must stay valid until the
   * transaction is committed or aborted.
   *
 - * (TODO: The reinstating does not happen yet, but it will once
 - * bdrv_replace_child_noperm() NULLs *childp when new_bs is NULL.)
 + * If @free_empty_child is true and @new_bs is NULL, the BdrvChild is
 + * freed (on commit).  @free_empty_child should only be false if the
 + * caller will free the BDrvChild themselves (which may be important
 + * if this is in turn called in another transactional context).
   */
  static void bdrv_replace_child_tran(BdrvChild **childp,
                                      BlockDriverState *new_bs,
 -                                    Transaction *tran)
 +                                    Transaction *tran,
 +                                    bool free_empty_child)
  {
      BdrvReplaceChildState *s = g_new(BdrvReplaceChildState, 1);
      *s = (BdrvReplaceChildState) {
          .child = *childp,
          .childp = new_bs == NULL ? childp : NULL,
          .old_bs = (*childp)->bs,
 +        .free_empty_child = free_empty_child,
      };
      tran_add(tran, &bdrv_replace_child_drv, s);
 +    /* The abort handler relies on this */
 +    assert(s->old_bs != NULL);
 +
      if (new_bs) {
          bdrv_ref(new_bs);
      }
 -    bdrv_replace_child_noperm(childp, new_bs);
 +    /*
 +     * Pass free_empty_child=false, we will free the child (if
 +     * necessary) in bdrv_replace_child_commit() (if our
 +     * @free_empty_child parameter was true).
 +     */
 +    bdrv_replace_child_noperm(childp, new_bs, false);
      /* old_bs reference is transparently moved from *childp to @s */
  }
@@ -XXX,XX +XXX,XX @@ uint64_t bdrv_qapi_perm_to_blk_perm(BlockPermission qapi_perm)
      return permissions[qapi_perm];
  }
 +/**
 + * Replace (*childp)->bs by @new_bs.
 + *
 + * If @new_bs is NULL, *childp will be set to NULL, too: BDS parents
 + * generally cannot handle a BdrvChild with .bs == NULL, so clearing
 + * BdrvChild.bs should generally immediately be followed by the
 + * BdrvChild pointer being cleared as well.
 + *
 + * If @free_empty_child is true and @new_bs is NULL, the BdrvChild is
 + * freed.  @free_empty_child should only be false if the caller will
 + * free the BdrvChild themselves (this may be important in a
 + * transactional context, where it may only be freed on commit).
 + */
  static void bdrv_replace_child_noperm(BdrvChild **childp,
 -                                      BlockDriverState *new_bs)
 +                                      BlockDriverState *new_bs,
 +                                      bool free_empty_child)
  {
      BdrvChild *child = *childp;
      BlockDriverState *old_bs = child->bs;
@@ -XXX,XX +XXX,XX @@ static void bdrv_replace_child_noperm(BdrvChild **childp,
      }
      child->bs = new_bs;
 +    if (!new_bs) {
 +        *childp = NULL;
 +    }
      if (new_bs) {
          QLIST_INSERT_HEAD(&new_bs->parents, child, next_parent);
@@ -XXX,XX +XXX,XX @@ static void bdrv_replace_child_noperm(BdrvChild **childp,
          bdrv_parent_drained_end_single(child);
          drain_saldo++;
      }
 +
 +    if (free_empty_child && !child->bs) {
 +        bdrv_child_free(child);
 +    }
  }
  /**
@@ -XXX,XX +XXX,XX @@ static void bdrv_attach_child_common_abort(void *opaque)
      BdrvChild *child = *s->child;
      BlockDriverState *bs = child->bs;
 -    bdrv_replace_child_noperm(s->child, NULL);
 +    /*
 +     * Pass free_empty_child=false, because we still need the child
 +     * for the AioContext operations on the parent below; those
 +     * BdrvChildClass methods all work on a BdrvChild object, so we
 +     * need to keep it as an empty shell (after this function, it will
 +     * not be attached to any parent, and it will not have a .bs).
 +     */
 +    bdrv_replace_child_noperm(s->child, NULL, false);
      if (bdrv_get_aio_context(bs) != s->old_child_ctx) {
          bdrv_try_set_aio_context(bs, s->old_child_ctx, &error_abort);
@@ -XXX,XX +XXX,XX @@ static void bdrv_attach_child_common_abort(void *opaque)
      bdrv_unref(bs);
      bdrv_child_free(child);
 -    *s->child = NULL;
  }
  static TransactionActionDrv bdrv_attach_child_common_drv = {
@@ -XXX,XX +XXX,XX @@ static int bdrv_attach_child_common(BlockDriverState *child_bs,
      }
      bdrv_ref(child_bs);
 -    bdrv_replace_child_noperm(&new_child, child_bs);
 +    bdrv_replace_child_noperm(&new_child, child_bs, true);
 +    /* child_bs was non-NULL, so new_child must not have been freed */
 +    assert(new_child != NULL);
      *child = new_child;
@@ -XXX,XX +XXX,XX @@ static void bdrv_detach_child(BdrvChild **childp)
  {
      BlockDriverState *old_bs = (*childp)->bs;
 -    bdrv_replace_child_noperm(childp, NULL);
 -    bdrv_child_free(*childp);
 +    bdrv_replace_child_noperm(childp, NULL, true);
      if (old_bs) {
          /*
@@ -XXX,XX +XXX,XX @@ static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,
      }
      if (child->bs) {
 -        bdrv_replace_child_tran(childp, NULL, tran);
 +        /*
 +         * Pass free_empty_child=false, we will free the child in
 +         * bdrv_remove_filter_or_cow_child_commit()
 +         */
 +        bdrv_replace_child_tran(childp, NULL, tran, false);
      }
      s = g_new(BdrvRemoveFilterOrCowChild, 1);
@@ -XXX,XX +XXX,XX @@ static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,
          .is_backing = (childp == &bs->backing),
      };
      tran_add(tran, &bdrv_remove_filter_or_cow_child_drv, s);
 -
 -    *childp = NULL;
  }
  /*
@@ -XXX,XX +XXX,XX @@ static int bdrv_replace_node_noperm(BlockDriverState *from,
           * Passing a pointer to the local variable @c is fine here, because
           * @to is not NULL, and so &c will not be attached to the transaction.
           */
 -        bdrv_replace_child_tran(&c, to, tran);
 +        bdrv_replace_child_tran(&c, to, tran, true);
      }
      return 0;
@@ -XXX,XX +XXX,XX @@ int bdrv_replace_child_bs(BdrvChild *child, BlockDriverState *new_bs,
      bdrv_drained_begin(old_bs);
      bdrv_drained_begin(new_bs);
 -    bdrv_replace_child_tran(&child, new_bs, tran);
 +    bdrv_replace_child_tran(&child, new_bs, tran, true);
 +    /* @new_bs must have been non-NULL, so @child must not have been freed */
 +    assert(child != NULL);
      found = g_hash_table_new(NULL, NULL);
      refresh_list = bdrv_topological_dfs(refresh_list, found, old_bs);
 --
 .33.1

-[Qemu-devel] [PULL 10/12] block/rbd - update variable names to more apt names
+[PULL v2 10/13] iotests/030: Unthrottle parallel jobs in reverse
-Update 'clientname' to be 'user', which tracks better with both
+See the comment for why this is necessary.
 the QAPI and rados variable naming.
-Update 'name' to be 'image_name', as it indicates the rbd image.
+Signed-off-by: Hanna Reitz <hreitz@redhat.com>
-Naming it 'image' would have been ideal, but we are using that for
+Message-Id: <20211111120829.81329-11-hreitz@redhat.com>
-the rados_image_t value returned by rbd_open().
+Signed-off-by: Kevin Wolf <kwolf@redhat.com>
 Message-Id: <20211115145409.176785-11-kwolf@redhat.com>
 Signed-off-by: Hanna Reitz <hreitz@redhat.com>
 ---
  tests/qemu-iotests/030 | 11 ++++++++++-
 file changed, 10 insertions(+), 1 deletion(-)
-Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
+diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
-Signed-off-by: Jeff Cody <jcody@redhat.com>
+index XXXXXXX..XXXXXXX 100755
-Reviewed-by: John Snow <jsnow@redhat.com>
+--- a/tests/qemu-iotests/030
-Message-id: b7ec1fb2e1cf36f9b6911631447a5b0422590b7d.1491597120.git.jcody@redhat.com
++++ b/tests/qemu-iotests/030
----
+@@ -XXX,XX +XXX,XX @@ class TestParallelOps(iotests.QMPTestCase):
- block/rbd.c | 33 +++++++++++++++++----------------
+                                  speed=1024)
-file changed, 17 insertions(+), 16 deletions(-)
+             self.assert_qmp(result, 'return', {})
-diff --git a/block/rbd.c b/block/rbd.c
+-        for job in pending_jobs:
-index XXXXXXX..XXXXXXX 100644
++        # Do this in reverse: After unthrottling them, some jobs may finish
---- a/block/rbd.c
++        # before we have unthrottled all of them.  This will drain their
-+++ b/block/rbd.c
++        # subgraph, and this will make jobs above them advance (despite those
-@@ -XXX,XX +XXX,XX @@ typedef struct BDRVRBDState {
++        # jobs on top being throttled).  In the worst case, all jobs below the
-     rados_t cluster;
++        # top one are finished before we can unthrottle it, and this makes it
-     rados_ioctx_t io_ctx;
++        # advance so far that it completes before we can unthrottle it - which
-     rbd_image_t image;
++        # results in an error.
--    char *name;
++        # Starting from the top (i.e. in reverse) does not have this problem:
-+    char *image_name;
++        # When a job finishes, the ones below it are not advanced.
-     char *snap;
++        for job in reversed(pending_jobs):
- } BDRVRBDState;
+             result = self.vm.qmp('block-job-set-speed', device=job, speed=0)
+             self.assert_qmp(result, 'return', {})
@@ -XXX,XX +XXX,XX @@ static int qemu_rbd_create(const char *filename, QemuOpts *opts, Error **errp)
      int64_t bytes = 0;
      int64_t objsize;
      int obj_order = 0;
 -    const char *pool, *name, *conf, *clientname, *keypairs;
 +    const char *pool, *image_name, *conf, *user, *keypairs;
      const char *secretid;
      rados_t cluster;
      rados_ioctx_t io_ctx;
@@ -XXX,XX +XXX,XX @@ static int qemu_rbd_create(const char *filename, QemuOpts *opts, Error **errp)
       */
      pool       = qdict_get_try_str(options, "pool");
      conf       = qdict_get_try_str(options, "conf");
 -    clientname = qdict_get_try_str(options, "user");
 -    name       = qdict_get_try_str(options, "image");
 +    user       = qdict_get_try_str(options, "user");
 +    image_name = qdict_get_try_str(options, "image");
      keypairs   = qdict_get_try_str(options, "=keyvalue-pairs");
 -    ret = rados_create(&cluster, clientname);
 +    ret = rados_create(&cluster, user);
      if (ret < 0) {
          error_setg_errno(errp, -ret, "error initializing");
          goto exit;
@@ -XXX,XX +XXX,XX @@ static int qemu_rbd_create(const char *filename, QemuOpts *opts, Error **errp)
          goto shutdown;
      }
 -    ret = rbd_create(io_ctx, name, bytes, &obj_order);
 +    ret = rbd_create(io_ctx, image_name, bytes, &obj_order);
      if (ret < 0) {
          error_setg_errno(errp, -ret, "error rbd create");
      }
@@ -XXX,XX +XXX,XX @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
                           Error **errp)
  {
      BDRVRBDState *s = bs->opaque;
 -    const char *pool, *snap, *conf, *clientname, *name, *keypairs;
 +    const char *pool, *snap, *conf, *user, *image_name, *keypairs;
      const char *secretid;
      QemuOpts *opts;
      Error *local_err = NULL;
@@ -XXX,XX +XXX,XX @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
      pool           = qemu_opt_get(opts, "pool");
      conf           = qemu_opt_get(opts, "conf");
      snap           = qemu_opt_get(opts, "snapshot");
 -    clientname     = qemu_opt_get(opts, "user");
 -    name           = qemu_opt_get(opts, "image");
 +    user           = qemu_opt_get(opts, "user");
 +    image_name     = qemu_opt_get(opts, "image");
      keypairs       = qemu_opt_get(opts, "=keyvalue-pairs");
 -    if (!pool || !name) {
 +    if (!pool || !image_name) {
          error_setg(errp, "Parameters 'pool' and 'image' are required");
          r = -EINVAL;
          goto failed_opts;
      }
 -    r = rados_create(&s->cluster, clientname);
 +    r = rados_create(&s->cluster, user);
      if (r < 0) {
          error_setg_errno(errp, -r, "error initializing");
          goto failed_opts;
      }
      s->snap = g_strdup(snap);
 -    s->name = g_strdup(name);
 +    s->image_name = g_strdup(image_name);
      /* try default location when conf=NULL, but ignore failure */
      r = rados_conf_read_file(s->cluster, conf);
@@ -XXX,XX +XXX,XX @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
      }
      /* rbd_open is always r/w */
 -    r = rbd_open(s->io_ctx, s->name, &s->image, s->snap);
 +    r = rbd_open(s->io_ctx, s->image_name, &s->image, s->snap);
      if (r < 0) {
 -        error_setg_errno(errp, -r, "error reading header from %s", s->name);
 +        error_setg_errno(errp, -r, "error reading header from %s",
 +                         s->image_name);
          goto failed_open;
      }
@@ -XXX,XX +XXX,XX @@ failed_open:
  failed_shutdown:
      rados_shutdown(s->cluster);
      g_free(s->snap);
 -    g_free(s->name);
 +    g_free(s->image_name);
  failed_opts:
      qemu_opts_del(opts);
      g_free(mon_host);
@@ -XXX,XX +XXX,XX @@ static void qemu_rbd_close(BlockDriverState *bs)
      rbd_close(s->image);
      rados_ioctx_destroy(s->io_ctx);
      g_free(s->snap);
 -    g_free(s->name);
 +    g_free(s->image_name);
      rados_shutdown(s->cluster);
  }
 --
-.9.3
+.33.1

-[Qemu-devel] [PULL 03/12] qemu-iotests: exclude vxhs from image creation via protocol
+[PULL v2 11/13] docs: Deprecate incorrectly typed device_add arguments
-The protocol VXHS does not support image creation.  Some tests expect
+From: Kevin Wolf <kwolf@redhat.com>
 to be able to create images through the protocol.  Exclude VXHS from
 these tests.
-Signed-off-by: Jeff Cody <jcody@redhat.com>
+While introducing a non-QemuOpts code path for device creation for JSON
 -device, we noticed that QMP device_add doesn't check its input
 correctly (accepting arguments that should have been rejected), and that
 users may be relying on this behaviour (libvirt did until it was fixed
 recently).
 Let's use a deprecation period before we fix this bug in QEMU to avoid
 nasty surprises for users.
 Signed-off-by: Kevin Wolf <kwolf@redhat.com>
 Message-Id: <20211111143530.18985-1-kwolf@redhat.com>
 Reviewed-by: Markus Armbruster <armbru@redhat.com>
 Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
 Signed-off-by: Kevin Wolf <kwolf@redhat.com>
 Message-Id: <20211115145409.176785-12-kwolf@redhat.com>
 Signed-off-by: Hanna Reitz <hreitz@redhat.com>
 ---
- tests/qemu-iotests/017 | 1 +
+ docs/about/deprecated.rst | 14 ++++++++++++++
- tests/qemu-iotests/020 | 1 +
+file changed, 14 insertions(+)
  tests/qemu-iotests/029 | 1 +
  tests/qemu-iotests/073 | 1 +
  tests/qemu-iotests/114 | 1 +
  tests/qemu-iotests/130 | 1 +
  tests/qemu-iotests/134 | 1 +
  tests/qemu-iotests/156 | 1 +
  tests/qemu-iotests/158 | 1 +
 files changed, 9 insertions(+)
-diff --git a/tests/qemu-iotests/017 b/tests/qemu-iotests/017
+diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
-index XXXXXXX..XXXXXXX 100755
+index XXXXXXX..XXXXXXX 100644
---- a/tests/qemu-iotests/017
+--- a/docs/about/deprecated.rst
-+++ b/tests/qemu-iotests/017
++++ b/docs/about/deprecated.rst
-@@ -XXX,XX +XXX,XX @@ trap "_cleanup; exit \$status" 0 1 2 3 15
+@@ -XXX,XX +XXX,XX @@ options are removed in favor of using explicit ``blockdev-create`` and
- # Any format supporting backing files
+ ``blockdev-add`` calls. See :doc:`/interop/live-block-operations` for
- _supported_fmt qcow qcow2 vmdk qed
+ details.
- _supported_proto generic
-+_unsupported_proto vxhs
++Incorrectly typed ``device_add`` arguments (since 6.2)
- _supported_os Linux
++''''''''''''''''''''''''''''''''''''''''''''''''''''''
- _unsupported_imgopts "subformat=monolithicFlat" "subformat=twoGbMaxExtentFlat"
++
++Due to shortcomings in the internal implementation of ``device_add``, QEMU
-diff --git a/tests/qemu-iotests/020 b/tests/qemu-iotests/020
++incorrectly accepts certain invalid arguments: Any object or list arguments are
-index XXXXXXX..XXXXXXX 100755
++silently ignored. Other argument types are not checked, but an implicit
---- a/tests/qemu-iotests/020
++conversion happens, so that e.g. string values can be assigned to integer
-+++ b/tests/qemu-iotests/020
++device properties or vice versa.
-@@ -XXX,XX +XXX,XX @@ trap "_cleanup; exit \$status" 0 1 2 3 15
++
- # Any format supporting backing files
++This is a bug in QEMU that will be fixed in the future so that previously
- _supported_fmt qcow qcow2 vmdk qed
++accepted incorrect commands will return an error. Users should make sure that
- _supported_proto generic
++all arguments passed to ``device_add`` are consistent with the documented
-+_unsupported_proto vxhs
++property types.
- _supported_os Linux
++
- _unsupported_imgopts "subformat=monolithicFlat" \
+ System accelerators
-                      "subformat=twoGbMaxExtentFlat" \
+ -------------------
 diff --git a/tests/qemu-iotests/029 b/tests/qemu-iotests/029
 index XXXXXXX..XXXXXXX 100755
 --- a/tests/qemu-iotests/029
 +++ b/tests/qemu-iotests/029
@@ -XXX,XX +XXX,XX @@ trap "_cleanup; exit \$status" 0 1 2 3 15
  # Any format supporting intenal snapshots
  _supported_fmt qcow2
  _supported_proto generic
 +_unsupported_proto vxhs
  _supported_os Linux
  # Internal snapshots are (currently) impossible with refcount_bits=1
  _unsupported_imgopts 'refcount_bits=1[^0-9]'
 diff --git a/tests/qemu-iotests/073 b/tests/qemu-iotests/073
 index XXXXXXX..XXXXXXX 100755
 --- a/tests/qemu-iotests/073
 +++ b/tests/qemu-iotests/073
@@ -XXX,XX +XXX,XX @@ trap "_cleanup; exit \$status" 0 1 2 3 15
  _supported_fmt qcow2
  _supported_proto generic
 +_unsupported_proto vxhs
  _supported_os Linux
  CLUSTER_SIZE=64k
 diff --git a/tests/qemu-iotests/114 b/tests/qemu-iotests/114
 index XXXXXXX..XXXXXXX 100755
 --- a/tests/qemu-iotests/114
 +++ b/tests/qemu-iotests/114
@@ -XXX,XX +XXX,XX @@ trap "_cleanup; exit \$status" 0 1 2 3 15
  _supported_fmt qcow2
  _supported_proto generic
 +_unsupported_proto vxhs
  _supported_os Linux
 diff --git a/tests/qemu-iotests/130 b/tests/qemu-iotests/130
 index XXXXXXX..XXXXXXX 100755
 --- a/tests/qemu-iotests/130
 +++ b/tests/qemu-iotests/130
@@ -XXX,XX +XXX,XX @@ trap "_cleanup; exit \$status" 0 1 2 3 15
  _supported_fmt qcow2
  _supported_proto generic
 +_unsupported_proto vxhs
  _supported_os Linux
  qemu_comm_method="monitor"
 diff --git a/tests/qemu-iotests/134 b/tests/qemu-iotests/134
 index XXXXXXX..XXXXXXX 100755
 --- a/tests/qemu-iotests/134
 +++ b/tests/qemu-iotests/134
@@ -XXX,XX +XXX,XX @@ trap "_cleanup; exit \$status" 0 1 2 3 15
  _supported_fmt qcow2
  _supported_proto generic
 +_unsupported_proto vxhs
  _supported_os Linux
 diff --git a/tests/qemu-iotests/156 b/tests/qemu-iotests/156
 index XXXXXXX..XXXXXXX 100755
 --- a/tests/qemu-iotests/156
 +++ b/tests/qemu-iotests/156
@@ -XXX,XX +XXX,XX @@ trap "_cleanup; exit \$status" 0 1 2 3 15
  _supported_fmt qcow2 qed
  _supported_proto generic
 +_unsupported_proto vxhs
  _supported_os Linux
  # Create source disk
 diff --git a/tests/qemu-iotests/158 b/tests/qemu-iotests/158
 index XXXXXXX..XXXXXXX 100755
 --- a/tests/qemu-iotests/158
 +++ b/tests/qemu-iotests/158
@@ -XXX,XX +XXX,XX @@ trap "_cleanup; exit \$status" 0 1 2 3 15
  _supported_fmt qcow2
  _supported_proto generic
 +_unsupported_proto vxhs
  _supported_os Linux
 --
-.9.3
+.33.1

-[Qemu-devel] [PULL 01/12] block/vxhs.c: Add support for a new block device type called "vxhs"
+[PULL v2 12/13] softmmu/qdev-monitor: fix use-after-free in qdev_set_id()
-From: Ashish Mittal <ashmit602@gmail.com>
+From: Stefan Hajnoczi <stefanha@redhat.com>
-Source code for the qnio library that this code loads can be downloaded from:
+Reported by Coverity (CID 1465222).
 https://github.com/VeritasHyperScale/libqnio.git
-Sample command line using JSON syntax:
+Fixes: 4a1d937796de0fecd8b22d7dbebf87f38e8282fd ("softmmu/qdev-monitor: add error handling in qdev_set_id")
-./x86_64-softmmu/qemu-system-x86_64 -name instance-00000008 -S -vnc 0.0.0.0:0
+Cc: Damien Hedde <damien.hedde@greensocs.com>
--k en-us -vga cirrus -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x5
+Cc: Kevin Wolf <kwolf@redhat.com>
--msg timestamp=on
+Cc: Michael S. Tsirkin <mst@redhat.com>
-'json:{"driver":"vxhs","vdisk-id":"c3e9095a-a5ee-4dce-afeb-2a59fb387410",
+Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
-"server":{"host":"172.172.17.4","port":"9999"}}'
+Message-Id: <20211102163342.31162-1-stefanha@redhat.com>
 Reviewed-by: Kevin Wolf <kwolf@redhat.com>
 Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
 Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
 Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
 Reviewed-by: Damien Hedde <damien.hedde@greensocs.com>
 Reviewed-by: Markus Armbruster <armbru@redhat.com>
 Signed-off-by: Kevin Wolf <kwolf@redhat.com>
 Message-Id: <20211115145409.176785-14-kwolf@redhat.com>
 Signed-off-by: Hanna Reitz <hreitz@redhat.com>
 ---
  softmmu/qdev-monitor.c | 2 +-
 file changed, 1 insertion(+), 1 deletion(-)
-Sample command line using URI syntax:
+diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
 qemu-img convert -f raw -O raw -n
 /var/lib/nova/instances/_base/0c5eacd5ebea5ed914b6a3e7b18f1ce734c386ad
 vxhs://192.168.0.1:9999/c6718f6b-0401-441d-a8c3-1f0064d75ee0
 Sample command line using TLS credentials (run in secure mode):
 ./qemu-io --object
 tls-creds-x509,id=tls0,dir=/etc/pki/qemu/vxhs,endpoint=client -c 'read
 -v 66000 2.5k' 'json:{"server.host": "127.0.0.1", "server.port": "9999",
 "vdisk-id": "/test.raw", "driver": "vxhs", "tls-creds":"tls0"}'
 Signed-off-by: Ashish Mittal <Ashish.Mittal@veritas.com>
 Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
 Reviewed-by: Jeff Cody <jcody@redhat.com>
 Signed-off-by: Jeff Cody <jcody@redhat.com>
 Message-id: 1491277689-24949-2-git-send-email-Ashish.Mittal@veritas.com
 ---
  block/Makefile.objs  |   2 +
  block/trace-events   |  17 ++
  block/vxhs.c         | 575 +++++++++++++++++++++++++++++++++++++++++++++++++++
  configure            |  39 ++++
  qapi/block-core.json |  23 ++-
 files changed, 654 insertions(+), 2 deletions(-)
  create mode 100644 block/vxhs.c
 diff --git a/block/Makefile.objs b/block/Makefile.objs
 index XXXXXXX..XXXXXXX 100644
---- a/block/Makefile.objs
+--- a/softmmu/qdev-monitor.c
-+++ b/block/Makefile.objs
++++ b/softmmu/qdev-monitor.c
-@@ -XXX,XX +XXX,XX @@ block-obj-$(CONFIG_LIBNFS) += nfs.o
+@@ -XXX,XX +XXX,XX @@ const char *qdev_set_id(DeviceState *dev, char *id, Error **errp)
- block-obj-$(CONFIG_CURL) += curl.o
+         if (prop) {
- block-obj-$(CONFIG_RBD) += rbd.o
+             dev->id = id;
- block-obj-$(CONFIG_GLUSTERFS) += gluster.o
+         } else {
-+block-obj-$(CONFIG_VXHS) += vxhs.o
+-            g_free(id);
- block-obj-$(CONFIG_LIBSSH2) += ssh.o
+             error_setg(errp, "Duplicate device ID '%s'", id);
- block-obj-y += accounting.o dirty-bitmap.o
++            g_free(id);
- block-obj-y += write-threshold.o
+             return NULL;
-@@ -XXX,XX +XXX,XX @@ rbd.o-cflags       := $(RBD_CFLAGS)
+         }
- rbd.o-libs         := $(RBD_LIBS)
+     } else {
  gluster.o-cflags   := $(GLUSTERFS_CFLAGS)
  gluster.o-libs     := $(GLUSTERFS_LIBS)
 +vxhs.o-libs        := $(VXHS_LIBS)
  ssh.o-cflags       := $(LIBSSH2_CFLAGS)
  ssh.o-libs         := $(LIBSSH2_LIBS)
  block-obj-$(if $(CONFIG_BZIP2),m,n) += dmg-bz2.o
 diff --git a/block/trace-events b/block/trace-events
 index XXXXXXX..XXXXXXX 100644
 --- a/block/trace-events
 +++ b/block/trace-events
@@ -XXX,XX +XXX,XX @@ qed_aio_write_data(void *s, void *acb, int ret, uint64_t offset, size_t len) "s
  qed_aio_write_prefill(void *s, void *acb, uint64_t start, size_t len, uint64_t offset) "s %p acb %p start %"PRIu64" len %zu offset %"PRIu64
  qed_aio_write_postfill(void *s, void *acb, uint64_t start, size_t len, uint64_t offset) "s %p acb %p start %"PRIu64" len %zu offset %"PRIu64
  qed_aio_write_main(void *s, void *acb, int ret, uint64_t offset, size_t len) "s %p acb %p ret %d offset %"PRIu64" len %zu"
 +
 +# block/vxhs.c
 +vxhs_iio_callback(int error) "ctx is NULL: error %d"
 +vxhs_iio_callback_chnfail(int err, int error) "QNIO channel failed, no i/o %d, %d"
 +vxhs_iio_callback_unknwn(int opcode, int err) "unexpected opcode %d, errno %d"
 +vxhs_aio_rw_invalid(int req) "Invalid I/O request iodir %d"
 +vxhs_aio_rw_ioerr(char *guid, int iodir, uint64_t size, uint64_t off, void *acb, int ret, int err) "IO ERROR (vDisk %s) FOR : Read/Write = %d size = %lu offset = %lu ACB = %p. Error = %d, errno = %d"
 +vxhs_get_vdisk_stat_err(char *guid, int ret, int err) "vDisk (%s) stat ioctl failed, ret = %d, errno = %d"
 +vxhs_get_vdisk_stat(char *vdisk_guid, uint64_t vdisk_size) "vDisk %s stat ioctl returned size %lu"
 +vxhs_complete_aio(void *acb, uint64_t ret) "aio failed acb %p ret %ld"
 +vxhs_parse_uri_filename(const char *filename) "URI passed via bdrv_parse_filename %s"
 +vxhs_open_vdiskid(const char *vdisk_id) "Opening vdisk-id %s"
 +vxhs_open_hostinfo(char *of_vsa_addr, int port) "Adding host %s:%d to BDRVVXHSState"
 +vxhs_open_iio_open(const char *host) "Failed to connect to storage agent on host %s"
 +vxhs_parse_uri_hostinfo(char *host, int port) "Host: IP %s, Port %d"
 +vxhs_close(char *vdisk_guid) "Closing vdisk %s"
 +vxhs_get_creds(const char *cacert, const char *client_key, const char *client_cert) "cacert %s, client_key %s, client_cert %s"
 diff --git a/block/vxhs.c b/block/vxhs.c
 new file mode 100644
 index XXXXXXX..XXXXXXX
 --- /dev/null
 +++ b/block/vxhs.c
@@ -XXX,XX +XXX,XX @@
 +/*
 + * QEMU Block driver for Veritas HyperScale (VxHS)
 + *
 + * Copyright (c) 2017 Veritas Technologies LLC.
 + *
 + * This work is licensed under the terms of the GNU GPL, version 2 or later.
 + * See the COPYING file in the top-level directory.
 + *
 + */
 +
 +#include "qemu/osdep.h"
 +#include <qnio/qnio_api.h>
 +#include <sys/param.h>
 +#include "block/block_int.h"
 +#include "qapi/qmp/qerror.h"
 +#include "qapi/qmp/qdict.h"
 +#include "qapi/qmp/qstring.h"
 +#include "trace.h"
 +#include "qemu/uri.h"
 +#include "qapi/error.h"
 +#include "qemu/uuid.h"
 +#include "crypto/tlscredsx509.h"
 +
 +#define VXHS_OPT_FILENAME           "filename"
 +#define VXHS_OPT_VDISK_ID           "vdisk-id"
 +#define VXHS_OPT_SERVER             "server"
 +#define VXHS_OPT_HOST               "host"
 +#define VXHS_OPT_PORT               "port"
 +
 +/* Only accessed under QEMU global mutex */
 +static uint32_t vxhs_ref;
 +
 +typedef enum {
 +    VDISK_AIO_READ,
 +    VDISK_AIO_WRITE,
 +} VDISKAIOCmd;
 +
 +/*
 + * HyperScale AIO callbacks structure
 + */
 +typedef struct VXHSAIOCB {
 +    BlockAIOCB common;
 +    int err;
 +} VXHSAIOCB;
 +
 +typedef struct VXHSvDiskHostsInfo {
 +    void *dev_handle; /* Device handle */
 +    char *host; /* Host name or IP */
 +    int port; /* Host's port number */
 +} VXHSvDiskHostsInfo;
 +
 +/*
 + * Structure per vDisk maintained for state
 + */
 +typedef struct BDRVVXHSState {
 +    VXHSvDiskHostsInfo vdisk_hostinfo; /* Per host info */
 +    char *vdisk_guid;
 +    char *tlscredsid; /* tlscredsid */
 +} BDRVVXHSState;
 +
 +static void vxhs_complete_aio_bh(void *opaque)
 +{
 +    VXHSAIOCB *acb = opaque;
 +    BlockCompletionFunc *cb = acb->common.cb;
 +    void *cb_opaque = acb->common.opaque;
 +    int ret = 0;
 +
 +    if (acb->err != 0) {
 +        trace_vxhs_complete_aio(acb, acb->err);
 +        ret = (-EIO);
 +    }
 +
 +    qemu_aio_unref(acb);
 +    cb(cb_opaque, ret);
 +}
 +
 +/*
 + * Called from a libqnio thread
 + */
 +static void vxhs_iio_callback(void *ctx, uint32_t opcode, uint32_t error)
 +{
 +    VXHSAIOCB *acb = NULL;
 +
 +    switch (opcode) {
 +    case IRP_READ_REQUEST:
 +    case IRP_WRITE_REQUEST:
 +
 +        /*
 +         * ctx is VXHSAIOCB*
 +         * ctx is NULL if error is QNIOERROR_CHANNEL_HUP
 +         */
 +        if (ctx) {
 +            acb = ctx;
 +        } else {
 +            trace_vxhs_iio_callback(error);
 +            goto out;
 +        }
 +
 +        if (error) {
 +            if (!acb->err) {
 +                acb->err = error;
 +            }
 +            trace_vxhs_iio_callback(error);
 +        }
 +
 +        aio_bh_schedule_oneshot(bdrv_get_aio_context(acb->common.bs),
 +                                vxhs_complete_aio_bh, acb);
 +        break;
 +
 +    default:
 +        if (error == QNIOERROR_HUP) {
 +            /*
 +             * Channel failed, spontaneous notification,
 +             * not in response to I/O
 +             */
 +            trace_vxhs_iio_callback_chnfail(error, errno);
 +        } else {
 +            trace_vxhs_iio_callback_unknwn(opcode, error);
 +        }
 +        break;
 +    }
 +out:
 +    return;
 +}
 +
 +static QemuOptsList runtime_opts = {
 +    .name = "vxhs",
 +    .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
 +    .desc = {
 +        {
 +            .name = VXHS_OPT_FILENAME,
 +            .type = QEMU_OPT_STRING,
 +            .help = "URI to the Veritas HyperScale image",
 +        },
 +        {
 +            .name = VXHS_OPT_VDISK_ID,
 +            .type = QEMU_OPT_STRING,
 +            .help = "UUID of the VxHS vdisk",
 +        },
 +        {
 +            .name = "tls-creds",
 +            .type = QEMU_OPT_STRING,
 +            .help = "ID of the TLS/SSL credentials to use",
 +        },
 +        { /* end of list */ }
 +    },
 +};
 +
 +static QemuOptsList runtime_tcp_opts = {
 +    .name = "vxhs_tcp",
 +    .head = QTAILQ_HEAD_INITIALIZER(runtime_tcp_opts.head),
 +    .desc = {
 +        {
 +            .name = VXHS_OPT_HOST,
 +            .type = QEMU_OPT_STRING,
 +            .help = "host address (ipv4 addresses)",
 +        },
 +        {
 +            .name = VXHS_OPT_PORT,
 +            .type = QEMU_OPT_NUMBER,
 +            .help = "port number on which VxHSD is listening (default 9999)",
 +            .def_value_str = "9999"
 +        },
 +        { /* end of list */ }
 +    },
 +};
 +
 +/*
 + * Parse incoming URI and populate *options with the host
 + * and device information
 + */
 +static int vxhs_parse_uri(const char *filename, QDict *options)
 +{
 +    URI *uri = NULL;
 +    char *port;
 +    int ret = 0;
 +
 +    trace_vxhs_parse_uri_filename(filename);
 +    uri = uri_parse(filename);
 +    if (!uri || !uri->server || !uri->path) {
 +        uri_free(uri);
 +        return -EINVAL;
 +    }
 +
 +    qdict_put(options, VXHS_OPT_SERVER".host", qstring_from_str(uri->server));
 +
 +    if (uri->port) {
 +        port = g_strdup_printf("%d", uri->port);
 +        qdict_put(options, VXHS_OPT_SERVER".port", qstring_from_str(port));
 +        g_free(port);
 +    }
 +
 +    qdict_put(options, "vdisk-id", qstring_from_str(uri->path));
 +
 +    trace_vxhs_parse_uri_hostinfo(uri->server, uri->port);
 +    uri_free(uri);
 +
 +    return ret;
 +}
 +
 +static void vxhs_parse_filename(const char *filename, QDict *options,
 +                                Error **errp)
 +{
 +    if (qdict_haskey(options, "vdisk-id") || qdict_haskey(options, "server")) {
 +        error_setg(errp, "vdisk-id/server and a file name may not be specified "
 +                         "at the same time");
 +        return;
 +    }
 +
 +    if (strstr(filename, "://")) {
 +        int ret = vxhs_parse_uri(filename, options);
 +        if (ret < 0) {
 +            error_setg(errp, "Invalid URI. URI should be of the form "
 +                       "  vxhs://<host_ip>:<port>/<vdisk-id>");
 +        }
 +    }
 +}
 +
 +static int vxhs_init_and_ref(void)
 +{
 +    if (vxhs_ref++ == 0) {
 +        if (iio_init(QNIO_VERSION, vxhs_iio_callback)) {
 +            return -ENODEV;
 +        }
 +    }
 +    return 0;
 +}
 +
 +static void vxhs_unref(void)
 +{
 +    if (--vxhs_ref == 0) {
 +        iio_fini();
 +    }
 +}
 +
 +static void vxhs_get_tls_creds(const char *id, char **cacert,
 +                               char **key, char **cert, Error **errp)
 +{
 +    Object *obj;
 +    QCryptoTLSCreds *creds;
 +    QCryptoTLSCredsX509 *creds_x509;
 +
 +    obj = object_resolve_path_component(
 +        object_get_objects_root(), id);
 +
 +    if (!obj) {
 +        error_setg(errp, "No TLS credentials with id '%s'",
 +                   id);
 +        return;
 +    }
 +
 +    creds_x509 = (QCryptoTLSCredsX509 *)
 +        object_dynamic_cast(obj, TYPE_QCRYPTO_TLS_CREDS_X509);
 +
 +    if (!creds_x509) {
 +        error_setg(errp, "Object with id '%s' is not TLS credentials",
 +                   id);
 +        return;
 +    }
 +
 +    creds = &creds_x509->parent_obj;
 +
 +    if (creds->endpoint != QCRYPTO_TLS_CREDS_ENDPOINT_CLIENT) {
 +        error_setg(errp,
 +                   "Expecting TLS credentials with a client endpoint");
 +        return;
 +    }
 +
 +    /*
 +     * Get the cacert, client_cert and client_key file names.
 +     */
 +    if (!creds->dir) {
 +        error_setg(errp, "TLS object missing 'dir' property value");
 +        return;
 +    }
 +
 +    *cacert = g_strdup_printf("%s/%s", creds->dir,
 +                              QCRYPTO_TLS_CREDS_X509_CA_CERT);
 +    *cert = g_strdup_printf("%s/%s", creds->dir,
 +                            QCRYPTO_TLS_CREDS_X509_CLIENT_CERT);
 +    *key = g_strdup_printf("%s/%s", creds->dir,
 +                           QCRYPTO_TLS_CREDS_X509_CLIENT_KEY);
 +}
 +
 +static int vxhs_open(BlockDriverState *bs, QDict *options,
 +                     int bdrv_flags, Error **errp)
 +{
 +    BDRVVXHSState *s = bs->opaque;
 +    void *dev_handlep;
 +    QDict *backing_options = NULL;
 +    QemuOpts *opts = NULL;
 +    QemuOpts *tcp_opts = NULL;
 +    char *of_vsa_addr = NULL;
 +    Error *local_err = NULL;
 +    const char *vdisk_id_opt;
 +    const char *server_host_opt;
 +    int ret = 0;
 +    char *cacert = NULL;
 +    char *client_key = NULL;
 +    char *client_cert = NULL;
 +
 +    ret = vxhs_init_and_ref();
 +    if (ret < 0) {
 +        ret = -EINVAL;
 +        goto out;
 +    }
 +
 +    /* Create opts info from runtime_opts and runtime_tcp_opts list */
 +    opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
 +    tcp_opts = qemu_opts_create(&runtime_tcp_opts, NULL, 0, &error_abort);
 +
 +    qemu_opts_absorb_qdict(opts, options, &local_err);
 +    if (local_err) {
 +        ret = -EINVAL;
 +        goto out;
 +    }
 +
 +    /* vdisk-id is the disk UUID */
 +    vdisk_id_opt = qemu_opt_get(opts, VXHS_OPT_VDISK_ID);
 +    if (!vdisk_id_opt) {
 +        error_setg(&local_err, QERR_MISSING_PARAMETER, VXHS_OPT_VDISK_ID);
 +        ret = -EINVAL;
 +        goto out;
 +    }
 +
 +    /* vdisk-id may contain a leading '/' */
 +    if (strlen(vdisk_id_opt) > UUID_FMT_LEN + 1) {
 +        error_setg(&local_err, "vdisk-id cannot be more than %d characters",
 +                   UUID_FMT_LEN);
 +        ret = -EINVAL;
 +        goto out;
 +    }
 +
 +    s->vdisk_guid = g_strdup(vdisk_id_opt);
 +    trace_vxhs_open_vdiskid(vdisk_id_opt);
 +
 +    /* get the 'server.' arguments */
 +    qdict_extract_subqdict(options, &backing_options, VXHS_OPT_SERVER".");
 +
 +    qemu_opts_absorb_qdict(tcp_opts, backing_options, &local_err);
 +    if (local_err != NULL) {
 +        ret = -EINVAL;
 +        goto out;
 +    }
 +
 +    server_host_opt = qemu_opt_get(tcp_opts, VXHS_OPT_HOST);
 +    if (!server_host_opt) {
 +        error_setg(&local_err, QERR_MISSING_PARAMETER,
 +                   VXHS_OPT_SERVER"."VXHS_OPT_HOST);
 +        ret = -EINVAL;
 +        goto out;
 +    }
 +
 +    if (strlen(server_host_opt) > MAXHOSTNAMELEN) {
 +        error_setg(&local_err, "server.host cannot be more than %d characters",
 +                   MAXHOSTNAMELEN);
 +        ret = -EINVAL;
 +        goto out;
 +    }
 +
 +    /* check if we got tls-creds via the --object argument */
 +    s->tlscredsid = g_strdup(qemu_opt_get(opts, "tls-creds"));
 +    if (s->tlscredsid) {
 +        vxhs_get_tls_creds(s->tlscredsid, &cacert, &client_key,
 +                           &client_cert, &local_err);
 +        if (local_err != NULL) {
 +            ret = -EINVAL;
 +            goto out;
 +        }
 +        trace_vxhs_get_creds(cacert, client_key, client_cert);
 +    }
 +
 +    s->vdisk_hostinfo.host = g_strdup(server_host_opt);
 +    s->vdisk_hostinfo.port = g_ascii_strtoll(qemu_opt_get(tcp_opts,
 +                                                          VXHS_OPT_PORT),
 +                                                          NULL, 0);
 +
 +    trace_vxhs_open_hostinfo(s->vdisk_hostinfo.host,
 +                             s->vdisk_hostinfo.port);
 +
 +    of_vsa_addr = g_strdup_printf("of://%s:%d",
 +                                  s->vdisk_hostinfo.host,
 +                                  s->vdisk_hostinfo.port);
 +
 +    /*
 +     * Open qnio channel to storage agent if not opened before
 +     */
 +    dev_handlep = iio_open(of_vsa_addr, s->vdisk_guid, 0,
 +                           cacert, client_key, client_cert);
 +    if (dev_handlep == NULL) {
 +        trace_vxhs_open_iio_open(of_vsa_addr);
 +        ret = -ENODEV;
 +        goto out;
 +    }
 +    s->vdisk_hostinfo.dev_handle = dev_handlep;
 +
 +out:
 +    g_free(of_vsa_addr);
 +    QDECREF(backing_options);
 +    qemu_opts_del(tcp_opts);
 +    qemu_opts_del(opts);
 +    g_free(cacert);
 +    g_free(client_key);
 +    g_free(client_cert);
 +
 +    if (ret < 0) {
 +        vxhs_unref();
 +        error_propagate(errp, local_err);
 +        g_free(s->vdisk_hostinfo.host);
 +        g_free(s->vdisk_guid);
 +        g_free(s->tlscredsid);
 +        s->vdisk_guid = NULL;
 +    }
 +
 +    return ret;
 +}
 +
 +static const AIOCBInfo vxhs_aiocb_info = {
 +    .aiocb_size = sizeof(VXHSAIOCB)
 +};
 +
 +/*
 + * This allocates QEMU-VXHS callback for each IO
 + * and is passed to QNIO. When QNIO completes the work,
 + * it will be passed back through the callback.
 + */
 +static BlockAIOCB *vxhs_aio_rw(BlockDriverState *bs, int64_t sector_num,
 +                               QEMUIOVector *qiov, int nb_sectors,
 +                               BlockCompletionFunc *cb, void *opaque,
 +                               VDISKAIOCmd iodir)
 +{
 +    VXHSAIOCB *acb = NULL;
 +    BDRVVXHSState *s = bs->opaque;
 +    size_t size;
 +    uint64_t offset;
 +    int iio_flags = 0;
 +    int ret = 0;
 +    void *dev_handle = s->vdisk_hostinfo.dev_handle;
 +
 +    offset = sector_num * BDRV_SECTOR_SIZE;
 +    size = nb_sectors * BDRV_SECTOR_SIZE;
 +    acb = qemu_aio_get(&vxhs_aiocb_info, bs, cb, opaque);
 +
 +    /*
 +     * Initialize VXHSAIOCB.
 +     */
 +    acb->err = 0;
 +
 +    iio_flags = IIO_FLAG_ASYNC;
 +
 +    switch (iodir) {
 +    case VDISK_AIO_WRITE:
 +            ret = iio_writev(dev_handle, acb, qiov->iov, qiov->niov,
 +                             offset, (uint64_t)size, iio_flags);
 +            break;
 +    case VDISK_AIO_READ:
 +            ret = iio_readv(dev_handle, acb, qiov->iov, qiov->niov,
 +                            offset, (uint64_t)size, iio_flags);
 +            break;
 +    default:
 +            trace_vxhs_aio_rw_invalid(iodir);
 +            goto errout;
 +    }
 +
 +    if (ret != 0) {
 +        trace_vxhs_aio_rw_ioerr(s->vdisk_guid, iodir, size, offset,
 +                                acb, ret, errno);
 +        goto errout;
 +    }
 +    return &acb->common;
 +
 +errout:
 +    qemu_aio_unref(acb);
 +    return NULL;
 +}
 +
 +static BlockAIOCB *vxhs_aio_readv(BlockDriverState *bs,
 +                                   int64_t sector_num, QEMUIOVector *qiov,
 +                                   int nb_sectors,
 +                                   BlockCompletionFunc *cb, void *opaque)
 +{
 +    return vxhs_aio_rw(bs, sector_num, qiov, nb_sectors, cb,
 +                       opaque, VDISK_AIO_READ);
 +}
 +
 +static BlockAIOCB *vxhs_aio_writev(BlockDriverState *bs,
 +                                   int64_t sector_num, QEMUIOVector *qiov,
 +                                   int nb_sectors,
 +                                   BlockCompletionFunc *cb, void *opaque)
 +{
 +    return vxhs_aio_rw(bs, sector_num, qiov, nb_sectors,
 +                       cb, opaque, VDISK_AIO_WRITE);
 +}
 +
 +static void vxhs_close(BlockDriverState *bs)
 +{
 +    BDRVVXHSState *s = bs->opaque;
 +
 +    trace_vxhs_close(s->vdisk_guid);
 +
 +    g_free(s->vdisk_guid);
 +    s->vdisk_guid = NULL;
 +
 +    /*
 +     * Close vDisk device
 +     */
 +    if (s->vdisk_hostinfo.dev_handle) {
 +        iio_close(s->vdisk_hostinfo.dev_handle);
 +        s->vdisk_hostinfo.dev_handle = NULL;
 +    }
 +
 +    vxhs_unref();
 +
 +    /*
 +     * Free the dynamically allocated host string etc
 +     */
 +    g_free(s->vdisk_hostinfo.host);
 +    g_free(s->tlscredsid);
 +    s->tlscredsid = NULL;
 +    s->vdisk_hostinfo.host = NULL;
 +    s->vdisk_hostinfo.port = 0;
 +}
 +
 +static int64_t vxhs_get_vdisk_stat(BDRVVXHSState *s)
 +{
 +    int64_t vdisk_size = -1;
 +    int ret = 0;
 +    void *dev_handle = s->vdisk_hostinfo.dev_handle;
 +
 +    ret = iio_ioctl(dev_handle, IOR_VDISK_STAT, &vdisk_size, 0);
 +    if (ret < 0) {
 +        trace_vxhs_get_vdisk_stat_err(s->vdisk_guid, ret, errno);
 +        return -EIO;
 +    }
 +
 +    trace_vxhs_get_vdisk_stat(s->vdisk_guid, vdisk_size);
 +    return vdisk_size;
 +}
 +
 +/*
 + * Returns the size of vDisk in bytes. This is required
 + * by QEMU block upper block layer so that it is visible
 + * to guest.
 + */
 +static int64_t vxhs_getlength(BlockDriverState *bs)
 +{
 +    BDRVVXHSState *s = bs->opaque;
 +    int64_t vdisk_size;
 +
 +    vdisk_size = vxhs_get_vdisk_stat(s);
 +    if (vdisk_size < 0) {
 +        return -EIO;
 +    }
 +
 +    return vdisk_size;
 +}
 +
 +static BlockDriver bdrv_vxhs = {
 +    .format_name                  = "vxhs",
 +    .protocol_name                = "vxhs",
 +    .instance_size                = sizeof(BDRVVXHSState),
 +    .bdrv_file_open               = vxhs_open,
 +    .bdrv_parse_filename          = vxhs_parse_filename,
 +    .bdrv_close                   = vxhs_close,
 +    .bdrv_getlength               = vxhs_getlength,
 +    .bdrv_aio_readv               = vxhs_aio_readv,
 +    .bdrv_aio_writev              = vxhs_aio_writev,
 +};
 +
 +static void bdrv_vxhs_init(void)
 +{
 +    bdrv_register(&bdrv_vxhs);
 +}
 +
 +block_init(bdrv_vxhs_init);
 diff --git a/configure b/configure
 index XXXXXXX..XXXXXXX 100755
 --- a/configure
 +++ b/configure
@@ -XXX,XX +XXX,XX @@ numa=""
  tcmalloc="no"
  jemalloc="no"
  replication="yes"
 +vxhs=""
  supported_cpu="no"
  supported_os="no"
@@ -XXX,XX +XXX,XX @@ for opt do
    ;;
    --enable-replication) replication="yes"
    ;;
 +  --disable-vxhs) vxhs="no"
 +  ;;
 +  --enable-vxhs) vxhs="yes"
 +  ;;
    *)
        echo "ERROR: unknown option $opt"
        echo "Try '$0 --help' for more information"
@@ -XXX,XX +XXX,XX @@ disabled with --disable-FEATURE, default is enabled if available:
    xfsctl          xfsctl support
    qom-cast-debug  cast debugging support
    tools           build qemu-io, qemu-nbd and qemu-image tools
 +  vxhs            Veritas HyperScale vDisk backend support
  NOTE: The object files are built at the place where configure is launched
  EOF
@@ -XXX,XX +XXX,XX @@ if compile_prog "" "" ; then
  fi
  ##########################################
 +# Veritas HyperScale block driver VxHS
 +# Check if libvxhs is installed
 +
 +if test "$vxhs" != "no" ; then
 +  cat > $TMPC <<EOF
 +#include <stdint.h>
 +#include <qnio/qnio_api.h>
 +
 +void *vxhs_callback;
 +
 +int main(void) {
 +    iio_init(QNIO_VERSION, vxhs_callback);
 +    return 0;
 +}
 +EOF
 +  vxhs_libs="-lvxhs -lssl"
 +  if compile_prog "" "$vxhs_libs" ; then
 +    vxhs=yes
 +  else
 +    if test "$vxhs" = "yes" ; then
 +      feature_not_found "vxhs block device" "Install libvxhs See github"
 +    fi
 +    vxhs=no
 +  fi
 +fi
 +
 +##########################################
  # End of CC checks
  # After here, no more $cc or $ld runs
@@ -XXX,XX +XXX,XX @@ echo "tcmalloc support  $tcmalloc"
  echo "jemalloc support  $jemalloc"
  echo "avx2 optimization $avx2_opt"
  echo "replication support $replication"
 +echo "VxHS block device $vxhs"
  if test "$sdl_too_old" = "yes"; then
  echo "-> Your SDL version is too old - please upgrade to have SDL support"
@@ -XXX,XX +XXX,XX @@ if test "$pthread_setname_np" = "yes" ; then
    echo "CONFIG_PTHREAD_SETNAME_NP=y" >> $config_host_mak
  fi
 +if test "$vxhs" = "yes" ; then
 +  echo "CONFIG_VXHS=y" >> $config_host_mak
 +  echo "VXHS_LIBS=$vxhs_libs" >> $config_host_mak
 +fi
 +
  if test "$tcg_interpreter" = "yes"; then
    QEMU_INCLUDES="-I\$(SRC_PATH)/tcg/tci $QEMU_INCLUDES"
  elif test "$ARCH" = "sparc64" ; then
 diff --git a/qapi/block-core.json b/qapi/block-core.json
 index XXXXXXX..XXXXXXX 100644
 --- a/qapi/block-core.json
 +++ b/qapi/block-core.json
@@ -XXX,XX +XXX,XX @@
  #
  # Drivers that are supported in block device operations.
  #
 +# @vxhs: Since 2.10
 +#
  # Since: 2.9
  ##
  { 'enum': 'BlockdevDriver',
@@ -XXX,XX +XXX,XX @@
              'host_device', 'http', 'https', 'iscsi', 'luks', 'nbd', 'nfs',
              'null-aio', 'null-co', 'parallels', 'qcow', 'qcow2', 'qed',
              'quorum', 'raw', 'rbd', 'replication', 'sheepdog', 'ssh',
 -            'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] }
 +            'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat', 'vxhs' ] }
  ##
  # @BlockdevOptionsFile:
@@ -XXX,XX +XXX,XX @@
    'data': { '*offset': 'int', '*size': 'int' } }
  ##
 +# @BlockdevOptionsVxHS:
 +#
 +# Driver specific block device options for VxHS
 +#
 +# @vdisk-id:    UUID of VxHS volume
 +# @server:      vxhs server IP, port
 +# @tls-creds:   TLS credentials ID
 +#
 +# Since: 2.10
 +##
 +{ 'struct': 'BlockdevOptionsVxHS',
 +  'data': { 'vdisk-id': 'str',
 +            'server': 'InetSocketAddressBase',
 +            '*tls-creds': 'str' } }
 +
 +##
  # @BlockdevOptions:
  #
  # Options for creating a block device.  Many options are available for all
@@ -XXX,XX +XXX,XX @@
        'vhdx':       'BlockdevOptionsGenericFormat',
        'vmdk':       'BlockdevOptionsGenericCOWFormat',
        'vpc':        'BlockdevOptionsGenericFormat',
 -      'vvfat':      'BlockdevOptionsVVFAT'
 +      'vvfat':      'BlockdevOptionsVVFAT',
 +      'vxhs':       'BlockdevOptionsVxHS'
    } }
  ##
 --
-.9.3
+.33.1

-[Qemu-devel] [PULL 11/12] block/rbd: Add support for reopen()
+[PULL v2 13/13] file-posix: Fix alignment after reopen changing O_DIRECT
-This adds support for reopen in rbd, for changing between r/w and r/o.
+From: Kevin Wolf <kwolf@redhat.com>
-Note, that this is only a flag change, but we will block a change from
+At the end of a reopen, we already call bdrv_refresh_limits(), which
-r/o to r/w if we are using an RBD internal snapshot.
+should update bs->request_alignment according to the new file
 descriptor. However, raw_probe_alignment() relies on s->needs_alignment
 and just uses 1 if it isn't set. We neglected to update this field, so
 starting with cache=writeback and then reopening with cache=none means
 that we get an incorrect bs->request_alignment == 1 and unaligned
 requests fail instead of being automatically aligned.
-Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
+Fix this by recalculating s->needs_alignment in raw_refresh_limits()
-Signed-off-by: Jeff Cody <jcody@redhat.com>
+before calling raw_probe_alignment().
-Reviewed-by: John Snow <jsnow@redhat.com>
-Message-id: d4e87539167ec6527d44c97b164eabcccf96e4f3.1491597120.git.jcody@redhat.com
+Signed-off-by: Kevin Wolf <kwolf@redhat.com>
 Message-Id: <20211104113109.56336-1-kwolf@redhat.com>
 Reviewed-by: Hanna Reitz <hreitz@redhat.com>
 Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
 Signed-off-by: Kevin Wolf <kwolf@redhat.com>
 Message-Id: <20211115145409.176785-13-kwolf@redhat.com>
 [hreitz: Fix iotest 142 for block sizes greater than 512 by operating on
          a file with a size of 1 MB]
 Signed-off-by: Hanna Reitz <hreitz@redhat.com>
 Message-Id: <20211116101431.105252-1-hreitz@redhat.com>
 ---
- block/rbd.c | 21 +++++++++++++++++++++
+ block/file-posix.c         | 20 ++++++++++++++++----
-file changed, 21 insertions(+)
+ tests/qemu-iotests/142     | 29 +++++++++++++++++++++++++++++
  tests/qemu-iotests/142.out | 18 ++++++++++++++++++
 files changed, 63 insertions(+), 4 deletions(-)
-diff --git a/block/rbd.c b/block/rbd.c
+diff --git a/block/file-posix.c b/block/file-posix.c
 index XXXXXXX..XXXXXXX 100644
---- a/block/rbd.c
+--- a/block/file-posix.c
-+++ b/block/rbd.c
++++ b/block/file-posix.c
-@@ -XXX,XX +XXX,XX @@ failed_opts:
+@@ -XXX,XX +XXX,XX @@ typedef struct BDRVRawState {
-     return r;
+     int page_cache_inconsistent; /* errno from fdatasync failure */
      bool has_fallocate;
      bool needs_alignment;
 +    bool force_alignment;
      bool drop_cache;
      bool check_cache_dropped;
      struct {
@@ -XXX,XX +XXX,XX @@ static bool dio_byte_aligned(int fd)
      return false;
  }
++static bool raw_needs_alignment(BlockDriverState *bs)
++{
++    BDRVRawState *s = bs->opaque;
 +
-+/* Since RBD is currently always opened R/W via the API,
++    if ((bs->open_flags & BDRV_O_NOCACHE) != 0 && !dio_byte_aligned(s->fd)) {
-+ * we just need to check if we are using a snapshot or not, in
++        return true;
 + * order to determine if we will allow it to be R/W */
 +static int qemu_rbd_reopen_prepare(BDRVReopenState *state,
 +                                   BlockReopenQueue *queue, Error **errp)
 +{
 +    BDRVRBDState *s = state->bs->opaque;
 +    int ret = 0;
 +
 +    if (s->snap && state->flags & BDRV_O_RDWR) {
 +        error_setg(errp,
 +                   "Cannot change node '%s' to r/w when using RBD snapshot",
 +                   bdrv_get_device_or_node_name(state->bs));
 +        ret = -EINVAL;
 +    }
 +
-+    return ret;
++    return s->force_alignment;
 +}
 +
- static void qemu_rbd_close(BlockDriverState *bs)
+ /* Check if read is allowed with given memory buffer and length.
- {
+  *
-     BDRVRBDState *s = bs->opaque;
+  * This function is used to check O_DIRECT memory buffer and request alignment.
-@@ -XXX,XX +XXX,XX @@ static BlockDriver bdrv_rbd = {
+@@ -XXX,XX +XXX,XX @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
-     .bdrv_parse_filename    = qemu_rbd_parse_filename,
-     .bdrv_file_open         = qemu_rbd_open,
+     s->has_discard = true;
-     .bdrv_close             = qemu_rbd_close,
+     s->has_write_zeroes = true;
-+    .bdrv_reopen_prepare    = qemu_rbd_reopen_prepare,
+-    if ((bs->open_flags & BDRV_O_NOCACHE) != 0 && !dio_byte_aligned(s->fd)) {
-     .bdrv_create            = qemu_rbd_create,
+-        s->needs_alignment = true;
-     .bdrv_has_zero_init     = bdrv_has_zero_init_1,
+-    }
-     .bdrv_get_info          = qemu_rbd_getinfo,
      if (fstat(s->fd, &st) < 0) {
          ret = -errno;
@@ -XXX,XX +XXX,XX @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
           * so QEMU makes sure all IO operations on the device are aligned
           * to sector size, or else FreeBSD will reject them with EINVAL.
           */
 -        s->needs_alignment = true;
 +        s->force_alignment = true;
      }
  #endif
 +    s->needs_alignment = raw_needs_alignment(bs);
  #ifdef CONFIG_XFS
      if (platform_test_xfs_fd(s->fd)) {
@@ -XXX,XX +XXX,XX @@ static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
      BDRVRawState *s = bs->opaque;
      struct stat st;
 +    s->needs_alignment = raw_needs_alignment(bs);
      raw_probe_alignment(bs, s->fd, errp);
 +
      bs->bl.min_mem_alignment = s->buf_align;
      bs->bl.opt_mem_alignment = MAX(s->buf_align, qemu_real_host_page_size);
 diff --git a/tests/qemu-iotests/142 b/tests/qemu-iotests/142
 index XXXXXXX..XXXXXXX 100755
 --- a/tests/qemu-iotests/142
 +++ b/tests/qemu-iotests/142
@@ -XXX,XX +XXX,XX @@ info block backing-file"
  echo "$hmp_cmds" | run_qemu -drive "$files","$ids" | grep "Cache"
 +echo
 +echo "--- Alignment after changing O_DIRECT ---"
 +echo
 +
 +# Directly test the protocol level: Can unaligned requests succeed even if
 +# O_DIRECT was only enabled through a reopen and vice versa?
 +
 +# Ensure image size is a multiple of the sector size (required for O_DIRECT)
 +$QEMU_IMG create -f file "$TEST_IMG" 1M | _filter_img_create
 +
 +# And write some data (not strictly necessary, but it feels better to actually
 +# have something to be read)
 +$QEMU_IO -f file -c 'write 0 4096' "$TEST_IMG" | _filter_qemu_io
 +
 +$QEMU_IO --cache=writeback -f file $TEST_IMG <<EOF | _filter_qemu_io
 +read 42 42
 +reopen -o cache.direct=on
 +read 42 42
 +reopen -o cache.direct=off
 +read 42 42
 +EOF
 +$QEMU_IO --cache=none -f file $TEST_IMG <<EOF | _filter_qemu_io
 +read 42 42
 +reopen -o cache.direct=off
 +read 42 42
 +reopen -o cache.direct=on
 +read 42 42
 +EOF
 +
  # success, all done
  echo "*** done"
  rm -f $seq.full
 diff --git a/tests/qemu-iotests/142.out b/tests/qemu-iotests/142.out
 index XXXXXXX..XXXXXXX 100644
 --- a/tests/qemu-iotests/142.out
 +++ b/tests/qemu-iotests/142.out
@@ -XXX,XX +XXX,XX @@ cache.no-flush=on on backing-file
      Cache mode:       writeback
      Cache mode:       writeback, direct
      Cache mode:       writeback, ignore flushes
 +
 +--- Alignment after changing O_DIRECT ---
 +
 +Formatting 'TEST_DIR/t.IMGFMT', fmt=file size=1048576
 +wrote 4096/4096 bytes at offset 0
 +4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 +read 42/42 bytes at offset 42
 +42 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 +read 42/42 bytes at offset 42
 +42 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 +read 42/42 bytes at offset 42
 +42 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 +read 42/42 bytes at offset 42
 +42 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 +read 42/42 bytes at offset 42
 +42 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 +read 42/42 bytes at offset 42
 +42 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
  *** done
 --
-.9.3
+.33.1

The following changes since commit bfec359afba088aaacc7d316f43302f28c6e642a:

Merge remote-tracking branch 'remotes/armbru/tags/pull-qdev-2017-04-21' into staging (2017-04-21 11:42:03 +0100)

are available in the git repository at:

git://github.com/codyprime/qemu-kvm-jtc.git tags/block-pull-request

for you to fetch changes up to 1507631e438930bc07f776f303af127a9cdb4d41:

qemu-iotests: _cleanup_qemu must be called on exit (2017-04-21 08:32:44 -0400)

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

Block patches for 2.10

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

Ashish Mittal (2):
  block/vxhs.c: Add support for a new block device type called "vxhs"
  block/vxhs.c: Add qemu-iotests for new block device type "vxhs"

Jeff Cody (10):
  qemu-iotests: exclude vxhs from image creation via protocol
  block: add bdrv_set_read_only() helper function
  block: do not set BDS read_only if copy_on_read enabled
  block: honor BDRV_O_ALLOW_RDWR when clearing bs->read_only
  block: code movement
  block: introduce bdrv_can_set_read_only()
  block: use bdrv_can_set_read_only() during reopen
  block/rbd - update variable names to more apt names
  block/rbd: Add support for reopen()
  qemu-iotests: _cleanup_qemu must be called on exit

-- 
2.9.3

From: Ashish Mittal <ashmit602@gmail.com>

Source code for the qnio library that this code loads can be downloaded from:
https://github.com/VeritasHyperScale/libqnio.git

Sample command line using JSON syntax:
./x86_64-softmmu/qemu-system-x86_64 -name instance-00000008 -S -vnc 0.0.0.0:0
-k en-us -vga cirrus -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x5
-msg timestamp=on
'json:{"driver":"vxhs","vdisk-id":"c3e9095a-a5ee-4dce-afeb-2a59fb387410",
"server":{"host":"172.172.17.4","port":"9999"}}'

Sample command line using URI syntax:
qemu-img convert -f raw -O raw -n
/var/lib/nova/instances/_base/0c5eacd5ebea5ed914b6a3e7b18f1ce734c386ad
vxhs://192.168.0.1:9999/c6718f6b-0401-441d-a8c3-1f0064d75ee0

Sample command line using TLS credentials (run in secure mode):
./qemu-io --object
tls-creds-x509,id=tls0,dir=/etc/pki/qemu/vxhs,endpoint=client -c 'read
-v 66000 2.5k' 'json:{"server.host": "127.0.0.1", "server.port": "9999",
"vdisk-id": "/test.raw", "driver": "vxhs", "tls-creds":"tls0"}'

Signed-off-by: Ashish Mittal <Ashish.Mittal@veritas.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Jeff Cody <jcody@redhat.com>
Signed-off-by: Jeff Cody <jcody@redhat.com>
Message-id: 1491277689-24949-2-git-send-email-Ashish.Mittal@veritas.com
---
 block/Makefile.objs  |   2 +
 block/trace-events   |  17 ++
 block/vxhs.c         | 575 +++++++++++++++++++++++++++++++++++++++++++++++++++
 configure            |  39 ++++
 qapi/block-core.json |  23 ++-
 5 files changed, 654 insertions(+), 2 deletions(-)
 create mode 100644 block/vxhs.c

diff --git a/block/Makefile.objs b/block/Makefile.objs
index XXXXXXX..XXXXXXX 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -XXX,XX +XXX,XX @@ block-obj-$(CONFIG_LIBNFS) += nfs.o
 block-obj-$(CONFIG_CURL) += curl.o
 block-obj-$(CONFIG_RBD) += rbd.o
 block-obj-$(CONFIG_GLUSTERFS) += gluster.o
+block-obj-$(CONFIG_VXHS) += vxhs.o
 block-obj-$(CONFIG_LIBSSH2) += ssh.o
 block-obj-y += accounting.o dirty-bitmap.o
 block-obj-y += write-threshold.o
@@ -XXX,XX +XXX,XX @@ rbd.o-cflags       := $(RBD_CFLAGS)
 rbd.o-libs         := $(RBD_LIBS)
 gluster.o-cflags   := $(GLUSTERFS_CFLAGS)
 gluster.o-libs     := $(GLUSTERFS_LIBS)
+vxhs.o-libs        := $(VXHS_LIBS)
 ssh.o-cflags       := $(LIBSSH2_CFLAGS)
 ssh.o-libs         := $(LIBSSH2_LIBS)
 block-obj-$(if $(CONFIG_BZIP2),m,n) += dmg-bz2.o
diff --git a/block/trace-events b/block/trace-events
index XXXXXXX..XXXXXXX 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -XXX,XX +XXX,XX @@ qed_aio_write_data(void *s, void *acb, int ret, uint64_t offset, size_t len) "s
 qed_aio_write_prefill(void *s, void *acb, uint64_t start, size_t len, uint64_t offset) "s %p acb %p start %"PRIu64" len %zu offset %"PRIu64
 qed_aio_write_postfill(void *s, void *acb, uint64_t start, size_t len, uint64_t offset) "s %p acb %p start %"PRIu64" len %zu offset %"PRIu64
 qed_aio_write_main(void *s, void *acb, int ret, uint64_t offset, size_t len) "s %p acb %p ret %d offset %"PRIu64" len %zu"
+
+# block/vxhs.c
+vxhs_iio_callback(int error) "ctx is NULL: error %d"
+vxhs_iio_callback_chnfail(int err, int error) "QNIO channel failed, no i/o %d, %d"
+vxhs_iio_callback_unknwn(int opcode, int err) "unexpected opcode %d, errno %d"
+vxhs_aio_rw_invalid(int req) "Invalid I/O request iodir %d"
+vxhs_aio_rw_ioerr(char *guid, int iodir, uint64_t size, uint64_t off, void *acb, int ret, int err) "IO ERROR (vDisk %s) FOR : Read/Write = %d size = %lu offset = %lu ACB = %p. Error = %d, errno = %d"
+vxhs_get_vdisk_stat_err(char *guid, int ret, int err) "vDisk (%s) stat ioctl failed, ret = %d, errno = %d"
+vxhs_get_vdisk_stat(char *vdisk_guid, uint64_t vdisk_size) "vDisk %s stat ioctl returned size %lu"
+vxhs_complete_aio(void *acb, uint64_t ret) "aio failed acb %p ret %ld"
+vxhs_parse_uri_filename(const char *filename) "URI passed via bdrv_parse_filename %s"
+vxhs_open_vdiskid(const char *vdisk_id) "Opening vdisk-id %s"
+vxhs_open_hostinfo(char *of_vsa_addr, int port) "Adding host %s:%d to BDRVVXHSState"
+vxhs_open_iio_open(const char *host) "Failed to connect to storage agent on host %s"
+vxhs_parse_uri_hostinfo(char *host, int port) "Host: IP %s, Port %d"
+vxhs_close(char *vdisk_guid) "Closing vdisk %s"
+vxhs_get_creds(const char *cacert, const char *client_key, const char *client_cert) "cacert %s, client_key %s, client_cert %s"
diff --git a/block/vxhs.c b/block/vxhs.c
new file mode 100644
index XXXXXXX..XXXXXXX
--- /dev/null
+++ b/block/vxhs.c
@@ -XXX,XX +XXX,XX @@
+/*
+ * QEMU Block driver for Veritas HyperScale (VxHS)
+ *
+ * Copyright (c) 2017 Veritas Technologies LLC.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include <qnio/qnio_api.h>
+#include <sys/param.h>
+#include "block/block_int.h"
+#include "qapi/qmp/qerror.h"
+#include "qapi/qmp/qdict.h"
+#include "qapi/qmp/qstring.h"
+#include "trace.h"
+#include "qemu/uri.h"
+#include "qapi/error.h"
+#include "qemu/uuid.h"
+#include "crypto/tlscredsx509.h"
+
+#define VXHS_OPT_FILENAME           "filename"
+#define VXHS_OPT_VDISK_ID           "vdisk-id"
+#define VXHS_OPT_SERVER             "server"
+#define VXHS_OPT_HOST               "host"
+#define VXHS_OPT_PORT               "port"
+
+/* Only accessed under QEMU global mutex */
+static uint32_t vxhs_ref;
+
+typedef enum {
+    VDISK_AIO_READ,
+    VDISK_AIO_WRITE,
+} VDISKAIOCmd;
+
+/*
+ * HyperScale AIO callbacks structure
+ */
+typedef struct VXHSAIOCB {
+    BlockAIOCB common;
+    int err;
+} VXHSAIOCB;
+
+typedef struct VXHSvDiskHostsInfo {
+    void *dev_handle; /* Device handle */
+    char *host; /* Host name or IP */
+    int port; /* Host's port number */
+} VXHSvDiskHostsInfo;
+
+/*
+ * Structure per vDisk maintained for state
+ */
+typedef struct BDRVVXHSState {
+    VXHSvDiskHostsInfo vdisk_hostinfo; /* Per host info */
+    char *vdisk_guid;
+    char *tlscredsid; /* tlscredsid */
+} BDRVVXHSState;
+
+static void vxhs_complete_aio_bh(void *opaque)
+{
+    VXHSAIOCB *acb = opaque;
+    BlockCompletionFunc *cb = acb->common.cb;
+    void *cb_opaque = acb->common.opaque;
+    int ret = 0;
+
+    if (acb->err != 0) {
+        trace_vxhs_complete_aio(acb, acb->err);
+        ret = (-EIO);
+    }
+
+    qemu_aio_unref(acb);
+    cb(cb_opaque, ret);
+}
+
+/*
+ * Called from a libqnio thread
+ */
+static void vxhs_iio_callback(void *ctx, uint32_t opcode, uint32_t error)
+{
+    VXHSAIOCB *acb = NULL;
+
+    switch (opcode) {
+    case IRP_READ_REQUEST:
+    case IRP_WRITE_REQUEST:
+
+        /*
+         * ctx is VXHSAIOCB*
+         * ctx is NULL if error is QNIOERROR_CHANNEL_HUP
+         */
+        if (ctx) {
+            acb = ctx;
+        } else {
+            trace_vxhs_iio_callback(error);
+            goto out;
+        }
+
+        if (error) {
+            if (!acb->err) {
+                acb->err = error;
+            }
+            trace_vxhs_iio_callback(error);
+        }
+
+        aio_bh_schedule_oneshot(bdrv_get_aio_context(acb->common.bs),
+                                vxhs_complete_aio_bh, acb);
+        break;
+
+    default:
+        if (error == QNIOERROR_HUP) {
+            /*
+             * Channel failed, spontaneous notification,
+             * not in response to I/O
+             */
+            trace_vxhs_iio_callback_chnfail(error, errno);
+        } else {
+            trace_vxhs_iio_callback_unknwn(opcode, error);
+        }
+        break;
+    }
+out:
+    return;
+}
+
+static QemuOptsList runtime_opts = {
+    .name = "vxhs",
+    .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
+    .desc = {
+        {
+            .name = VXHS_OPT_FILENAME,
+            .type = QEMU_OPT_STRING,
+            .help = "URI to the Veritas HyperScale image",
+        },
+        {
+            .name = VXHS_OPT_VDISK_ID,
+            .type = QEMU_OPT_STRING,
+            .help = "UUID of the VxHS vdisk",
+        },
+        {
+            .name = "tls-creds",
+            .type = QEMU_OPT_STRING,
+            .help = "ID of the TLS/SSL credentials to use",
+        },
+        { /* end of list */ }
+    },
+};
+
+static QemuOptsList runtime_tcp_opts = {
+    .name = "vxhs_tcp",
+    .head = QTAILQ_HEAD_INITIALIZER(runtime_tcp_opts.head),
+    .desc = {
+        {
+            .name = VXHS_OPT_HOST,
+            .type = QEMU_OPT_STRING,
+            .help = "host address (ipv4 addresses)",
+        },
+        {
+            .name = VXHS_OPT_PORT,
+            .type = QEMU_OPT_NUMBER,
+            .help = "port number on which VxHSD is listening (default 9999)",
+            .def_value_str = "9999"
+        },
+        { /* end of list */ }
+    },
+};
+
+/*
+ * Parse incoming URI and populate *options with the host
+ * and device information
+ */
+static int vxhs_parse_uri(const char *filename, QDict *options)
+{
+    URI *uri = NULL;
+    char *port;
+    int ret = 0;
+
+    trace_vxhs_parse_uri_filename(filename);
+    uri = uri_parse(filename);
+    if (!uri || !uri->server || !uri->path) {
+        uri_free(uri);
+        return -EINVAL;
+    }
+
+    qdict_put(options, VXHS_OPT_SERVER".host", qstring_from_str(uri->server));
+
+    if (uri->port) {
+        port = g_strdup_printf("%d", uri->port);
+        qdict_put(options, VXHS_OPT_SERVER".port", qstring_from_str(port));
+        g_free(port);
+    }
+
+    qdict_put(options, "vdisk-id", qstring_from_str(uri->path));
+
+    trace_vxhs_parse_uri_hostinfo(uri->server, uri->port);
+    uri_free(uri);
+
+    return ret;
+}
+
+static void vxhs_parse_filename(const char *filename, QDict *options,
+                                Error **errp)
+{
+    if (qdict_haskey(options, "vdisk-id") || qdict_haskey(options, "server")) {
+        error_setg(errp, "vdisk-id/server and a file name may not be specified "
+                         "at the same time");
+        return;
+    }
+
+    if (strstr(filename, "://")) {
+        int ret = vxhs_parse_uri(filename, options);
+        if (ret < 0) {
+            error_setg(errp, "Invalid URI. URI should be of the form "
+                       "  vxhs://<host_ip>:<port>/<vdisk-id>");
+        }
+    }
+}
+
+static int vxhs_init_and_ref(void)
+{
+    if (vxhs_ref++ == 0) {
+        if (iio_init(QNIO_VERSION, vxhs_iio_callback)) {
+            return -ENODEV;
+        }
+    }
+    return 0;
+}
+
+static void vxhs_unref(void)
+{
+    if (--vxhs_ref == 0) {
+        iio_fini();
+    }
+}
+
+static void vxhs_get_tls_creds(const char *id, char **cacert,
+                               char **key, char **cert, Error **errp)
+{
+    Object *obj;
+    QCryptoTLSCreds *creds;
+    QCryptoTLSCredsX509 *creds_x509;
+
+    obj = object_resolve_path_component(
+        object_get_objects_root(), id);
+
+    if (!obj) {
+        error_setg(errp, "No TLS credentials with id '%s'",
+                   id);
+        return;
+    }
+
+    creds_x509 = (QCryptoTLSCredsX509 *)
+        object_dynamic_cast(obj, TYPE_QCRYPTO_TLS_CREDS_X509);
+
+    if (!creds_x509) {
+        error_setg(errp, "Object with id '%s' is not TLS credentials",
+                   id);
+        return;
+    }
+
+    creds = &creds_x509->parent_obj;
+
+    if (creds->endpoint != QCRYPTO_TLS_CREDS_ENDPOINT_CLIENT) {
+        error_setg(errp,
+                   "Expecting TLS credentials with a client endpoint");
+        return;
+    }
+
+    /*
+     * Get the cacert, client_cert and client_key file names.
+     */
+    if (!creds->dir) {
+        error_setg(errp, "TLS object missing 'dir' property value");
+        return;
+    }
+
+    *cacert = g_strdup_printf("%s/%s", creds->dir,
+                              QCRYPTO_TLS_CREDS_X509_CA_CERT);
+    *cert = g_strdup_printf("%s/%s", creds->dir,
+                            QCRYPTO_TLS_CREDS_X509_CLIENT_CERT);
+    *key = g_strdup_printf("%s/%s", creds->dir,
+                           QCRYPTO_TLS_CREDS_X509_CLIENT_KEY);
+}
+
+static int vxhs_open(BlockDriverState *bs, QDict *options,
+                     int bdrv_flags, Error **errp)
+{
+    BDRVVXHSState *s = bs->opaque;
+    void *dev_handlep;
+    QDict *backing_options = NULL;
+    QemuOpts *opts = NULL;
+    QemuOpts *tcp_opts = NULL;
+    char *of_vsa_addr = NULL;
+    Error *local_err = NULL;
+    const char *vdisk_id_opt;
+    const char *server_host_opt;
+    int ret = 0;
+    char *cacert = NULL;
+    char *client_key = NULL;
+    char *client_cert = NULL;
+
+    ret = vxhs_init_and_ref();
+    if (ret < 0) {
+        ret = -EINVAL;
+        goto out;
+    }
+
+    /* Create opts info from runtime_opts and runtime_tcp_opts list */
+    opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
+    tcp_opts = qemu_opts_create(&runtime_tcp_opts, NULL, 0, &error_abort);
+
+    qemu_opts_absorb_qdict(opts, options, &local_err);
+    if (local_err) {
+        ret = -EINVAL;
+        goto out;
+    }
+
+    /* vdisk-id is the disk UUID */
+    vdisk_id_opt = qemu_opt_get(opts, VXHS_OPT_VDISK_ID);
+    if (!vdisk_id_opt) {
+        error_setg(&local_err, QERR_MISSING_PARAMETER, VXHS_OPT_VDISK_ID);
+        ret = -EINVAL;
+        goto out;
+    }
+
+    /* vdisk-id may contain a leading '/' */
+    if (strlen(vdisk_id_opt) > UUID_FMT_LEN + 1) {
+        error_setg(&local_err, "vdisk-id cannot be more than %d characters",
+                   UUID_FMT_LEN);
+        ret = -EINVAL;
+        goto out;
+    }
+
+    s->vdisk_guid = g_strdup(vdisk_id_opt);
+    trace_vxhs_open_vdiskid(vdisk_id_opt);
+
+    /* get the 'server.' arguments */
+    qdict_extract_subqdict(options, &backing_options, VXHS_OPT_SERVER".");
+
+    qemu_opts_absorb_qdict(tcp_opts, backing_options, &local_err);
+    if (local_err != NULL) {
+        ret = -EINVAL;
+        goto out;
+    }
+
+    server_host_opt = qemu_opt_get(tcp_opts, VXHS_OPT_HOST);
+    if (!server_host_opt) {
+        error_setg(&local_err, QERR_MISSING_PARAMETER,
+                   VXHS_OPT_SERVER"."VXHS_OPT_HOST);
+        ret = -EINVAL;
+        goto out;
+    }
+
+    if (strlen(server_host_opt) > MAXHOSTNAMELEN) {
+        error_setg(&local_err, "server.host cannot be more than %d characters",
+                   MAXHOSTNAMELEN);
+        ret = -EINVAL;
+        goto out;
+    }
+
+    /* check if we got tls-creds via the --object argument */
+    s->tlscredsid = g_strdup(qemu_opt_get(opts, "tls-creds"));
+    if (s->tlscredsid) {
+        vxhs_get_tls_creds(s->tlscredsid, &cacert, &client_key,
+                           &client_cert, &local_err);
+        if (local_err != NULL) {
+            ret = -EINVAL;
+            goto out;
+        }
+        trace_vxhs_get_creds(cacert, client_key, client_cert);
+    }
+
+    s->vdisk_hostinfo.host = g_strdup(server_host_opt);
+    s->vdisk_hostinfo.port = g_ascii_strtoll(qemu_opt_get(tcp_opts,
+                                                          VXHS_OPT_PORT),
+                                                          NULL, 0);
+
+    trace_vxhs_open_hostinfo(s->vdisk_hostinfo.host,
+                             s->vdisk_hostinfo.port);
+
+    of_vsa_addr = g_strdup_printf("of://%s:%d",
+                                  s->vdisk_hostinfo.host,
+                                  s->vdisk_hostinfo.port);
+
+    /*
+     * Open qnio channel to storage agent if not opened before
+     */
+    dev_handlep = iio_open(of_vsa_addr, s->vdisk_guid, 0,
+                           cacert, client_key, client_cert);
+    if (dev_handlep == NULL) {
+        trace_vxhs_open_iio_open(of_vsa_addr);
+        ret = -ENODEV;
+        goto out;
+    }
+    s->vdisk_hostinfo.dev_handle = dev_handlep;
+
+out:
+    g_free(of_vsa_addr);
+    QDECREF(backing_options);
+    qemu_opts_del(tcp_opts);
+    qemu_opts_del(opts);
+    g_free(cacert);
+    g_free(client_key);
+    g_free(client_cert);
+
+    if (ret < 0) {
+        vxhs_unref();
+        error_propagate(errp, local_err);
+        g_free(s->vdisk_hostinfo.host);
+        g_free(s->vdisk_guid);
+        g_free(s->tlscredsid);
+        s->vdisk_guid = NULL;
+    }
+
+    return ret;
+}
+
+static const AIOCBInfo vxhs_aiocb_info = {
+    .aiocb_size = sizeof(VXHSAIOCB)
+};
+
+/*
+ * This allocates QEMU-VXHS callback for each IO
+ * and is passed to QNIO. When QNIO completes the work,
+ * it will be passed back through the callback.
+ */
+static BlockAIOCB *vxhs_aio_rw(BlockDriverState *bs, int64_t sector_num,
+                               QEMUIOVector *qiov, int nb_sectors,
+                               BlockCompletionFunc *cb, void *opaque,
+                               VDISKAIOCmd iodir)
+{
+    VXHSAIOCB *acb = NULL;
+    BDRVVXHSState *s = bs->opaque;
+    size_t size;
+    uint64_t offset;
+    int iio_flags = 0;
+    int ret = 0;
+    void *dev_handle = s->vdisk_hostinfo.dev_handle;
+
+    offset = sector_num * BDRV_SECTOR_SIZE;
+    size = nb_sectors * BDRV_SECTOR_SIZE;
+    acb = qemu_aio_get(&vxhs_aiocb_info, bs, cb, opaque);
+
+    /*
+     * Initialize VXHSAIOCB.
+     */
+    acb->err = 0;
+
+    iio_flags = IIO_FLAG_ASYNC;
+
+    switch (iodir) {
+    case VDISK_AIO_WRITE:
+            ret = iio_writev(dev_handle, acb, qiov->iov, qiov->niov,
+                             offset, (uint64_t)size, iio_flags);
+            break;
+    case VDISK_AIO_READ:
+            ret = iio_readv(dev_handle, acb, qiov->iov, qiov->niov,
+                            offset, (uint64_t)size, iio_flags);
+            break;
+    default:
+            trace_vxhs_aio_rw_invalid(iodir);
+            goto errout;
+    }
+
+    if (ret != 0) {
+        trace_vxhs_aio_rw_ioerr(s->vdisk_guid, iodir, size, offset,
+                                acb, ret, errno);
+        goto errout;
+    }
+    return &acb->common;
+
+errout:
+    qemu_aio_unref(acb);
+    return NULL;
+}
+
+static BlockAIOCB *vxhs_aio_readv(BlockDriverState *bs,
+                                   int64_t sector_num, QEMUIOVector *qiov,
+                                   int nb_sectors,
+                                   BlockCompletionFunc *cb, void *opaque)
+{
+    return vxhs_aio_rw(bs, sector_num, qiov, nb_sectors, cb,
+                       opaque, VDISK_AIO_READ);
+}
+
+static BlockAIOCB *vxhs_aio_writev(BlockDriverState *bs,
+                                   int64_t sector_num, QEMUIOVector *qiov,
+                                   int nb_sectors,
+                                   BlockCompletionFunc *cb, void *opaque)
+{
+    return vxhs_aio_rw(bs, sector_num, qiov, nb_sectors,
+                       cb, opaque, VDISK_AIO_WRITE);
+}
+
+static void vxhs_close(BlockDriverState *bs)
+{
+    BDRVVXHSState *s = bs->opaque;
+
+    trace_vxhs_close(s->vdisk_guid);
+
+    g_free(s->vdisk_guid);
+    s->vdisk_guid = NULL;
+
+    /*
+     * Close vDisk device
+     */
+    if (s->vdisk_hostinfo.dev_handle) {
+        iio_close(s->vdisk_hostinfo.dev_handle);
+        s->vdisk_hostinfo.dev_handle = NULL;
+    }
+
+    vxhs_unref();
+
+    /*
+     * Free the dynamically allocated host string etc
+     */
+    g_free(s->vdisk_hostinfo.host);
+    g_free(s->tlscredsid);
+    s->tlscredsid = NULL;
+    s->vdisk_hostinfo.host = NULL;
+    s->vdisk_hostinfo.port = 0;
+}
+
+static int64_t vxhs_get_vdisk_stat(BDRVVXHSState *s)
+{
+    int64_t vdisk_size = -1;
+    int ret = 0;
+    void *dev_handle = s->vdisk_hostinfo.dev_handle;
+
+    ret = iio_ioctl(dev_handle, IOR_VDISK_STAT, &vdisk_size, 0);
+    if (ret < 0) {
+        trace_vxhs_get_vdisk_stat_err(s->vdisk_guid, ret, errno);
+        return -EIO;
+    }
+
+    trace_vxhs_get_vdisk_stat(s->vdisk_guid, vdisk_size);
+    return vdisk_size;
+}
+
+/*
+ * Returns the size of vDisk in bytes. This is required
+ * by QEMU block upper block layer so that it is visible
+ * to guest.
+ */
+static int64_t vxhs_getlength(BlockDriverState *bs)
+{
+    BDRVVXHSState *s = bs->opaque;
+    int64_t vdisk_size;
+
+    vdisk_size = vxhs_get_vdisk_stat(s);
+    if (vdisk_size < 0) {
+        return -EIO;
+    }
+
+    return vdisk_size;
+}
+
+static BlockDriver bdrv_vxhs = {
+    .format_name                  = "vxhs",
+    .protocol_name                = "vxhs",
+    .instance_size                = sizeof(BDRVVXHSState),
+    .bdrv_file_open               = vxhs_open,
+    .bdrv_parse_filename          = vxhs_parse_filename,
+    .bdrv_close                   = vxhs_close,
+    .bdrv_getlength               = vxhs_getlength,
+    .bdrv_aio_readv               = vxhs_aio_readv,
+    .bdrv_aio_writev              = vxhs_aio_writev,
+};
+
+static void bdrv_vxhs_init(void)
+{
+    bdrv_register(&bdrv_vxhs);
+}
+
+block_init(bdrv_vxhs_init);
diff --git a/configure b/configure
index XXXXXXX..XXXXXXX 100755
--- a/configure
+++ b/configure
@@ -XXX,XX +XXX,XX @@ numa=""
 tcmalloc="no"
 jemalloc="no"
 replication="yes"
+vxhs=""
 
 supported_cpu="no"
 supported_os="no"
@@ -XXX,XX +XXX,XX @@ for opt do
   ;;
   --enable-replication) replication="yes"
   ;;
+  --disable-vxhs) vxhs="no"
+  ;;
+  --enable-vxhs) vxhs="yes"
+  ;;
   *)
       echo "ERROR: unknown option $opt"
       echo "Try '$0 --help' for more information"
@@ -XXX,XX +XXX,XX @@ disabled with --disable-FEATURE, default is enabled if available:
   xfsctl          xfsctl support
   qom-cast-debug  cast debugging support
   tools           build qemu-io, qemu-nbd and qemu-image tools
+  vxhs            Veritas HyperScale vDisk backend support
 
 NOTE: The object files are built at the place where configure is launched
 EOF
@@ -XXX,XX +XXX,XX @@ if compile_prog "" "" ; then
 fi
 
 ##########################################
+# Veritas HyperScale block driver VxHS
+# Check if libvxhs is installed
+
+if test "$vxhs" != "no" ; then
+  cat > $TMPC <<EOF
+#include <stdint.h>
+#include <qnio/qnio_api.h>
+
+void *vxhs_callback;
+
+int main(void) {
+    iio_init(QNIO_VERSION, vxhs_callback);
+    return 0;
+}
+EOF
+  vxhs_libs="-lvxhs -lssl"
+  if compile_prog "" "$vxhs_libs" ; then
+    vxhs=yes
+  else
+    if test "$vxhs" = "yes" ; then
+      feature_not_found "vxhs block device" "Install libvxhs See github"
+    fi
+    vxhs=no
+  fi
+fi
+
+##########################################
 # End of CC checks
 # After here, no more $cc or $ld runs
 
@@ -XXX,XX +XXX,XX @@ echo "tcmalloc support  $tcmalloc"
 echo "jemalloc support  $jemalloc"
 echo "avx2 optimization $avx2_opt"
 echo "replication support $replication"
+echo "VxHS block device $vxhs"
 
 if test "$sdl_too_old" = "yes"; then
 echo "-> Your SDL version is too old - please upgrade to have SDL support"
@@ -XXX,XX +XXX,XX @@ if test "$pthread_setname_np" = "yes" ; then
   echo "CONFIG_PTHREAD_SETNAME_NP=y" >> $config_host_mak
 fi
 
+if test "$vxhs" = "yes" ; then
+  echo "CONFIG_VXHS=y" >> $config_host_mak
+  echo "VXHS_LIBS=$vxhs_libs" >> $config_host_mak
+fi
+
 if test "$tcg_interpreter" = "yes"; then
   QEMU_INCLUDES="-I\$(SRC_PATH)/tcg/tci $QEMU_INCLUDES"
 elif test "$ARCH" = "sparc64" ; then
diff --git a/qapi/block-core.json b/qapi/block-core.json
index XXXXXXX..XXXXXXX 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -XXX,XX +XXX,XX @@
 #
 # Drivers that are supported in block device operations.
 #
+# @vxhs: Since 2.10
+#
 # Since: 2.9
 ##
 { 'enum': 'BlockdevDriver',
@@ -XXX,XX +XXX,XX @@
             'host_device', 'http', 'https', 'iscsi', 'luks', 'nbd', 'nfs',
             'null-aio', 'null-co', 'parallels', 'qcow', 'qcow2', 'qed',
             'quorum', 'raw', 'rbd', 'replication', 'sheepdog', 'ssh',
-            'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] }
+            'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat', 'vxhs' ] }
 
 ##
 # @BlockdevOptionsFile:
@@ -XXX,XX +XXX,XX @@
   'data': { '*offset': 'int', '*size': 'int' } }
 
 ##
+# @BlockdevOptionsVxHS:
+#
+# Driver specific block device options for VxHS
+#
+# @vdisk-id:    UUID of VxHS volume
+# @server:      vxhs server IP, port
+# @tls-creds:   TLS credentials ID
+#
+# Since: 2.10
+##
+{ 'struct': 'BlockdevOptionsVxHS',
+  'data': { 'vdisk-id': 'str',
+            'server': 'InetSocketAddressBase',
+            '*tls-creds': 'str' } }
+
+##
 # @BlockdevOptions:
 #
 # Options for creating a block device.  Many options are available for all
@@ -XXX,XX +XXX,XX @@
       'vhdx':       'BlockdevOptionsGenericFormat',
       'vmdk':       'BlockdevOptionsGenericCOWFormat',
       'vpc':        'BlockdevOptionsGenericFormat',
-      'vvfat':      'BlockdevOptionsVVFAT'
+      'vvfat':      'BlockdevOptionsVVFAT',
+      'vxhs':       'BlockdevOptionsVxHS'
   } }
 
 ##
-- 
2.9.3

From: Ashish Mittal <ashmit602@gmail.com>

These changes use a vxhs test server that is a part of the following
repository:
https://github.com/VeritasHyperScale/libqnio.git

Signed-off-by: Ashish Mittal <Ashish.Mittal@veritas.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Jeff Cody <jcody@redhat.com>
Signed-off-by: Jeff Cody <jcody@redhat.com>
Message-id: 1491277689-24949-3-git-send-email-Ashish.Mittal@veritas.com
---
 tests/qemu-iotests/common        |  6 ++++++
 tests/qemu-iotests/common.config | 13 +++++++++++++
 tests/qemu-iotests/common.filter |  1 +
 tests/qemu-iotests/common.rc     | 19 +++++++++++++++++++
 4 files changed, 39 insertions(+)

diff --git a/tests/qemu-iotests/common b/tests/qemu-iotests/common
index XXXXXXX..XXXXXXX 100644
--- a/tests/qemu-iotests/common
+++ b/tests/qemu-iotests/common
@@ -XXX,XX +XXX,XX @@ check options
     -ssh                test ssh
     -nfs                test nfs
     -luks               test luks
+    -vxhs               test vxhs
     -xdiff              graphical mode diff
     -nocache            use O_DIRECT on backing file
     -misalign           misalign memory allocations
@@ -XXX,XX +XXX,XX @@ testlist options
             xpand=false
             ;;
 
+        -vxhs)
+            IMGPROTO=vxhs
+            xpand=false
+            ;;
+
         -ssh)
             IMGPROTO=ssh
             xpand=false
diff --git a/tests/qemu-iotests/common.config b/tests/qemu-iotests/common.config
index XXXXXXX..XXXXXXX 100644
--- a/tests/qemu-iotests/common.config
+++ b/tests/qemu-iotests/common.config
@@ -XXX,XX +XXX,XX @@ if [ -z "$QEMU_NBD_PROG" ]; then
     export QEMU_NBD_PROG="`set_prog_path qemu-nbd`"
 fi
 
+if [ -z "$QEMU_VXHS_PROG" ]; then
+    export QEMU_VXHS_PROG="`set_prog_path qnio_server`"
+fi
+
 _qemu_wrapper()
 {
     (
@@ -XXX,XX +XXX,XX @@ _qemu_nbd_wrapper()
     )
 }
 
+_qemu_vxhs_wrapper()
+{
+    (
+        echo $BASHPID > "${TEST_DIR}/qemu-vxhs.pid"
+        exec "$QEMU_VXHS_PROG" $QEMU_VXHS_OPTIONS "$@"
+    )
+}
+
 export QEMU=_qemu_wrapper
 export QEMU_IMG=_qemu_img_wrapper
 export QEMU_IO=_qemu_io_wrapper
 export QEMU_NBD=_qemu_nbd_wrapper
+export QEMU_VXHS=_qemu_vxhs_wrapper
 
 QEMU_IMG_EXTRA_ARGS=
 if [ "$IMGOPTSSYNTAX" = "true" ]; then
diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
index XXXXXXX..XXXXXXX 100644
--- a/tests/qemu-iotests/common.filter
+++ b/tests/qemu-iotests/common.filter
@@ -XXX,XX +XXX,XX @@ _filter_img_info()
         -e "s#$TEST_DIR#TEST_DIR#g" \
         -e "s#$IMGFMT#IMGFMT#g" \
         -e 's#nbd://127.0.0.1:10810$#TEST_DIR/t.IMGFMT#g' \
+        -e 's#json.*vdisk-id.*vxhs"}}#TEST_DIR/t.IMGFMT#' \
         -e "/encrypted: yes/d" \
         -e "/cluster_size: [0-9]\\+/d" \
         -e "/table_size: [0-9]\\+/d" \
diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index XXXXXXX..XXXXXXX 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -XXX,XX +XXX,XX @@ else
     elif [ "$IMGPROTO" = "nfs" ]; then
         TEST_DIR="nfs://127.0.0.1/$TEST_DIR"
         TEST_IMG=$TEST_DIR/t.$IMGFMT
+    elif [ "$IMGPROTO" = "vxhs" ]; then
+        TEST_IMG_FILE=$TEST_DIR/t.$IMGFMT
+        TEST_IMG="vxhs://127.0.0.1:9999/t.$IMGFMT"
     else
         TEST_IMG=$IMGPROTO:$TEST_DIR/t.$IMGFMT
     fi
@@ -XXX,XX +XXX,XX @@ _make_test_img()
         eval "$QEMU_NBD -v -t -b 127.0.0.1 -p 10810 -f $IMGFMT  $TEST_IMG_FILE >/dev/null &"
         sleep 1 # FIXME: qemu-nbd needs to be listening before we continue
     fi
+
+    # Start QNIO server on image directory for vxhs protocol
+    if [ $IMGPROTO = "vxhs" ]; then
+        eval "$QEMU_VXHS -d  $TEST_DIR > /dev/null &"
+        sleep 1 # Wait for server to come up.
+    fi
 }
 
 _rm_test_img()
@@ -XXX,XX +XXX,XX @@ _cleanup_test_img()
             fi
             rm -f "$TEST_IMG_FILE"
             ;;
+        vxhs)
+            if [ -f "${TEST_DIR}/qemu-vxhs.pid" ]; then
+                local QEMU_VXHS_PID
+                read QEMU_VXHS_PID < "${TEST_DIR}/qemu-vxhs.pid"
+                kill ${QEMU_VXHS_PID} >/dev/null 2>&1
+                rm -f "${TEST_DIR}/qemu-vxhs.pid"
+            fi
+            rm -f "$TEST_IMG_FILE"
+            ;;
+
         file)
             _rm_test_img "$TEST_DIR/t.$IMGFMT"
             _rm_test_img "$TEST_DIR/t.$IMGFMT.orig"
-- 
2.9.3

The protocol VXHS does not support image creation.  Some tests expect
to be able to create images through the protocol.  Exclude VXHS from
these tests.

diff --git a/tests/qemu-iotests/017 b/tests/qemu-iotests/017
index XXXXXXX..XXXXXXX 100755
--- a/tests/qemu-iotests/017
+++ b/tests/qemu-iotests/017
@@ -XXX,XX +XXX,XX @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 # Any format supporting backing files
 _supported_fmt qcow qcow2 vmdk qed
 _supported_proto generic
+_unsupported_proto vxhs
 _supported_os Linux
 _unsupported_imgopts "subformat=monolithicFlat" "subformat=twoGbMaxExtentFlat"
 
diff --git a/tests/qemu-iotests/020 b/tests/qemu-iotests/020
index XXXXXXX..XXXXXXX 100755
--- a/tests/qemu-iotests/020
+++ b/tests/qemu-iotests/020
@@ -XXX,XX +XXX,XX @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 # Any format supporting backing files
 _supported_fmt qcow qcow2 vmdk qed
 _supported_proto generic
+_unsupported_proto vxhs
 _supported_os Linux
 _unsupported_imgopts "subformat=monolithicFlat" \
                      "subformat=twoGbMaxExtentFlat" \
diff --git a/tests/qemu-iotests/029 b/tests/qemu-iotests/029
index XXXXXXX..XXXXXXX 100755
--- a/tests/qemu-iotests/029
+++ b/tests/qemu-iotests/029
@@ -XXX,XX +XXX,XX @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 # Any format supporting intenal snapshots
 _supported_fmt qcow2
 _supported_proto generic
+_unsupported_proto vxhs
 _supported_os Linux
 # Internal snapshots are (currently) impossible with refcount_bits=1
 _unsupported_imgopts 'refcount_bits=1[^0-9]'
diff --git a/tests/qemu-iotests/073 b/tests/qemu-iotests/073
index XXXXXXX..XXXXXXX 100755
--- a/tests/qemu-iotests/073
+++ b/tests/qemu-iotests/073
@@ -XXX,XX +XXX,XX @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 
 _supported_fmt qcow2
 _supported_proto generic
+_unsupported_proto vxhs
 _supported_os Linux
 
 CLUSTER_SIZE=64k
diff --git a/tests/qemu-iotests/114 b/tests/qemu-iotests/114
index XXXXXXX..XXXXXXX 100755
--- a/tests/qemu-iotests/114
+++ b/tests/qemu-iotests/114
@@ -XXX,XX +XXX,XX @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 
 _supported_fmt qcow2
 _supported_proto generic
+_unsupported_proto vxhs
 _supported_os Linux
 
 
diff --git a/tests/qemu-iotests/130 b/tests/qemu-iotests/130
index XXXXXXX..XXXXXXX 100755
--- a/tests/qemu-iotests/130
+++ b/tests/qemu-iotests/130
@@ -XXX,XX +XXX,XX @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 
 _supported_fmt qcow2
 _supported_proto generic
+_unsupported_proto vxhs
 _supported_os Linux
 
 qemu_comm_method="monitor"
diff --git a/tests/qemu-iotests/134 b/tests/qemu-iotests/134
index XXXXXXX..XXXXXXX 100755
--- a/tests/qemu-iotests/134
+++ b/tests/qemu-iotests/134
@@ -XXX,XX +XXX,XX @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 
 _supported_fmt qcow2
 _supported_proto generic
+_unsupported_proto vxhs
 _supported_os Linux
 
 
diff --git a/tests/qemu-iotests/156 b/tests/qemu-iotests/156
index XXXXXXX..XXXXXXX 100755
--- a/tests/qemu-iotests/156
+++ b/tests/qemu-iotests/156
@@ -XXX,XX +XXX,XX @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 
 _supported_fmt qcow2 qed
 _supported_proto generic
+_unsupported_proto vxhs
 _supported_os Linux
 
 # Create source disk
diff --git a/tests/qemu-iotests/158 b/tests/qemu-iotests/158
index XXXXXXX..XXXXXXX 100755
--- a/tests/qemu-iotests/158
+++ b/tests/qemu-iotests/158
@@ -XXX,XX +XXX,XX @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 
 _supported_fmt qcow2
 _supported_proto generic
+_unsupported_proto vxhs
 _supported_os Linux
 
 
-- 
2.9.3

We have a helper wrapper for checking for the BDS read_only flag,
add a helper wrapper to set the read_only flag as well.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Jeff Cody <jcody@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Message-id: 9b18972d05f5fa2ac16c014f0af98d680553048d.1491597120.git.jcody@redhat.com
---
 block.c               | 5 +++++
 block/bochs.c         | 2 +-
 block/cloop.c         | 2 +-
 block/dmg.c           | 2 +-
 block/rbd.c           | 2 +-
 block/vvfat.c         | 4 ++--
 include/block/block.h | 1 +
 7 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index XXXXXXX..XXXXXXX 100644
--- a/block.c
+++ b/block.c
@@ -XXX,XX +XXX,XX @@ void path_combine(char *dest, int dest_size,
     }
 }
 
+void bdrv_set_read_only(BlockDriverState *bs, bool read_only)
+{
+    bs->read_only = read_only;
+}
+
 void bdrv_get_full_backing_filename_from_filename(const char *backed,
                                                   const char *backing,
                                                   char *dest, size_t sz,
diff --git a/block/bochs.c b/block/bochs.c
index XXXXXXX..XXXXXXX 100644
--- a/block/bochs.c
+++ b/block/bochs.c
@@ -XXX,XX +XXX,XX @@ static int bochs_open(BlockDriverState *bs, QDict *options, int flags,
         return -EINVAL;
     }
 
-    bs->read_only = true; /* no write support yet */
+    bdrv_set_read_only(bs, true); /* no write support yet */
 
     ret = bdrv_pread(bs->file, 0, &bochs, sizeof(bochs));
     if (ret < 0) {
diff --git a/block/cloop.c b/block/cloop.c
index XXXXXXX..XXXXXXX 100644
--- a/block/cloop.c
+++ b/block/cloop.c
@@ -XXX,XX +XXX,XX @@ static int cloop_open(BlockDriverState *bs, QDict *options, int flags,
         return -EINVAL;
     }
 
-    bs->read_only = true;
+    bdrv_set_read_only(bs, true);
 
     /* read header */
     ret = bdrv_pread(bs->file, 128, &s->block_size, 4);
diff --git a/block/dmg.c b/block/dmg.c
index XXXXXXX..XXXXXXX 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -XXX,XX +XXX,XX @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     block_module_load_one("dmg-bz2");
-    bs->read_only = true;
+    bdrv_set_read_only(bs, true);
 
     s->n_chunks = 0;
     s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL;
diff --git a/block/rbd.c b/block/rbd.c
index XXXXXXX..XXXXXXX 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -XXX,XX +XXX,XX @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
         goto failed_open;
     }
 
-    bs->read_only = (s->snap != NULL);
+    bdrv_set_read_only(bs, (s->snap != NULL));
 
     qemu_opts_del(opts);
     return 0;
diff --git a/block/vvfat.c b/block/vvfat.c
index XXXXXXX..XXXXXXX 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -XXX,XX +XXX,XX @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
     s->current_cluster=0xffffffff;
 
     /* read only is the default for safety */
-    bs->read_only = true;
+    bdrv_set_read_only(bs, true);
     s->qcow = NULL;
     s->qcow_filename = NULL;
     s->fat2 = NULL;
@@ -XXX,XX +XXX,XX @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
         if (ret < 0) {
             goto fail;
         }
-        bs->read_only = false;
+        bdrv_set_read_only(bs, false);
     }
 
     bs->total_sectors = cyls * heads * secs;
diff --git a/include/block/block.h b/include/block/block.h
index XXXXXXX..XXXXXXX 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -XXX,XX +XXX,XX @@ int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
                             int64_t sector_num, int nb_sectors, int *pnum);
 
 bool bdrv_is_read_only(BlockDriverState *bs);
+void bdrv_set_read_only(BlockDriverState *bs, bool read_only);
 bool bdrv_is_sg(BlockDriverState *bs);
 bool bdrv_is_inserted(BlockDriverState *bs);
 int bdrv_media_changed(BlockDriverState *bs);
-- 
2.9.3

A few block drivers will set the BDS read_only flag from their
.bdrv_open() function.  This means the bs->read_only flag could
be set after we enable copy_on_read, as the BDRV_O_COPY_ON_READ
flag check occurs prior to the call to bdrv->bdrv_open().

This adds an error return to bdrv_set_read_only(), and an error will be
return if we try to set the BDS to read_only while copy_on_read is
enabled.

This patch also changes the behavior of vvfat.  Before, vvfat could
override the drive 'readonly' flag with its own, internal 'rw' flag.

For instance, this -drive parameter would result in a writable image:

"-drive format=vvfat,dir=/tmp/vvfat,rw,if=virtio,readonly=on"

This is not correct.  Now, attempting to use the above -drive parameter
will result in an error (i.e., 'rw' is incompatible with 'readonly=on').

Signed-off-by: Jeff Cody <jcody@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Message-id: 0c5b4c1cc2c651471b131f21376dfd5ea24d2196.1491597120.git.jcody@redhat.com
---
 block.c               | 10 +++++++++-
 block/bochs.c         |  5 ++++-
 block/cloop.c         |  5 ++++-
 block/dmg.c           |  6 +++++-
 block/rbd.c           | 11 ++++++++++-
 block/vvfat.c         | 19 +++++++++++++++----
 include/block/block.h |  2 +-
 7 files changed, 48 insertions(+), 10 deletions(-)

diff --git a/block.c b/block.c
index XXXXXXX..XXXXXXX 100644
--- a/block.c
+++ b/block.c
@@ -XXX,XX +XXX,XX @@ void path_combine(char *dest, int dest_size,
     }
 }
 
-void bdrv_set_read_only(BlockDriverState *bs, bool read_only)
+int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
 {
+    /* Do not set read_only if copy_on_read is enabled */
+    if (bs->copy_on_read && read_only) {
+        error_setg(errp, "Can't set node '%s' to r/o with copy-on-read enabled",
+                   bdrv_get_device_or_node_name(bs));
+        return -EINVAL;
+    }
+
     bs->read_only = read_only;
+    return 0;
 }
 
 void bdrv_get_full_backing_filename_from_filename(const char *backed,
diff --git a/block/bochs.c b/block/bochs.c
index XXXXXXX..XXXXXXX 100644
--- a/block/bochs.c
+++ b/block/bochs.c
@@ -XXX,XX +XXX,XX @@ static int bochs_open(BlockDriverState *bs, QDict *options, int flags,
         return -EINVAL;
     }
 
-    bdrv_set_read_only(bs, true); /* no write support yet */
+    ret = bdrv_set_read_only(bs, true, errp); /* no write support yet */
+    if (ret < 0) {
+        return ret;
+    }
 
     ret = bdrv_pread(bs->file, 0, &bochs, sizeof(bochs));
     if (ret < 0) {
diff --git a/block/cloop.c b/block/cloop.c
index XXXXXXX..XXXXXXX 100644
--- a/block/cloop.c
+++ b/block/cloop.c
@@ -XXX,XX +XXX,XX @@ static int cloop_open(BlockDriverState *bs, QDict *options, int flags,
         return -EINVAL;
     }
 
-    bdrv_set_read_only(bs, true);
+    ret = bdrv_set_read_only(bs, true, errp);
+    if (ret < 0) {
+        return ret;
+    }
 
     /* read header */
     ret = bdrv_pread(bs->file, 128, &s->block_size, 4);
diff --git a/block/dmg.c b/block/dmg.c
index XXXXXXX..XXXXXXX 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -XXX,XX +XXX,XX @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
         return -EINVAL;
     }
 
+    ret = bdrv_set_read_only(bs, true, errp);
+    if (ret < 0) {
+        return ret;
+    }
+
     block_module_load_one("dmg-bz2");
-    bdrv_set_read_only(bs, true);
 
     s->n_chunks = 0;
     s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL;
diff --git a/block/rbd.c b/block/rbd.c
index XXXXXXX..XXXXXXX 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -XXX,XX +XXX,XX @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
         goto failed_shutdown;
     }
 
+    /* rbd_open is always r/w */
     r = rbd_open(s->io_ctx, s->name, &s->image, s->snap);
     if (r < 0) {
         error_setg_errno(errp, -r, "error reading header from %s", s->name);
         goto failed_open;
     }
 
-    bdrv_set_read_only(bs, (s->snap != NULL));
+    /* If we are using an rbd snapshot, we must be r/o, otherwise
+     * leave as-is */
+    if (s->snap != NULL) {
+        r = bdrv_set_read_only(bs, true, &local_err);
+        if (r < 0) {
+            error_propagate(errp, local_err);
+            goto failed_open;
+        }
+    }
 
     qemu_opts_del(opts);
     return 0;
diff --git a/block/vvfat.c b/block/vvfat.c
index XXXXXXX..XXXXXXX 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -XXX,XX +XXX,XX @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
 
     s->current_cluster=0xffffffff;
 
-    /* read only is the default for safety */
-    bdrv_set_read_only(bs, true);
     s->qcow = NULL;
     s->qcow_filename = NULL;
     s->fat2 = NULL;
@@ -XXX,XX +XXX,XX @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
     s->sector_count = cyls * heads * secs - (s->first_sectors_number - 1);
 
     if (qemu_opt_get_bool(opts, "rw", false)) {
-        ret = enable_write_target(bs, errp);
+        if (!bdrv_is_read_only(bs)) {
+            ret = enable_write_target(bs, errp);
+            if (ret < 0) {
+                goto fail;
+            }
+        } else {
+            ret = -EPERM;
+            error_setg(errp,
+                       "Unable to set VVFAT to 'rw' when drive is read-only");
+            goto fail;
+        }
+    } else  {
+        /* read only is the default for safety */
+        ret = bdrv_set_read_only(bs, true, &local_err);
         if (ret < 0) {
+            error_propagate(errp, local_err);
             goto fail;
         }
-        bdrv_set_read_only(bs, false);
     }
 
     bs->total_sectors = cyls * heads * secs;
diff --git a/include/block/block.h b/include/block/block.h
index XXXXXXX..XXXXXXX 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -XXX,XX +XXX,XX @@ int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
                             int64_t sector_num, int nb_sectors, int *pnum);
 
 bool bdrv_is_read_only(BlockDriverState *bs);
-void bdrv_set_read_only(BlockDriverState *bs, bool read_only);
+int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
 bool bdrv_is_sg(BlockDriverState *bs);
 bool bdrv_is_inserted(BlockDriverState *bs);
 int bdrv_media_changed(BlockDriverState *bs);
-- 
2.9.3

The BDRV_O_ALLOW_RDWR flag allows / prohibits the changing of
the BDS 'read_only' state, but there are a few places where it
is ignored.  In the bdrv_set_read_only() helper, make sure to
honor the flag.

Signed-off-by: Jeff Cody <jcody@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Message-id: be2e5fb2d285cbece2b6d06bed54a6f56520d251.1491597120.git.jcody@redhat.com
---
 block.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/block.c b/block.c
index XXXXXXX..XXXXXXX 100644
--- a/block.c
+++ b/block.c
@@ -XXX,XX +XXX,XX @@ int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
         return -EINVAL;
     }
 
+    /* Do not clear read_only if it is prohibited */
+    if (!read_only && !(bs->open_flags & BDRV_O_ALLOW_RDWR)) {
+        error_setg(errp, "Node '%s' is read only",
+                   bdrv_get_device_or_node_name(bs));
+        return -EPERM;
+    }
+
     bs->read_only = read_only;
     return 0;
 }
-- 
2.9.3

Move bdrv_is_read_only() up with its friends.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Signed-off-by: Jeff Cody <jcody@redhat.com>
Message-id: 73b2399459760c32506f9407efb9dddb3a2789de.1491597120.git.jcody@redhat.com
---
 block.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/block.c b/block.c
index XXXXXXX..XXXXXXX 100644
--- a/block.c
+++ b/block.c
@@ -XXX,XX +XXX,XX @@ void path_combine(char *dest, int dest_size,
     }
 }
 
+bool bdrv_is_read_only(BlockDriverState *bs)
+{
+    return bs->read_only;
+}
+
 int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
 {
     /* Do not set read_only if copy_on_read is enabled */
@@ -XXX,XX +XXX,XX @@ void bdrv_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr)
     *nb_sectors_ptr = nb_sectors < 0 ? 0 : nb_sectors;
 }
 
-bool bdrv_is_read_only(BlockDriverState *bs)
-{
-    return bs->read_only;
-}
-
 bool bdrv_is_sg(BlockDriverState *bs)
 {
     return bs->sg;
-- 
2.9.3

Introduce check function for setting read_only flags.  Will return < 0 on
error, with appropriate Error value set.  Does not alter any flags.

Signed-off-by: Jeff Cody <jcody@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Message-id: e2bba34ac3bc76a0c42adc390413f358ae0566e8.1491597120.git.jcody@redhat.com
---
 block.c               | 14 +++++++++++++-
 include/block/block.h |  1 +
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index XXXXXXX..XXXXXXX 100644
--- a/block.c
+++ b/block.c
@@ -XXX,XX +XXX,XX @@ bool bdrv_is_read_only(BlockDriverState *bs)
     return bs->read_only;
 }
 
-int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
+int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
 {
     /* Do not set read_only if copy_on_read is enabled */
     if (bs->copy_on_read && read_only) {
@@ -XXX,XX +XXX,XX @@ int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
         return -EPERM;
     }
 
+    return 0;
+}
+
+int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
+{
+    int ret = 0;
+
+    ret = bdrv_can_set_read_only(bs, read_only, errp);
+    if (ret < 0) {
+        return ret;
+    }
+
     bs->read_only = read_only;
     return 0;
 }
diff --git a/include/block/block.h b/include/block/block.h
index XXXXXXX..XXXXXXX 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -XXX,XX +XXX,XX @@ int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
                             int64_t sector_num, int nb_sectors, int *pnum);
 
 bool bdrv_is_read_only(BlockDriverState *bs);
+int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
 int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
 bool bdrv_is_sg(BlockDriverState *bs);
 bool bdrv_is_inserted(BlockDriverState *bs);
-- 
2.9.3

Signed-off-by: Jeff Cody <jcody@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Message-id: 00aed7ffdd7be4b9ed9ce1007d50028a72b34ebe.1491597120.git.jcody@redhat.com
---
 block.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index XXXXXXX..XXXXXXX 100644
--- a/block.c
+++ b/block.c
@@ -XXX,XX +XXX,XX @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
     BlockDriver *drv;
     QemuOpts *opts;
     const char *value;
+    bool read_only;
 
     assert(reopen_state != NULL);
     assert(reopen_state->bs->drv != NULL);
@@ -XXX,XX +XXX,XX @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
         qdict_put(reopen_state->options, "driver", qstring_from_str(value));
     }
 
-    /* if we are to stay read-only, do not allow permission change
-     * to r/w */
-    if (!(reopen_state->bs->open_flags & BDRV_O_ALLOW_RDWR) &&
-        reopen_state->flags & BDRV_O_RDWR) {
-        error_setg(errp, "Node '%s' is read only",
-                   bdrv_get_device_or_node_name(reopen_state->bs));
+    /* If we are to stay read-only, do not allow permission change
+     * to r/w. Attempting to set to r/w may fail if either BDRV_O_ALLOW_RDWR is
+     * not set, or if the BDS still has copy_on_read enabled */
+    read_only = !(reopen_state->flags & BDRV_O_RDWR);
+    ret = bdrv_can_set_read_only(reopen_state->bs, read_only, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
         goto error;
     }
 
-- 
2.9.3

Update 'clientname' to be 'user', which tracks better with both
the QAPI and rados variable naming.

Update 'name' to be 'image_name', as it indicates the rbd image.
Naming it 'image' would have been ideal, but we are using that for
the rados_image_t value returned by rbd_open().

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Jeff Cody <jcody@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Message-id: b7ec1fb2e1cf36f9b6911631447a5b0422590b7d.1491597120.git.jcody@redhat.com
---
 block/rbd.c | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index XXXXXXX..XXXXXXX 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -XXX,XX +XXX,XX @@ typedef struct BDRVRBDState {
     rados_t cluster;
     rados_ioctx_t io_ctx;
     rbd_image_t image;
-    char *name;
+    char *image_name;
     char *snap;
 } BDRVRBDState;
 
@@ -XXX,XX +XXX,XX @@ static int qemu_rbd_create(const char *filename, QemuOpts *opts, Error **errp)
     int64_t bytes = 0;
     int64_t objsize;
     int obj_order = 0;
-    const char *pool, *name, *conf, *clientname, *keypairs;
+    const char *pool, *image_name, *conf, *user, *keypairs;
     const char *secretid;
     rados_t cluster;
     rados_ioctx_t io_ctx;
@@ -XXX,XX +XXX,XX @@ static int qemu_rbd_create(const char *filename, QemuOpts *opts, Error **errp)
      */
     pool       = qdict_get_try_str(options, "pool");
     conf       = qdict_get_try_str(options, "conf");
-    clientname = qdict_get_try_str(options, "user");
-    name       = qdict_get_try_str(options, "image");
+    user       = qdict_get_try_str(options, "user");
+    image_name = qdict_get_try_str(options, "image");
     keypairs   = qdict_get_try_str(options, "=keyvalue-pairs");
 
-    ret = rados_create(&cluster, clientname);
+    ret = rados_create(&cluster, user);
     if (ret < 0) {
         error_setg_errno(errp, -ret, "error initializing");
         goto exit;
@@ -XXX,XX +XXX,XX @@ static int qemu_rbd_create(const char *filename, QemuOpts *opts, Error **errp)
         goto shutdown;
     }
 
-    ret = rbd_create(io_ctx, name, bytes, &obj_order);
+    ret = rbd_create(io_ctx, image_name, bytes, &obj_order);
     if (ret < 0) {
         error_setg_errno(errp, -ret, "error rbd create");
     }
@@ -XXX,XX +XXX,XX @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
                          Error **errp)
 {
     BDRVRBDState *s = bs->opaque;
-    const char *pool, *snap, *conf, *clientname, *name, *keypairs;
+    const char *pool, *snap, *conf, *user, *image_name, *keypairs;
     const char *secretid;
     QemuOpts *opts;
     Error *local_err = NULL;
@@ -XXX,XX +XXX,XX @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
     pool           = qemu_opt_get(opts, "pool");
     conf           = qemu_opt_get(opts, "conf");
     snap           = qemu_opt_get(opts, "snapshot");
-    clientname     = qemu_opt_get(opts, "user");
-    name           = qemu_opt_get(opts, "image");
+    user           = qemu_opt_get(opts, "user");
+    image_name     = qemu_opt_get(opts, "image");
     keypairs       = qemu_opt_get(opts, "=keyvalue-pairs");
 
-    if (!pool || !name) {
+    if (!pool || !image_name) {
         error_setg(errp, "Parameters 'pool' and 'image' are required");
         r = -EINVAL;
         goto failed_opts;
     }
 
-    r = rados_create(&s->cluster, clientname);
+    r = rados_create(&s->cluster, user);
     if (r < 0) {
         error_setg_errno(errp, -r, "error initializing");
         goto failed_opts;
     }
 
     s->snap = g_strdup(snap);
-    s->name = g_strdup(name);
+    s->image_name = g_strdup(image_name);
 
     /* try default location when conf=NULL, but ignore failure */
     r = rados_conf_read_file(s->cluster, conf);
@@ -XXX,XX +XXX,XX @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     /* rbd_open is always r/w */
-    r = rbd_open(s->io_ctx, s->name, &s->image, s->snap);
+    r = rbd_open(s->io_ctx, s->image_name, &s->image, s->snap);
     if (r < 0) {
-        error_setg_errno(errp, -r, "error reading header from %s", s->name);
+        error_setg_errno(errp, -r, "error reading header from %s",
+                         s->image_name);
         goto failed_open;
     }
 
@@ -XXX,XX +XXX,XX @@ failed_open:
 failed_shutdown:
     rados_shutdown(s->cluster);
     g_free(s->snap);
-    g_free(s->name);
+    g_free(s->image_name);
 failed_opts:
     qemu_opts_del(opts);
     g_free(mon_host);
@@ -XXX,XX +XXX,XX @@ static void qemu_rbd_close(BlockDriverState *bs)
     rbd_close(s->image);
     rados_ioctx_destroy(s->io_ctx);
     g_free(s->snap);
-    g_free(s->name);
+    g_free(s->image_name);
     rados_shutdown(s->cluster);
 }
 
-- 
2.9.3

This adds support for reopen in rbd, for changing between r/w and r/o.

Note, that this is only a flag change, but we will block a change from
r/o to r/w if we are using an RBD internal snapshot.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Jeff Cody <jcody@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Message-id: d4e87539167ec6527d44c97b164eabcccf96e4f3.1491597120.git.jcody@redhat.com
---
 block/rbd.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/block/rbd.c b/block/rbd.c
index XXXXXXX..XXXXXXX 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -XXX,XX +XXX,XX @@ failed_opts:
     return r;
 }
 
+
+/* Since RBD is currently always opened R/W via the API,
+ * we just need to check if we are using a snapshot or not, in
+ * order to determine if we will allow it to be R/W */
+static int qemu_rbd_reopen_prepare(BDRVReopenState *state,
+                                   BlockReopenQueue *queue, Error **errp)
+{
+    BDRVRBDState *s = state->bs->opaque;
+    int ret = 0;
+
+    if (s->snap && state->flags & BDRV_O_RDWR) {
+        error_setg(errp,
+                   "Cannot change node '%s' to r/w when using RBD snapshot",
+                   bdrv_get_device_or_node_name(state->bs));
+        ret = -EINVAL;
+    }
+
+    return ret;
+}
+
 static void qemu_rbd_close(BlockDriverState *bs)
 {
     BDRVRBDState *s = bs->opaque;
@@ -XXX,XX +XXX,XX @@ static BlockDriver bdrv_rbd = {
     .bdrv_parse_filename    = qemu_rbd_parse_filename,
     .bdrv_file_open         = qemu_rbd_open,
     .bdrv_close             = qemu_rbd_close,
+    .bdrv_reopen_prepare    = qemu_rbd_reopen_prepare,
     .bdrv_create            = qemu_rbd_create,
     .bdrv_has_zero_init     = bdrv_has_zero_init_1,
     .bdrv_get_info          = qemu_rbd_getinfo,
-- 
2.9.3

For the tests that use the common.qemu functions for running a QEMU
process, _cleanup_qemu must be called in the exit function.

If it is not, if the qemu process aborts, then not all of the droppings
are cleaned up (e.g. pidfile, fifos).

This updates those tests that did not have a cleanup in qemu-iotests.

(I swapped spaces for tabs in test 102 as well)

Reported-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Jeff Cody <jcody@redhat.com>
Message-id: d59c2f6ad6c1da8b9b3c7f357c94a7122ccfc55a.1492544096.git.jcody@redhat.com
---
 tests/qemu-iotests/028 |  1 +
 tests/qemu-iotests/094 | 11 ++++++++---
 tests/qemu-iotests/102 |  5 +++--
 tests/qemu-iotests/109 |  1 +
 tests/qemu-iotests/117 |  1 +
 tests/qemu-iotests/130 |  1 +
 tests/qemu-iotests/140 |  1 +
 tests/qemu-iotests/141 |  1 +
 tests/qemu-iotests/143 |  1 +
 tests/qemu-iotests/156 |  1 +
 10 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/tests/qemu-iotests/028 b/tests/qemu-iotests/028
index XXXXXXX..XXXXXXX 100755
--- a/tests/qemu-iotests/028
+++ b/tests/qemu-iotests/028
@@ -XXX,XX +XXX,XX @@ status=1	# failure is the default!
 
 _cleanup()
 {
+    _cleanup_qemu
     rm -f "${TEST_IMG}.copy"
     _cleanup_test_img
 }
diff --git a/tests/qemu-iotests/094 b/tests/qemu-iotests/094
index XXXXXXX..XXXXXXX 100755
--- a/tests/qemu-iotests/094
+++ b/tests/qemu-iotests/094
@@ -XXX,XX +XXX,XX @@ echo "QA output created by $seq"
 here="$PWD"
 status=1	# failure is the default!
 
-trap "exit \$status" 0 1 2 3 15
+_cleanup()
+{
+    _cleanup_qemu
+    _cleanup_test_img
+    rm -f "$TEST_DIR/source.$IMGFMT"
+}
+
+trap "_cleanup; exit \$status" 0 1 2 3 15
 
 # get standard environment, filters and checks
 . ./common.rc
@@ -XXX,XX +XXX,XX @@ _send_qemu_cmd $QEMU_HANDLE \
 
 wait=1 _cleanup_qemu
 
-_cleanup_test_img
-rm -f "$TEST_DIR/source.$IMGFMT"
 
 # success, all done
 echo '*** done'
diff --git a/tests/qemu-iotests/102 b/tests/qemu-iotests/102
index XXXXXXX..XXXXXXX 100755
--- a/tests/qemu-iotests/102
+++ b/tests/qemu-iotests/102
@@ -XXX,XX +XXX,XX @@ seq=$(basename $0)
 echo "QA output created by $seq"
 
 here=$PWD
-status=1	# failure is the default!
+status=1    # failure is the default!
 
 _cleanup()
 {
-	_cleanup_test_img
+    _cleanup_qemu
+    _cleanup_test_img
 }
 trap "_cleanup; exit \$status" 0 1 2 3 15
 
diff --git a/tests/qemu-iotests/109 b/tests/qemu-iotests/109
index XXXXXXX..XXXXXXX 100755
--- a/tests/qemu-iotests/109
+++ b/tests/qemu-iotests/109
@@ -XXX,XX +XXX,XX @@ status=1	# failure is the default!
 
 _cleanup()
 {
+    _cleanup_qemu
     rm -f $TEST_IMG.src
 	_cleanup_test_img
 }
diff --git a/tests/qemu-iotests/117 b/tests/qemu-iotests/117
index XXXXXXX..XXXXXXX 100755
--- a/tests/qemu-iotests/117
+++ b/tests/qemu-iotests/117
@@ -XXX,XX +XXX,XX @@ status=1	# failure is the default!
 
 _cleanup()
 {
+    _cleanup_qemu
 	_cleanup_test_img
 }
 trap "_cleanup; exit \$status" 0 1 2 3 15
diff --git a/tests/qemu-iotests/130 b/tests/qemu-iotests/130
index XXXXXXX..XXXXXXX 100755
--- a/tests/qemu-iotests/130
+++ b/tests/qemu-iotests/130
@@ -XXX,XX +XXX,XX @@ status=1	# failure is the default!
 
 _cleanup()
 {
+    _cleanup_qemu
     _cleanup_test_img
 }
 trap "_cleanup; exit \$status" 0 1 2 3 15
diff --git a/tests/qemu-iotests/140 b/tests/qemu-iotests/140
index XXXXXXX..XXXXXXX 100755
--- a/tests/qemu-iotests/140
+++ b/tests/qemu-iotests/140
@@ -XXX,XX +XXX,XX @@ status=1	# failure is the default!
 
 _cleanup()
 {
+    _cleanup_qemu
     _cleanup_test_img
     rm -f "$TEST_DIR/nbd"
 }
diff --git a/tests/qemu-iotests/141 b/tests/qemu-iotests/141
index XXXXXXX..XXXXXXX 100755
--- a/tests/qemu-iotests/141
+++ b/tests/qemu-iotests/141
@@ -XXX,XX +XXX,XX @@ status=1	# failure is the default!
 
 _cleanup()
 {
+    _cleanup_qemu
     _cleanup_test_img
     rm -f "$TEST_DIR/{b,m,o}.$IMGFMT"
 }
diff --git a/tests/qemu-iotests/143 b/tests/qemu-iotests/143
index XXXXXXX..XXXXXXX 100755
--- a/tests/qemu-iotests/143
+++ b/tests/qemu-iotests/143
@@ -XXX,XX +XXX,XX @@ status=1	# failure is the default!
 
 _cleanup()
 {
+    _cleanup_qemu
     rm -f "$TEST_DIR/nbd"
 }
 trap "_cleanup; exit \$status" 0 1 2 3 15
diff --git a/tests/qemu-iotests/156 b/tests/qemu-iotests/156
index XXXXXXX..XXXXXXX 100755
--- a/tests/qemu-iotests/156
+++ b/tests/qemu-iotests/156
@@ -XXX,XX +XXX,XX @@ status=1	# failure is the default!
 
 _cleanup()
 {
+    _cleanup_qemu
     rm -f "$TEST_IMG{,.target}{,.backing,.overlay}"
 }
 trap "_cleanup; exit \$status" 0 1 2 3 15
-- 
2.9.3

The following changes since commit 42f6c9179be4401974dd3a75ee72defd16b5092d:

Merge tag 'pull-ppc-20211112' of https://github.com/legoater/qemu into staging (2021-11-12 12:28:25 +0100)

are available in the Git repository at:

https://gitlab.com/hreitz/qemu.git tags/pull-block-2021-11-16

for you to fetch changes up to 5dbd0ce115c7720268e653fe928bb6a0c1314a80:

file-posix: Fix alignment after reopen changing O_DIRECT (2021-11-16 11:30:29 +0100)

----------------------------------------------------------------
Block patches for 6.2.0-rc1:
- Fixes to image streaming job and block layer reconfiguration to make
  iotest 030 pass again
- docs: Deprecate incorrectly typed device_add arguments
- file-posix: Fix alignment after reopen changing O_DIRECT

----------------------------------------------------------------
v2:
- Fixed iotest 142 (modified by "file-posix: Fix alignment after reopen
  changing O_DIRECT") -- at least I hope so: for me, it now passes on a
  4k block device, and the gitlab pipeline passed, too

- Note that because I had to modify Kevin's pull request, I did not want
  to merge it partially (with a merge commit), but instead decided to
  apply all patches from the pull request mails (including their message
  IDs)

----------------------------------------------------------------
Hanna Reitz (10):
  stream: Traverse graph after modification
  block: Manipulate children list in .attach/.detach
  block: Unite remove_empty_child and child_free
  block: Drop detached child from ignore list
  block: Pass BdrvChild ** to replace_child_noperm
  block: Restructure remove_file_or_backing_child()
  transactions: Invoke clean() after everything else
  block: Let replace_child_tran keep indirect pointer
  block: Let replace_child_noperm free children
  iotests/030: Unthrottle parallel jobs in reverse

Kevin Wolf (2):
  docs: Deprecate incorrectly typed device_add arguments
  file-posix: Fix alignment after reopen changing O_DIRECT

Stefan Hajnoczi (1):
  softmmu/qdev-monitor: fix use-after-free in qdev_set_id()

-- 
2.33.1

bdrv_cor_filter_drop() modifies the block graph.  That means that other
parties can also modify the block graph before it returns.  Therefore,
we cannot assume that the result of a graph traversal we did before
remains valid afterwards.

We should thus fetch `base` and `unfiltered_base` afterwards instead of
before.

Signed-off-by: Hanna Reitz <hreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Message-Id: <20211111120829.81329-2-hreitz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-Id: <20211115145409.176785-2-kwolf@redhat.com>
Signed-off-by: Hanna Reitz <hreitz@redhat.com>
---
 block/stream.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/block/stream.c b/block/stream.c
index XXXXXXX..XXXXXXX 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -XXX,XX +XXX,XX @@ static int stream_prepare(Job *job)
 {
     StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
     BlockDriverState *unfiltered_bs = bdrv_skip_filters(s->target_bs);
-    BlockDriverState *base = bdrv_filter_or_cow_bs(s->above_base);
-    BlockDriverState *unfiltered_base = bdrv_skip_filters(base);
+    BlockDriverState *base;
+    BlockDriverState *unfiltered_base;
     Error *local_err = NULL;
     int ret = 0;
 
@@ -XXX,XX +XXX,XX @@ static int stream_prepare(Job *job)
     bdrv_cor_filter_drop(s->cor_filter_bs);
     s->cor_filter_bs = NULL;
 
+    base = bdrv_filter_or_cow_bs(s->above_base);
+    unfiltered_base = bdrv_skip_filters(base);
+
     if (bdrv_cow_child(unfiltered_bs)) {
         const char *base_id = NULL, *base_fmt = NULL;
         if (unfiltered_base) {
-- 
2.33.1

The children list is specific to BDS parents.  We should not modify it
in the general children modification code, but let BDS parents deal with
it in their .attach() and .detach() methods.

This also has the advantage that a BdrvChild is removed from the
children list before its .bs pointer can become NULL.  BDS parents
generally assume that their children's .bs pointer is never NULL, so
this is actually a bug fix.

Signed-off-by: Hanna Reitz <hreitz@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Message-Id: <20211111120829.81329-3-hreitz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-Id: <20211115145409.176785-3-kwolf@redhat.com>
Signed-off-by: Hanna Reitz <hreitz@redhat.com>
---
 block.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/block.c b/block.c
index XXXXXXX..XXXXXXX 100644
--- a/block.c
+++ b/block.c
@@ -XXX,XX +XXX,XX @@ static void bdrv_child_cb_attach(BdrvChild *child)
 {
     BlockDriverState *bs = child->opaque;
 
+    QLIST_INSERT_HEAD(&bs->children, child, next);
+
     if (child->role & BDRV_CHILD_COW) {
         bdrv_backing_attach(child);
     }
@@ -XXX,XX +XXX,XX @@ static void bdrv_child_cb_detach(BdrvChild *child)
     }
 
     bdrv_unapply_subtree_drain(child, bs);
+
+    QLIST_REMOVE(child, next);
 }
 
 static int bdrv_child_cb_update_filename(BdrvChild *c, BlockDriverState *base,
@@ -XXX,XX +XXX,XX @@ static void bdrv_child_free(void *opaque)
 static void bdrv_remove_empty_child(BdrvChild *child)
 {
     assert(!child->bs);
-    QLIST_SAFE_REMOVE(child, next);
+    assert(!child->next.le_prev); /* not in children list */
     bdrv_child_free(child);
 }
 
@@ -XXX,XX +XXX,XX @@ static int bdrv_attach_child_noperm(BlockDriverState *parent_bs,
         return ret;
     }
 
-    QLIST_INSERT_HEAD(&parent_bs->children, *child, next);
-    /*
-     * child is removed in bdrv_attach_child_common_abort(), so don't care to
-     * abort this change separately.
-     */
-
     return 0;
 }
 
@@ -XXX,XX +XXX,XX @@ static void bdrv_remove_filter_or_cow_child_abort(void *opaque)
     BdrvRemoveFilterOrCowChild *s = opaque;
     BlockDriverState *parent_bs = s->child->opaque;
 
-    QLIST_INSERT_HEAD(&parent_bs->children, s->child, next);
     if (s->is_backing) {
         parent_bs->backing = s->child;
     } else {
@@ -XXX,XX +XXX,XX @@ static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,
     };
     tran_add(tran, &bdrv_remove_filter_or_cow_child_drv, s);
 
-    QLIST_SAFE_REMOVE(child, next);
     if (s->is_backing) {
         bs->backing = NULL;
     } else {
-- 
2.33.1

Now that bdrv_remove_empty_child() no longer removes the child from the
parent's children list but only checks that it is not in such a list, it
is only a wrapper around bdrv_child_free() that checks that the child is
empty and unused.  That should apply to all children that we free, so
put those checks into bdrv_child_free() and drop
bdrv_remove_empty_child().

Signed-off-by: Hanna Reitz <hreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Message-Id: <20211111120829.81329-4-hreitz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-Id: <20211115145409.176785-4-kwolf@redhat.com>
Signed-off-by: Hanna Reitz <hreitz@redhat.com>
---
 block.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/block.c b/block.c
index XXXXXXX..XXXXXXX 100644
--- a/block.c
+++ b/block.c
@@ -XXX,XX +XXX,XX @@ static void bdrv_replace_child_noperm(BdrvChild *child,
     }
 }
 
-static void bdrv_child_free(void *opaque)
-{
-    BdrvChild *c = opaque;
-
-    g_free(c->name);
-    g_free(c);
-}
-
-static void bdrv_remove_empty_child(BdrvChild *child)
+/**
+ * Free the given @child.
+ *
+ * The child must be empty (i.e. `child->bs == NULL`) and it must be
+ * unused (i.e. not in a children list).
+ */
+static void bdrv_child_free(BdrvChild *child)
 {
     assert(!child->bs);
     assert(!child->next.le_prev); /* not in children list */
-    bdrv_child_free(child);
+
+    g_free(child->name);
+    g_free(child);
 }
 
 typedef struct BdrvAttachChildCommonState {
@@ -XXX,XX +XXX,XX @@ static void bdrv_attach_child_common_abort(void *opaque)
     }
 
     bdrv_unref(bs);
-    bdrv_remove_empty_child(child);
+    bdrv_child_free(child);
     *s->child = NULL;
 }
 
@@ -XXX,XX +XXX,XX @@ static int bdrv_attach_child_common(BlockDriverState *child_bs,
 
         if (ret < 0) {
             error_propagate(errp, local_err);
-            bdrv_remove_empty_child(new_child);
+            bdrv_child_free(new_child);
             return ret;
         }
     }
@@ -XXX,XX +XXX,XX @@ static void bdrv_detach_child(BdrvChild *child)
     BlockDriverState *old_bs = child->bs;
 
     bdrv_replace_child_noperm(child, NULL);
-    bdrv_remove_empty_child(child);
+    bdrv_child_free(child);
 
     if (old_bs) {
         /*
-- 
2.33.1

bdrv_attach_child_common_abort() restores the parent's AioContext.  To
do so, the child (which was supposed to be attached, but is now detached
again by this abort handler) is added to the ignore list for the
AioContext changing functions.

However, since we modify a BDS's children list in the BdrvChildClass's
.attach and .detach handlers, the child is already effectively detached
from the parent by this point.  We do not need to put it into the ignore
list.

Use this opportunity to clean up the empty line structure: Keep setting
the ignore list, invoking the AioContext function, and freeing the
ignore list in blocks separated by empty lines.

Signed-off-by: Hanna Reitz <hreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Message-Id: <20211111120829.81329-5-hreitz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-Id: <20211115145409.176785-5-kwolf@redhat.com>
Signed-off-by: Hanna Reitz <hreitz@redhat.com>
---
 block.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/block.c b/block.c
index XXXXXXX..XXXXXXX 100644
--- a/block.c
+++ b/block.c
@@ -XXX,XX +XXX,XX @@ static void bdrv_attach_child_common_abort(void *opaque)
     }
 
     if (bdrv_child_get_parent_aio_context(child) != s->old_parent_ctx) {
-        GSList *ignore = g_slist_prepend(NULL, child);
+        GSList *ignore;
 
+        /* No need to ignore `child`, because it has been detached already */
+        ignore = NULL;
         child->klass->can_set_aio_ctx(child, s->old_parent_ctx, &ignore,
                                       &error_abort);
         g_slist_free(ignore);
-        ignore = g_slist_prepend(NULL, child);
-        child->klass->set_aio_ctx(child, s->old_parent_ctx, &ignore);
 
+        ignore = NULL;
+        child->klass->set_aio_ctx(child, s->old_parent_ctx, &ignore);
         g_slist_free(ignore);
     }
 
-- 
2.33.1

bdrv_replace_child_noperm() modifies BdrvChild.bs, and can potentially
set it to NULL.  That is dangerous, because BDS parents generally assume
that their children's .bs pointer is never NULL.  We therefore want to
let bdrv_replace_child_noperm() set the corresponding BdrvChild pointer
to NULL, too.

This patch lays the foundation for it by passing a BdrvChild ** pointer
to bdrv_replace_child_noperm() so that it can later use it to NULL the
BdrvChild pointer immediately after setting BdrvChild.bs to NULL.

(We will still need to undertake some intermediate steps, though.)

Signed-off-by: Hanna Reitz <hreitz@redhat.com>
Message-Id: <20211111120829.81329-6-hreitz@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-Id: <20211115145409.176785-6-kwolf@redhat.com>
Signed-off-by: Hanna Reitz <hreitz@redhat.com>
---
 block.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

As of a future patch, bdrv_replace_child_tran() will take a BdrvChild **
pointer.  Prepare for that by getting such a pointer and using it where
applicable, and (dereferenced) as a parameter for
bdrv_replace_child_tran().

Signed-off-by: Hanna Reitz <hreitz@redhat.com>
Message-Id: <20211111120829.81329-7-hreitz@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-Id: <20211115145409.176785-7-kwolf@redhat.com>
Signed-off-by: Hanna Reitz <hreitz@redhat.com>
---
 block.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/block.c b/block.c
index XXXXXXX..XXXXXXX 100644
--- a/block.c
+++ b/block.c
@@ -XXX,XX +XXX,XX @@ static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,
                                               BdrvChild *child,
                                               Transaction *tran)
 {
+    BdrvChild **childp;
     BdrvRemoveFilterOrCowChild *s;
 
-    assert(child == bs->backing || child == bs->file);
-
     if (!child) {
         return;
     }
 
+    if (child == bs->backing) {
+        childp = &bs->backing;
+    } else if (child == bs->file) {
+        childp = &bs->file;
+    } else {
+        g_assert_not_reached();
+    }
+
     if (child->bs) {
-        bdrv_replace_child_tran(child, NULL, tran);
+        bdrv_replace_child_tran(*childp, NULL, tran);
     }
 
     s = g_new(BdrvRemoveFilterOrCowChild, 1);
     *s = (BdrvRemoveFilterOrCowChild) {
         .child = child,
-        .is_backing = (child == bs->backing),
+        .is_backing = (childp == &bs->backing),
     };
     tran_add(tran, &bdrv_remove_filter_or_cow_child_drv, s);
 
-    if (s->is_backing) {
-        bs->backing = NULL;
-    } else {
-        bs->file = NULL;
-    }
+    *childp = NULL;
 }
 
 /*
-- 
2.33.1

Invoke the transaction drivers' .clean() methods only after all
.commit() or .abort() handlers are done.

This makes it easier to have nested transactions where the top-level
transactions pass objects to lower transactions that the latter can
still use throughout their commit/abort phases, while the top-level
transaction keeps a reference that is released in its .clean() method.

(Before this commit, that is also possible, but the top-level
transaction would need to take care to invoke tran_add() before the
lower-level transaction does.  This commit makes the ordering
irrelevant, which is just a bit nicer.)

Signed-off-by: Hanna Reitz <hreitz@redhat.com>
Message-Id: <20211111120829.81329-8-hreitz@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-Id: <20211115145409.176785-8-kwolf@redhat.com>
Signed-off-by: Hanna Reitz <hreitz@redhat.com>
---
 include/qemu/transactions.h | 3 +++
 util/transactions.c         | 8 ++++++--
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/include/qemu/transactions.h b/include/qemu/transactions.h
index XXXXXXX..XXXXXXX 100644
--- a/include/qemu/transactions.h
+++ b/include/qemu/transactions.h
@@ -XXX,XX +XXX,XX @@
  * tran_create(), call your "prepare" functions on it, and finally call
  * tran_abort() or tran_commit() to finalize the transaction by corresponding
  * finalization actions in reverse order.
+ *
+ * The clean() functions registered by the drivers in a transaction are called
+ * last, after all abort() or commit() functions have been called.
  */
 
 #ifndef QEMU_TRANSACTIONS_H
diff --git a/util/transactions.c b/util/transactions.c
index XXXXXXX..XXXXXXX 100644
--- a/util/transactions.c
+++ b/util/transactions.c
@@ -XXX,XX +XXX,XX @@ void tran_abort(Transaction *tran)
 {
     TransactionAction *act, *next;
 
-    QSLIST_FOREACH_SAFE(act, &tran->actions, entry, next) {
+    QSLIST_FOREACH(act, &tran->actions, entry) {
         if (act->drv->abort) {
             act->drv->abort(act->opaque);
         }
+    }
 
+    QSLIST_FOREACH_SAFE(act, &tran->actions, entry, next) {
         if (act->drv->clean) {
             act->drv->clean(act->opaque);
         }
@@ -XXX,XX +XXX,XX @@ void tran_commit(Transaction *tran)
 {
     TransactionAction *act, *next;
 
-    QSLIST_FOREACH_SAFE(act, &tran->actions, entry, next) {
+    QSLIST_FOREACH(act, &tran->actions, entry) {
         if (act->drv->commit) {
             act->drv->commit(act->opaque);
         }
+    }
 
+    QSLIST_FOREACH_SAFE(act, &tran->actions, entry, next) {
         if (act->drv->clean) {
             act->drv->clean(act->opaque);
         }
-- 
2.33.1

As of a future commit, bdrv_replace_child_noperm() will clear the
indirect BdrvChild pointer passed to it if the new child BDS is NULL.
bdrv_replace_child_tran() will want to let it do that, but revert this
change in its abort handler.  For that, we need to have it receive a
BdrvChild ** pointer, too, and keep it stored in the
BdrvReplaceChildState object that we attach to the transaction.

Note that we do not need to store it in the BdrvReplaceChildState when
new_bs is not NULL, because then there is nothing to revert.  This is
important so that bdrv_replace_node_noperm() can pass a pointer to a
loop-local variable to bdrv_replace_child_tran() without worrying that
this pointer will outlive one loop iteration.

(Of course, for that to work, bdrv_replace_node_noperm() and in turn
bdrv_replace_node() and its relatives may not be called with a NULL @to
node.  Luckily, they already are not, but now we should assert this.)

bdrv_remove_file_or_backing_child() on the other hand needs to ensure
that the indirect pointer it passes will stay valid for the duration of
the transaction.  Ensure this by keeping a strong reference to the BDS
whose &bs->backing or &bs->file it passes to bdrv_replace_child_tran(),
and giving up that reference only in the transaction .clean() handler.

Signed-off-by: Hanna Reitz <hreitz@redhat.com>
Message-Id: <20211111120829.81329-9-hreitz@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-Id: <20211115145409.176785-9-kwolf@redhat.com>
Signed-off-by: Hanna Reitz <hreitz@redhat.com>
---
 block.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 73 insertions(+), 10 deletions(-)

diff --git a/block.c b/block.c
index XXXXXXX..XXXXXXX 100644
--- a/block.c
+++ b/block.c
@@ -XXX,XX +XXX,XX @@ static int bdrv_drv_set_perm(BlockDriverState *bs, uint64_t perm,
 
 typedef struct BdrvReplaceChildState {
     BdrvChild *child;
+    BdrvChild **childp;
     BlockDriverState *old_bs;
 } BdrvReplaceChildState;
 
@@ -XXX,XX +XXX,XX @@ static void bdrv_replace_child_abort(void *opaque)
     BdrvReplaceChildState *s = opaque;
     BlockDriverState *new_bs = s->child->bs;
 
-    /* old_bs reference is transparently moved from @s to @s->child */
+    /*
+     * old_bs reference is transparently moved from @s to s->child.
+     *
+     * Pass &s->child here instead of s->childp, because:
+     * (1) s->old_bs must be non-NULL, so bdrv_replace_child_noperm() will not
+     *     modify the BdrvChild * pointer we indirectly pass to it, i.e. it
+     *     will not modify s->child.  From that perspective, it does not matter
+     *     whether we pass s->childp or &s->child.
+     *     (TODO: Right now, bdrv_replace_child_noperm() never modifies that
+     *     pointer anyway (though it will in the future), so at this point it
+     *     absolutely does not matter whether we pass s->childp or &s->child.)
+     * (2) If new_bs is not NULL, s->childp will be NULL.  We then cannot use
+     *     it here.
+     * (3) If new_bs is NULL, *s->childp will have been NULLed by
+     *     bdrv_replace_child_tran()'s bdrv_replace_child_noperm() call, and we
+     *     must not pass a NULL *s->childp here.
+     *     (TODO: In its current state, bdrv_replace_child_noperm() will not
+     *     have NULLed *s->childp, so this does not apply yet.  It will in the
+     *     future.)
+     *
+     * So whether new_bs was NULL or not, we cannot pass s->childp here; and in
+     * any case, there is no reason to pass it anyway.
+     */
     bdrv_replace_child_noperm(&s->child, s->old_bs);
     bdrv_unref(new_bs);
 }
@@ -XXX,XX +XXX,XX @@ static TransactionActionDrv bdrv_replace_child_drv = {
  * Note: real unref of old_bs is done only on commit.
  *
  * The function doesn't update permissions, caller is responsible for this.
+ *
+ * Note that if new_bs == NULL, @childp is stored in a state object attached
+ * to @tran, so that the old child can be reinstated in the abort handler.
+ * Therefore, if @new_bs can be NULL, @childp must stay valid until the
+ * transaction is committed or aborted.
+ *
+ * (TODO: The reinstating does not happen yet, but it will once
+ * bdrv_replace_child_noperm() NULLs *childp when new_bs is NULL.)
  */
-static void bdrv_replace_child_tran(BdrvChild *child, BlockDriverState *new_bs,
+static void bdrv_replace_child_tran(BdrvChild **childp,
+                                    BlockDriverState *new_bs,
                                     Transaction *tran)
 {
     BdrvReplaceChildState *s = g_new(BdrvReplaceChildState, 1);
     *s = (BdrvReplaceChildState) {
-        .child = child,
-        .old_bs = child->bs,
+        .child = *childp,
+        .childp = new_bs == NULL ? childp : NULL,
+        .old_bs = (*childp)->bs,
     };
     tran_add(tran, &bdrv_replace_child_drv, s);
 
     if (new_bs) {
         bdrv_ref(new_bs);
     }
-    bdrv_replace_child_noperm(&child, new_bs);
-    /* old_bs reference is transparently moved from @child to @s */
+    bdrv_replace_child_noperm(childp, new_bs);
+    /* old_bs reference is transparently moved from *childp to @s */
 }
 
 /*
@@ -XXX,XX +XXX,XX @@ static bool should_update_child(BdrvChild *c, BlockDriverState *to)
 
 typedef struct BdrvRemoveFilterOrCowChild {
     BdrvChild *child;
+    BlockDriverState *bs;
     bool is_backing;
 } BdrvRemoveFilterOrCowChild;
 
@@ -XXX,XX +XXX,XX @@ static void bdrv_remove_filter_or_cow_child_commit(void *opaque)
     bdrv_child_free(s->child);
 }
 
+static void bdrv_remove_filter_or_cow_child_clean(void *opaque)
+{
+    BdrvRemoveFilterOrCowChild *s = opaque;
+
+    /* Drop the bs reference after the transaction is done */
+    bdrv_unref(s->bs);
+    g_free(s);
+}
+
 static TransactionActionDrv bdrv_remove_filter_or_cow_child_drv = {
     .abort = bdrv_remove_filter_or_cow_child_abort,
     .commit = bdrv_remove_filter_or_cow_child_commit,
-    .clean = g_free,
+    .clean = bdrv_remove_filter_or_cow_child_clean,
 };
 
 /*
@@ -XXX,XX +XXX,XX @@ static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,
         return;
     }
 
+    /*
+     * Keep a reference to @bs so @childp will stay valid throughout the
+     * transaction (required by bdrv_replace_child_tran())
+     */
+    bdrv_ref(bs);
     if (child == bs->backing) {
         childp = &bs->backing;
     } else if (child == bs->file) {
@@ -XXX,XX +XXX,XX @@ static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,
     }
 
     if (child->bs) {
-        bdrv_replace_child_tran(*childp, NULL, tran);
+        bdrv_replace_child_tran(childp, NULL, tran);
     }
 
     s = g_new(BdrvRemoveFilterOrCowChild, 1);
     *s = (BdrvRemoveFilterOrCowChild) {
         .child = child,
+        .bs = bs,
         .is_backing = (childp == &bs->backing),
     };
     tran_add(tran, &bdrv_remove_filter_or_cow_child_drv, s);
@@ -XXX,XX +XXX,XX @@ static int bdrv_replace_node_noperm(BlockDriverState *from,
 {
     BdrvChild *c, *next;
 
+    assert(to != NULL);
+
     QLIST_FOREACH_SAFE(c, &from->parents, next_parent, next) {
         assert(c->bs == from);
         if (!should_update_child(c, to)) {
@@ -XXX,XX +XXX,XX @@ static int bdrv_replace_node_noperm(BlockDriverState *from,
                        c->name, from->node_name);
             return -EPERM;
         }
-        bdrv_replace_child_tran(c, to, tran);
+
+        /*
+         * Passing a pointer to the local variable @c is fine here, because
+         * @to is not NULL, and so &c will not be attached to the transaction.
+         */
+        bdrv_replace_child_tran(&c, to, tran);
     }
 
     return 0;
@@ -XXX,XX +XXX,XX @@ static int bdrv_replace_node_noperm(BlockDriverState *from,
  *
  * With @detach_subchain=true @to must be in a backing chain of @from. In this
  * case backing link of the cow-parent of @to is removed.
+ *
+ * @to must not be NULL.
  */
 static int bdrv_replace_node_common(BlockDriverState *from,
                                     BlockDriverState *to,
@@ -XXX,XX +XXX,XX @@ static int bdrv_replace_node_common(BlockDriverState *from,
     BlockDriverState *to_cow_parent = NULL;
     int ret;
 
+    assert(to != NULL);
+
     if (detach_subchain) {
         assert(bdrv_chain_contains(from, to));
         assert(from != to);
@@ -XXX,XX +XXX,XX @@ out:
     return ret;
 }
 
+/**
+ * Replace node @from by @to (where neither may be NULL).
+ */
 int bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
                       Error **errp)
 {
@@ -XXX,XX +XXX,XX @@ int bdrv_replace_child_bs(BdrvChild *child, BlockDriverState *new_bs,
     bdrv_drained_begin(old_bs);
     bdrv_drained_begin(new_bs);
 
-    bdrv_replace_child_tran(child, new_bs, tran);
+    bdrv_replace_child_tran(&child, new_bs, tran);
 
     found = g_hash_table_new(NULL, NULL);
     refresh_list = bdrv_topological_dfs(refresh_list, found, old_bs);
-- 
2.33.1

In most of the block layer, especially when traversing down from other
BlockDriverStates, we assume that BdrvChild.bs can never be NULL.  When
it becomes NULL, it is expected that the corresponding BdrvChild pointer
also becomes NULL and the BdrvChild object is freed.

Therefore, once bdrv_replace_child_noperm() sets the BdrvChild.bs
pointer to NULL, it should also immediately set the corresponding
BdrvChild pointer (like bs->file or bs->backing) to NULL.

In that context, it also makes sense for this function to free the
child.  Sometimes we cannot do so, though, because it is called in a
transactional context where the caller might still want to reinstate the
child in the abort branch (and free it only on commit), so this behavior
has to remain optional.

In bdrv_replace_child_tran()'s abort handler, we now rely on the fact
that the BdrvChild passed to bdrv_replace_child_tran() must have had a
non-NULL .bs pointer initially.  Make a note of that and assert it.

Signed-off-by: Hanna Reitz <hreitz@redhat.com>
Message-Id: <20211111120829.81329-10-hreitz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-Id: <20211115145409.176785-10-kwolf@redhat.com>
Signed-off-by: Hanna Reitz <hreitz@redhat.com>
---
 block.c | 102 +++++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 79 insertions(+), 23 deletions(-)

diff --git a/block.c b/block.c
index XXXXXXX..XXXXXXX 100644
--- a/block.c
+++ b/block.c
@@ -XXX,XX +XXX,XX @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
 static bool bdrv_recurse_has_child(BlockDriverState *bs,
                                    BlockDriverState *child);
 
+static void bdrv_child_free(BdrvChild *child);
 static void bdrv_replace_child_noperm(BdrvChild **child,
-                                      BlockDriverState *new_bs);
+                                      BlockDriverState *new_bs,
+                                      bool free_empty_child);
 static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,
                                               BdrvChild *child,
                                               Transaction *tran);
@@ -XXX,XX +XXX,XX @@ typedef struct BdrvReplaceChildState {
     BdrvChild *child;
     BdrvChild **childp;
     BlockDriverState *old_bs;
+    bool free_empty_child;
 } BdrvReplaceChildState;
 
 static void bdrv_replace_child_commit(void *opaque)
 {
     BdrvReplaceChildState *s = opaque;
 
+    if (s->free_empty_child && !s->child->bs) {
+        bdrv_child_free(s->child);
+    }
     bdrv_unref(s->old_bs);
 }
 
@@ -XXX,XX +XXX,XX @@ static void bdrv_replace_child_abort(void *opaque)
      *     modify the BdrvChild * pointer we indirectly pass to it, i.e. it
      *     will not modify s->child.  From that perspective, it does not matter
      *     whether we pass s->childp or &s->child.
-     *     (TODO: Right now, bdrv_replace_child_noperm() never modifies that
-     *     pointer anyway (though it will in the future), so at this point it
-     *     absolutely does not matter whether we pass s->childp or &s->child.)
      * (2) If new_bs is not NULL, s->childp will be NULL.  We then cannot use
      *     it here.
      * (3) If new_bs is NULL, *s->childp will have been NULLed by
      *     bdrv_replace_child_tran()'s bdrv_replace_child_noperm() call, and we
      *     must not pass a NULL *s->childp here.
-     *     (TODO: In its current state, bdrv_replace_child_noperm() will not
-     *     have NULLed *s->childp, so this does not apply yet.  It will in the
-     *     future.)
      *
      * So whether new_bs was NULL or not, we cannot pass s->childp here; and in
      * any case, there is no reason to pass it anyway.
      */
-    bdrv_replace_child_noperm(&s->child, s->old_bs);
+    bdrv_replace_child_noperm(&s->child, s->old_bs, true);
+    /*
+     * The child was pre-existing, so s->old_bs must be non-NULL, and
+     * s->child thus must not have been freed
+     */
+    assert(s->child != NULL);
+    if (!new_bs) {
+        /* As described above, *s->childp was cleared, so restore it */
+        assert(s->childp != NULL);
+        *s->childp = s->child;
+    }
     bdrv_unref(new_bs);
 }
 
@@ -XXX,XX +XXX,XX @@ static TransactionActionDrv bdrv_replace_child_drv = {
  *
  * The function doesn't update permissions, caller is responsible for this.
  *
+ * (*childp)->bs must not be NULL.
+ *
  * Note that if new_bs == NULL, @childp is stored in a state object attached
  * to @tran, so that the old child can be reinstated in the abort handler.
  * Therefore, if @new_bs can be NULL, @childp must stay valid until the
  * transaction is committed or aborted.
  *
- * (TODO: The reinstating does not happen yet, but it will once
- * bdrv_replace_child_noperm() NULLs *childp when new_bs is NULL.)
+ * If @free_empty_child is true and @new_bs is NULL, the BdrvChild is
+ * freed (on commit).  @free_empty_child should only be false if the
+ * caller will free the BDrvChild themselves (which may be important
+ * if this is in turn called in another transactional context).
  */
 static void bdrv_replace_child_tran(BdrvChild **childp,
                                     BlockDriverState *new_bs,
-                                    Transaction *tran)
+                                    Transaction *tran,
+                                    bool free_empty_child)
 {
     BdrvReplaceChildState *s = g_new(BdrvReplaceChildState, 1);
     *s = (BdrvReplaceChildState) {
         .child = *childp,
         .childp = new_bs == NULL ? childp : NULL,
         .old_bs = (*childp)->bs,
+        .free_empty_child = free_empty_child,
     };
     tran_add(tran, &bdrv_replace_child_drv, s);
 
+    /* The abort handler relies on this */
+    assert(s->old_bs != NULL);
+
     if (new_bs) {
         bdrv_ref(new_bs);
     }
-    bdrv_replace_child_noperm(childp, new_bs);
+    /*
+     * Pass free_empty_child=false, we will free the child (if
+     * necessary) in bdrv_replace_child_commit() (if our
+     * @free_empty_child parameter was true).
+     */
+    bdrv_replace_child_noperm(childp, new_bs, false);
     /* old_bs reference is transparently moved from *childp to @s */
 }
 
@@ -XXX,XX +XXX,XX @@ uint64_t bdrv_qapi_perm_to_blk_perm(BlockPermission qapi_perm)
     return permissions[qapi_perm];
 }
 
+/**
+ * Replace (*childp)->bs by @new_bs.
+ *
+ * If @new_bs is NULL, *childp will be set to NULL, too: BDS parents
+ * generally cannot handle a BdrvChild with .bs == NULL, so clearing
+ * BdrvChild.bs should generally immediately be followed by the
+ * BdrvChild pointer being cleared as well.
+ *
+ * If @free_empty_child is true and @new_bs is NULL, the BdrvChild is
+ * freed.  @free_empty_child should only be false if the caller will
+ * free the BdrvChild themselves (this may be important in a
+ * transactional context, where it may only be freed on commit).
+ */
 static void bdrv_replace_child_noperm(BdrvChild **childp,
-                                      BlockDriverState *new_bs)
+                                      BlockDriverState *new_bs,
+                                      bool free_empty_child)
 {
     BdrvChild *child = *childp;
     BlockDriverState *old_bs = child->bs;
@@ -XXX,XX +XXX,XX @@ static void bdrv_replace_child_noperm(BdrvChild **childp,
     }
 
     child->bs = new_bs;
+    if (!new_bs) {
+        *childp = NULL;
+    }
 
     if (new_bs) {
         QLIST_INSERT_HEAD(&new_bs->parents, child, next_parent);
@@ -XXX,XX +XXX,XX @@ static void bdrv_replace_child_noperm(BdrvChild **childp,
         bdrv_parent_drained_end_single(child);
         drain_saldo++;
     }
+
+    if (free_empty_child && !child->bs) {
+        bdrv_child_free(child);
+    }
 }
 
 /**
@@ -XXX,XX +XXX,XX @@ static void bdrv_attach_child_common_abort(void *opaque)
     BdrvChild *child = *s->child;
     BlockDriverState *bs = child->bs;
 
-    bdrv_replace_child_noperm(s->child, NULL);
+    /*
+     * Pass free_empty_child=false, because we still need the child
+     * for the AioContext operations on the parent below; those
+     * BdrvChildClass methods all work on a BdrvChild object, so we
+     * need to keep it as an empty shell (after this function, it will
+     * not be attached to any parent, and it will not have a .bs).
+     */
+    bdrv_replace_child_noperm(s->child, NULL, false);
 
     if (bdrv_get_aio_context(bs) != s->old_child_ctx) {
         bdrv_try_set_aio_context(bs, s->old_child_ctx, &error_abort);
@@ -XXX,XX +XXX,XX @@ static void bdrv_attach_child_common_abort(void *opaque)
 
     bdrv_unref(bs);
     bdrv_child_free(child);
-    *s->child = NULL;
 }
 
 static TransactionActionDrv bdrv_attach_child_common_drv = {
@@ -XXX,XX +XXX,XX @@ static int bdrv_attach_child_common(BlockDriverState *child_bs,
     }
 
     bdrv_ref(child_bs);
-    bdrv_replace_child_noperm(&new_child, child_bs);
+    bdrv_replace_child_noperm(&new_child, child_bs, true);
+    /* child_bs was non-NULL, so new_child must not have been freed */
+    assert(new_child != NULL);
 
     *child = new_child;
 
@@ -XXX,XX +XXX,XX @@ static void bdrv_detach_child(BdrvChild **childp)
 {
     BlockDriverState *old_bs = (*childp)->bs;
 
-    bdrv_replace_child_noperm(childp, NULL);
-    bdrv_child_free(*childp);
+    bdrv_replace_child_noperm(childp, NULL, true);
 
     if (old_bs) {
         /*
@@ -XXX,XX +XXX,XX @@ static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,
     }
 
     if (child->bs) {
-        bdrv_replace_child_tran(childp, NULL, tran);
+        /*
+         * Pass free_empty_child=false, we will free the child in
+         * bdrv_remove_filter_or_cow_child_commit()
+         */
+        bdrv_replace_child_tran(childp, NULL, tran, false);
     }
 
     s = g_new(BdrvRemoveFilterOrCowChild, 1);
@@ -XXX,XX +XXX,XX @@ static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,
         .is_backing = (childp == &bs->backing),
     };
     tran_add(tran, &bdrv_remove_filter_or_cow_child_drv, s);
-
-    *childp = NULL;
 }
 
 /*
@@ -XXX,XX +XXX,XX @@ static int bdrv_replace_node_noperm(BlockDriverState *from,
          * Passing a pointer to the local variable @c is fine here, because
          * @to is not NULL, and so &c will not be attached to the transaction.
          */
-        bdrv_replace_child_tran(&c, to, tran);
+        bdrv_replace_child_tran(&c, to, tran, true);
     }
 
     return 0;
@@ -XXX,XX +XXX,XX @@ int bdrv_replace_child_bs(BdrvChild *child, BlockDriverState *new_bs,
     bdrv_drained_begin(old_bs);
     bdrv_drained_begin(new_bs);
 
-    bdrv_replace_child_tran(&child, new_bs, tran);
+    bdrv_replace_child_tran(&child, new_bs, tran, true);
+    /* @new_bs must have been non-NULL, so @child must not have been freed */
+    assert(child != NULL);
 
     found = g_hash_table_new(NULL, NULL);
     refresh_list = bdrv_topological_dfs(refresh_list, found, old_bs);
-- 
2.33.1

See the comment for why this is necessary.

Signed-off-by: Hanna Reitz <hreitz@redhat.com>
Message-Id: <20211111120829.81329-11-hreitz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-Id: <20211115145409.176785-11-kwolf@redhat.com>
Signed-off-by: Hanna Reitz <hreitz@redhat.com>
---
 tests/qemu-iotests/030 | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index XXXXXXX..XXXXXXX 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -XXX,XX +XXX,XX @@ class TestParallelOps(iotests.QMPTestCase):
                                  speed=1024)
             self.assert_qmp(result, 'return', {})
 
-        for job in pending_jobs:
+        # Do this in reverse: After unthrottling them, some jobs may finish
+        # before we have unthrottled all of them.  This will drain their
+        # subgraph, and this will make jobs above them advance (despite those
+        # jobs on top being throttled).  In the worst case, all jobs below the
+        # top one are finished before we can unthrottle it, and this makes it
+        # advance so far that it completes before we can unthrottle it - which
+        # results in an error.
+        # Starting from the top (i.e. in reverse) does not have this problem:
+        # When a job finishes, the ones below it are not advanced.
+        for job in reversed(pending_jobs):
             result = self.vm.qmp('block-job-set-speed', device=job, speed=0)
             self.assert_qmp(result, 'return', {})
 
-- 
2.33.1

From: Kevin Wolf <kwolf@redhat.com>

While introducing a non-QemuOpts code path for device creation for JSON
-device, we noticed that QMP device_add doesn't check its input
correctly (accepting arguments that should have been rejected), and that
users may be relying on this behaviour (libvirt did until it was fixed
recently).

Let's use a deprecation period before we fix this bug in QEMU to avoid
nasty surprises for users.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-Id: <20211111143530.18985-1-kwolf@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-Id: <20211115145409.176785-12-kwolf@redhat.com>
Signed-off-by: Hanna Reitz <hreitz@redhat.com>
---
 docs/about/deprecated.rst | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index XXXXXXX..XXXXXXX 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -XXX,XX +XXX,XX @@ options are removed in favor of using explicit ``blockdev-create`` and
 ``blockdev-add`` calls. See :doc:`/interop/live-block-operations` for
 details.
 
+Incorrectly typed ``device_add`` arguments (since 6.2)
+''''''''''''''''''''''''''''''''''''''''''''''''''''''
+
+Due to shortcomings in the internal implementation of ``device_add``, QEMU
+incorrectly accepts certain invalid arguments: Any object or list arguments are
+silently ignored. Other argument types are not checked, but an implicit
+conversion happens, so that e.g. string values can be assigned to integer
+device properties or vice versa.
+
+This is a bug in QEMU that will be fixed in the future so that previously
+accepted incorrect commands will return an error. Users should make sure that
+all arguments passed to ``device_add`` are consistent with the documented
+property types.
+
 System accelerators
 -------------------
 
-- 
2.33.1

From: Stefan Hajnoczi <stefanha@redhat.com>

Reported by Coverity (CID 1465222).

Fixes: 4a1d937796de0fecd8b22d7dbebf87f38e8282fd ("softmmu/qdev-monitor: add error handling in qdev_set_id")
Cc: Damien Hedde <damien.hedde@greensocs.com>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-Id: <20211102163342.31162-1-stefanha@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Damien Hedde <damien.hedde@greensocs.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-Id: <20211115145409.176785-14-kwolf@redhat.com>
Signed-off-by: Hanna Reitz <hreitz@redhat.com>
---
 softmmu/qdev-monitor.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index XXXXXXX..XXXXXXX 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -XXX,XX +XXX,XX @@ const char *qdev_set_id(DeviceState *dev, char *id, Error **errp)
         if (prop) {
             dev->id = id;
         } else {
-            g_free(id);
             error_setg(errp, "Duplicate device ID '%s'", id);
+            g_free(id);
             return NULL;
         }
     } else {
-- 
2.33.1

From: Kevin Wolf <kwolf@redhat.com>

At the end of a reopen, we already call bdrv_refresh_limits(), which
should update bs->request_alignment according to the new file
descriptor. However, raw_probe_alignment() relies on s->needs_alignment
and just uses 1 if it isn't set. We neglected to update this field, so
starting with cache=writeback and then reopening with cache=none means
that we get an incorrect bs->request_alignment == 1 and unaligned
requests fail instead of being automatically aligned.

Fix this by recalculating s->needs_alignment in raw_refresh_limits()
before calling raw_probe_alignment().

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-Id: <20211104113109.56336-1-kwolf@redhat.com>
Reviewed-by: Hanna Reitz <hreitz@redhat.com>
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-Id: <20211115145409.176785-13-kwolf@redhat.com>
[hreitz: Fix iotest 142 for block sizes greater than 512 by operating on
         a file with a size of 1 MB]
Signed-off-by: Hanna Reitz <hreitz@redhat.com>
Message-Id: <20211116101431.105252-1-hreitz@redhat.com>
---
 block/file-posix.c         | 20 ++++++++++++++++----
 tests/qemu-iotests/142     | 29 +++++++++++++++++++++++++++++
 tests/qemu-iotests/142.out | 18 ++++++++++++++++++
 3 files changed, 63 insertions(+), 4 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index XXXXXXX..XXXXXXX 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -XXX,XX +XXX,XX @@ typedef struct BDRVRawState {
     int page_cache_inconsistent; /* errno from fdatasync failure */
     bool has_fallocate;
     bool needs_alignment;
+    bool force_alignment;
     bool drop_cache;
     bool check_cache_dropped;
     struct {
@@ -XXX,XX +XXX,XX @@ static bool dio_byte_aligned(int fd)
     return false;
 }
 
+static bool raw_needs_alignment(BlockDriverState *bs)
+{
+    BDRVRawState *s = bs->opaque;
+
+    if ((bs->open_flags & BDRV_O_NOCACHE) != 0 && !dio_byte_aligned(s->fd)) {
+        return true;
+    }
+
+    return s->force_alignment;
+}
+
 /* Check if read is allowed with given memory buffer and length.
  *
  * This function is used to check O_DIRECT memory buffer and request alignment.
@@ -XXX,XX +XXX,XX @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
 
     s->has_discard = true;
     s->has_write_zeroes = true;
-    if ((bs->open_flags & BDRV_O_NOCACHE) != 0 && !dio_byte_aligned(s->fd)) {
-        s->needs_alignment = true;
-    }
 
     if (fstat(s->fd, &st) < 0) {
         ret = -errno;
@@ -XXX,XX +XXX,XX @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
          * so QEMU makes sure all IO operations on the device are aligned
          * to sector size, or else FreeBSD will reject them with EINVAL.
          */
-        s->needs_alignment = true;
+        s->force_alignment = true;
     }
 #endif
+    s->needs_alignment = raw_needs_alignment(bs);
 
 #ifdef CONFIG_XFS
     if (platform_test_xfs_fd(s->fd)) {
@@ -XXX,XX +XXX,XX @@ static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
     BDRVRawState *s = bs->opaque;
     struct stat st;
 
+    s->needs_alignment = raw_needs_alignment(bs);
     raw_probe_alignment(bs, s->fd, errp);
+
     bs->bl.min_mem_alignment = s->buf_align;
     bs->bl.opt_mem_alignment = MAX(s->buf_align, qemu_real_host_page_size);
 
diff --git a/tests/qemu-iotests/142 b/tests/qemu-iotests/142
index XXXXXXX..XXXXXXX 100755
--- a/tests/qemu-iotests/142
+++ b/tests/qemu-iotests/142
@@ -XXX,XX +XXX,XX @@ info block backing-file"
 
 echo "$hmp_cmds" | run_qemu -drive "$files","$ids" | grep "Cache"
 
+echo
+echo "--- Alignment after changing O_DIRECT ---"
+echo
+
+# Directly test the protocol level: Can unaligned requests succeed even if
+# O_DIRECT was only enabled through a reopen and vice versa?
+
+# Ensure image size is a multiple of the sector size (required for O_DIRECT)
+$QEMU_IMG create -f file "$TEST_IMG" 1M | _filter_img_create
+
+# And write some data (not strictly necessary, but it feels better to actually
+# have something to be read)
+$QEMU_IO -f file -c 'write 0 4096' "$TEST_IMG" | _filter_qemu_io
+
+$QEMU_IO --cache=writeback -f file $TEST_IMG <<EOF | _filter_qemu_io
+read 42 42
+reopen -o cache.direct=on
+read 42 42
+reopen -o cache.direct=off
+read 42 42
+EOF
+$QEMU_IO --cache=none -f file $TEST_IMG <<EOF | _filter_qemu_io
+read 42 42
+reopen -o cache.direct=off
+read 42 42
+reopen -o cache.direct=on
+read 42 42
+EOF
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/142.out b/tests/qemu-iotests/142.out
index XXXXXXX..XXXXXXX 100644
--- a/tests/qemu-iotests/142.out
+++ b/tests/qemu-iotests/142.out
@@ -XXX,XX +XXX,XX @@ cache.no-flush=on on backing-file
     Cache mode:       writeback
     Cache mode:       writeback, direct
     Cache mode:       writeback, ignore flushes
+
+--- Alignment after changing O_DIRECT ---
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=file size=1048576
+wrote 4096/4096 bytes at offset 0
+4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 42/42 bytes at offset 42
+42 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 42/42 bytes at offset 42
+42 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 42/42 bytes at offset 42
+42 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 42/42 bytes at offset 42
+42 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 42/42 bytes at offset 42
+42 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 42/42 bytes at offset 42
+42 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 *** done
-- 
2.33.1