[Qemu-devel] [PULL 00/27] Block layer fixes for 2.9.0-rc0

[Qemu-devel] [PULL 00/27] Block layer fixes for 2.9.0-rc0

[Kevin Wolf]

The following changes since commit ff79d5e939c38677a575e3493eb9b4d36eb21865:

  Merge remote-tracking branch 'remotes/xtensa/tags/20170306-xtensa' into staging (2017-03-07 09:57:14 +0000)

are available in the git repository at:


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

for you to fetch changes up to b69f00dde490e88d55f5ee731545e690b2dc61f8:

  commit: Don't use error_abort in commit_start (2017-03-07 14:53:29 +0100)

----------------------------------------------------------------
Block layer fixes for 2.9.0-rc0

----------------------------------------------------------------
Fam Zheng (2):
      block: Don't use error_abort in blk_new_open
      commit: Don't use error_abort in commit_start

Kevin Wolf (10):
      commit: Fix error handling
      mirror: Fix permission problem with 'replaces'
      mirror: Fix permissions for removing mirror_top_bs
      mirror: Fix error path for dirty bitmap creation
      block: Fix blockdev-snapshot error handling
      block: Factor out should_update_child()
      block: Factor out bdrv_replace_child_noperm()
      block: Ignore multiple children in bdrv_check_update_perm()
      block: Handle permission errors in change_parent_backing_link()
      block: Fix error handling in bdrv_replace_in_backing_chain()

Markus Armbruster (15):
      sheepdog: Defuse time bomb in sd_open() error handling
      sheepdog: Fix error handling in sd_snapshot_delete()
      sheepdog: Fix error handling sd_create()
      sheepdog: Mark sd_snapshot_delete() lossage FIXME
      sheepdog: Fix snapshot ID parsing in _open(), _create, _goto()
      sheepdog: Don't truncate long VDI name in _open(), _create()
      sheepdog: Report errors in pseudo-filename more usefully
      sheepdog: Use SocketAddress and socket_connect()
      sheepdog: Implement bdrv_parse_filename()
      gluster: Drop assumptions on SocketTransport names
      gluster: Don't duplicate qapi-util.c's qapi_enum_parse()
      gluster: Plug memory leaks in qemu_gluster_parse_json()
      qapi-schema: Rename GlusterServer to SocketAddressFlat
      qapi-schema: Rename SocketAddressFlat's variant tcp to inet
      sheepdog: Support blockdev-add

 block.c                   | 182 ++++++++++++-------
 block/block-backend.c     |   7 +-
 block/commit.c            |  18 +-
 block/gluster.c           | 127 ++++++-------
 block/mirror.c            |  35 ++--
 block/sheepdog.c          | 453 +++++++++++++++++++++++++++++++++-------------
 blockdev.c                |   6 +-
 include/block/block.h     |   4 +-
 include/block/block_int.h |   6 +-
 qapi-schema.json          |  38 ++++
 qapi/block-core.json      |  73 +++-----
 11 files changed, 623 insertions(+), 326 deletions(-)

[Qemu-devel] [PULL 01/27] commit: Fix error handling

[Kevin Wolf]

Apparently some kind of mismerge happened in commit 8dfba279, which
broke the error handling without any real reason by removing the
assignment of the return value to ret in a blk_insert_bs() call.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 block/commit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/commit.c b/block/commit.c
index 22a0a4d..e57c1cf 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -364,7 +364,7 @@ void commit_start(const char *job_id, BlockDriverState *bs,
 
     /* Required permissions are already taken with block_job_add_bdrv() */
     s->top = blk_new(0, BLK_PERM_ALL);
-    blk_insert_bs(s->top, top, errp);
+    ret = blk_insert_bs(s->top, top, errp);
     if (ret < 0) {
         goto fail;
     }
-- 
1.8.3.1

[Qemu-devel] [PULL 02/27] mirror: Fix permission problem with 'replaces'

[Kevin Wolf]

The 'replaces' option of drive-mirror can be used to mirror a Quorum
node to a new image and then let the target image replace one of the
Quorum children. In order for this graph modification to succeed, the
mirror job needs to lift its restrictions on the target node first
before actually replacing the child.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block/mirror.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 57f26c3..c9185b3 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -509,6 +509,13 @@ static void mirror_exit(BlockJob *job, void *opaque)
      * block_job_completed(). */
     bdrv_ref(src);
     bdrv_ref(mirror_top_bs);
+    bdrv_ref(target_bs);
+
+    /* Remove target parent that still uses BLK_PERM_WRITE/RESIZE before
+     * inserting target_bs at s->to_replace, where we might not be able to get
+     * these permissions. */
+    blk_unref(s->target);
+    s->target = NULL;
 
     /* We don't access the source any more. Dropping any WRITE/RESIZE is
      * required before it could become a backing file of target_bs. */
@@ -555,8 +562,7 @@ static void mirror_exit(BlockJob *job, void *opaque)
         aio_context_release(replace_aio_context);
     }
     g_free(s->replaces);
-    blk_unref(s->target);
-    s->target = NULL;
+    bdrv_unref(target_bs);
 
     /* Remove the mirror filter driver from the graph. Before this, get rid of
      * the blockers on the intermediate nodes so that the resulting state is
-- 
1.8.3.1

[Qemu-devel] [PULL 03/27] mirror: Fix permissions for removing mirror_top_bs

[Kevin Wolf]

mirror_top_bs takes write permissions on its backing file, which can
make it impossible to attach that backing file node to another parent.
However, this is exactly what needs to be done in order to remove
mirror_top_bs from the backing chain. So give up the write permission
first.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block/mirror.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/block/mirror.c b/block/mirror.c
index c9185b3..001b5f0 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -566,8 +566,10 @@ static void mirror_exit(BlockJob *job, void *opaque)
 
     /* Remove the mirror filter driver from the graph. Before this, get rid of
      * the blockers on the intermediate nodes so that the resulting state is
-     * valid. */
+     * valid. Also give up permissions on mirror_top_bs->backing, which might
+     * block the removal. */
     block_job_remove_all_bdrv(job);
+    bdrv_child_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL);
     bdrv_replace_in_backing_chain(mirror_top_bs, backing_bs(mirror_top_bs));
 
     /* We just changed the BDS the job BB refers to (with either or both of the
@@ -1234,6 +1236,7 @@ fail:
         block_job_unref(&s->common);
     }
 
+    bdrv_child_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL);
     bdrv_replace_in_backing_chain(mirror_top_bs, backing_bs(mirror_top_bs));
 }
 
-- 
1.8.3.1

[Qemu-devel] [PULL 04/27] mirror: Fix error path for dirty bitmap creation

[Kevin Wolf]

mirror_top_bs must be removed from the graph again when creating the
dirty bitmap fails.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block/mirror.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 001b5f0..f24dc51 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1197,10 +1197,7 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
 
     s->dirty_bitmap = bdrv_create_dirty_bitmap(bs, granularity, NULL, errp);
     if (!s->dirty_bitmap) {
-        g_free(s->replaces);
-        blk_unref(s->target);
-        block_job_unref(&s->common);
-        return;
+        goto fail;
     }
 
     /* Required permissions are already taken with blk_new() */
-- 
1.8.3.1

[Qemu-devel] [PULL 05/27] block: Fix blockdev-snapshot error handling

[Kevin Wolf]

For blockdev-snapshot, external_snapshot_prepare() accepts an arbitrary
node reference at first and only checks later whether it already has a
backing file. Between those places, other errors can occur.

Therefore checking in external_snapshot_abort() whether state->new_bs
has a backing file is not sufficient to tell whether bdrv_append() was
already completed or not. Trying to undo the bdrv_append() when it
wasn't even executed is wrong.

Introduce a new boolean flag in the state to fix this.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 blockdev.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/blockdev.c b/blockdev.c
index 8eb4e84..af67ce4 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1614,6 +1614,7 @@ typedef struct ExternalSnapshotState {
     BlockDriverState *old_bs;
     BlockDriverState *new_bs;
     AioContext *aio_context;
+    bool overlay_appended;
 } ExternalSnapshotState;
 
 static void external_snapshot_prepare(BlkActionState *common,
@@ -1780,6 +1781,7 @@ static void external_snapshot_prepare(BlkActionState *common,
         error_propagate(errp, local_err);
         return;
     }
+    state->overlay_appended = true;
 }
 
 static void external_snapshot_commit(BlkActionState *common)
@@ -1803,7 +1805,7 @@ static void external_snapshot_abort(BlkActionState *common)
     ExternalSnapshotState *state =
                              DO_UPCAST(ExternalSnapshotState, common, common);
     if (state->new_bs) {
-        if (state->new_bs->backing) {
+        if (state->overlay_appended) {
             bdrv_replace_in_backing_chain(state->new_bs, state->old_bs);
         }
     }
-- 
1.8.3.1

[Qemu-devel] [PULL 06/27] block: Factor out should_update_child()

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 block.c | 42 +++++++++++++++++++++++++++---------------
 1 file changed, 27 insertions(+), 15 deletions(-)

diff --git a/block.c b/block.c
index f293ccb..6dc02b8 100644
--- a/block.c
+++ b/block.c
@@ -2886,28 +2886,40 @@ void bdrv_close_all(void)
     assert(QTAILQ_EMPTY(&all_bdrv_states));
 }
 
+static bool should_update_child(BdrvChild *c, BlockDriverState *to)
+{
+    BdrvChild *to_c;
+
+    if (c->role->stay_at_node) {
+        return false;
+    }
+
+    if (c->role == &child_backing) {
+        /* If @from is a backing file of @to, ignore the child to avoid
+         * creating a loop. We only want to change the pointer of other
+         * parents. */
+        QLIST_FOREACH(to_c, &to->children, next) {
+            if (to_c == c) {
+                break;
+            }
+        }
+        if (to_c) {
+            return false;
+        }
+    }
+
+    return true;
+}
+
 static void change_parent_backing_link(BlockDriverState *from,
                                        BlockDriverState *to)
 {
-    BdrvChild *c, *next, *to_c;
+    BdrvChild *c, *next;
 
     QLIST_FOREACH_SAFE(c, &from->parents, next_parent, next) {
-        if (c->role->stay_at_node) {
+        if (!should_update_child(c, to)) {
             continue;
         }
-        if (c->role == &child_backing) {
-            /* If @from is a backing file of @to, ignore the child to avoid
-             * creating a loop. We only want to change the pointer of other
-             * parents. */
-            QLIST_FOREACH(to_c, &to->children, next) {
-                if (to_c == c) {
-                    break;
-                }
-            }
-            if (to_c) {
-                continue;
-            }
-        }
 
         bdrv_ref(to);
         /* FIXME Are we sure that bdrv_replace_child() can't run into
-- 
1.8.3.1

[Qemu-devel] [PULL 07/27] block: Factor out bdrv_replace_child_noperm()

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block.c | 38 +++++++++++++++++++++++++-------------
 1 file changed, 25 insertions(+), 13 deletions(-)

diff --git a/block.c b/block.c
index 6dc02b8..d4570c8 100644
--- a/block.c
+++ b/block.c
@@ -1713,11 +1713,10 @@ void bdrv_format_default_perms(BlockDriverState *bs, BdrvChild *c,
     *nshared = shared;
 }
 
-static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs,
-                               bool check_new_perm)
+static void bdrv_replace_child_noperm(BdrvChild *child,
+                                      BlockDriverState *new_bs)
 {
     BlockDriverState *old_bs = child->bs;
-    uint64_t perm, shared_perm;
 
     if (old_bs) {
         if (old_bs->quiesce_counter && child->role->drained_end) {
@@ -1727,7 +1726,29 @@ static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs,
             child->role->detach(child);
         }
         QLIST_REMOVE(child, next_parent);
+    }
+
+    child->bs = new_bs;
+
+    if (new_bs) {
+        QLIST_INSERT_HEAD(&new_bs->parents, child, next_parent);
+        if (new_bs->quiesce_counter && child->role->drained_begin) {
+            child->role->drained_begin(child);
+        }
+
+        if (child->role->attach) {
+            child->role->attach(child);
+        }
+    }
+}
 
+static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs,
+                               bool check_new_perm)
+{
+    BlockDriverState *old_bs = child->bs;
+    uint64_t perm, shared_perm;
+
+    if (old_bs) {
         /* Update permissions for old node. This is guaranteed to succeed
          * because we're just taking a parent away, so we're loosening
          * restrictions. */
@@ -1736,23 +1757,14 @@ static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs,
         bdrv_set_perm(old_bs, perm, shared_perm);
     }
 
-    child->bs = new_bs;
+    bdrv_replace_child_noperm(child, new_bs);
 
     if (new_bs) {
-        QLIST_INSERT_HEAD(&new_bs->parents, child, next_parent);
-        if (new_bs->quiesce_counter && child->role->drained_begin) {
-            child->role->drained_begin(child);
-        }
-
         bdrv_get_cumulative_perm(new_bs, &perm, &shared_perm);
         if (check_new_perm) {
             bdrv_check_perm(new_bs, perm, shared_perm, &error_abort);
         }
         bdrv_set_perm(new_bs, perm, shared_perm);
-
-        if (child->role->attach) {
-            child->role->attach(child);
-        }
     }
 }
 
-- 
1.8.3.1

[Qemu-devel] [PULL 08/27] block: Ignore multiple children in bdrv_check_update_perm()

[Kevin Wolf]

change_parent_backing_link() will need to update multiple BdrvChild
objects at once. Checking permissions reference by reference doesn't
work because permissions need to be consistent only with all parents
moved to the new child.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block.c                   | 35 ++++++++++++++++++++++-------------
 include/block/block_int.h |  2 +-
 2 files changed, 23 insertions(+), 14 deletions(-)

diff --git a/block.c b/block.c
index d4570c8..a7b09d3 100644
--- a/block.c
+++ b/block.c
@@ -1398,7 +1398,8 @@ static int bdrv_fill_options(QDict **options, const char *filename,
  * or bdrv_abort_perm_update().
  */
 static int bdrv_check_perm(BlockDriverState *bs, uint64_t cumulative_perms,
-                           uint64_t cumulative_shared_perms, Error **errp)
+                           uint64_t cumulative_shared_perms,
+                           GSList *ignore_children, Error **errp)
 {
     BlockDriver *drv = bs->drv;
     BdrvChild *c;
@@ -1434,7 +1435,8 @@ static int bdrv_check_perm(BlockDriverState *bs, uint64_t cumulative_perms,
         drv->bdrv_child_perm(bs, c, c->role,
                              cumulative_perms, cumulative_shared_perms,
                              &cur_perm, &cur_shared);
-        ret = bdrv_child_check_perm(c, cur_perm, cur_shared, errp);
+        ret = bdrv_child_check_perm(c, cur_perm, cur_shared, ignore_children,
+                                    errp);
         if (ret < 0) {
             return ret;
         }
@@ -1554,15 +1556,15 @@ static char *bdrv_perm_names(uint64_t perm)
 
 /*
  * Checks whether a new reference to @bs can be added if the new user requires
- * @new_used_perm/@new_shared_perm as its permissions. If @ignore_child is set,
- * this old reference is ignored in the calculations; this allows checking
- * permission updates for an existing reference.
+ * @new_used_perm/@new_shared_perm as its permissions. If @ignore_children is
+ * set, the BdrvChild objects in this list are ignored in the calculations;
+ * this allows checking permission updates for an existing reference.
  *
  * Needs to be followed by a call to either bdrv_set_perm() or
  * bdrv_abort_perm_update(). */
 static int bdrv_check_update_perm(BlockDriverState *bs, uint64_t new_used_perm,
                                   uint64_t new_shared_perm,
-                                  BdrvChild *ignore_child, Error **errp)
+                                  GSList *ignore_children, Error **errp)
 {
     BdrvChild *c;
     uint64_t cumulative_perms = new_used_perm;
@@ -1572,7 +1574,7 @@ static int bdrv_check_update_perm(BlockDriverState *bs, uint64_t new_used_perm,
     assert(new_shared_perm & BLK_PERM_WRITE_UNCHANGED);
 
     QLIST_FOREACH(c, &bs->parents, next_parent) {
-        if (c == ignore_child) {
+        if (g_slist_find(ignore_children, c)) {
             continue;
         }
 
@@ -1602,15 +1604,22 @@ static int bdrv_check_update_perm(BlockDriverState *bs, uint64_t new_used_perm,
         cumulative_shared_perms &= c->shared_perm;
     }
 
-    return bdrv_check_perm(bs, cumulative_perms, cumulative_shared_perms, errp);
+    return bdrv_check_perm(bs, cumulative_perms, cumulative_shared_perms,
+                           ignore_children, errp);
 }
 
 /* Needs to be followed by a call to either bdrv_child_set_perm() or
  * bdrv_child_abort_perm_update(). */
 int bdrv_child_check_perm(BdrvChild *c, uint64_t perm, uint64_t shared,
-                          Error **errp)
+                          GSList *ignore_children, Error **errp)
 {
-    return bdrv_check_update_perm(c->bs, perm, shared, c, errp);
+    int ret;
+
+    ignore_children = g_slist_prepend(g_slist_copy(ignore_children), c);
+    ret = bdrv_check_update_perm(c->bs, perm, shared, ignore_children, errp);
+    g_slist_free(ignore_children);
+
+    return ret;
 }
 
 void bdrv_child_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared)
@@ -1635,7 +1644,7 @@ int bdrv_child_try_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared,
 {
     int ret;
 
-    ret = bdrv_child_check_perm(c, perm, shared, errp);
+    ret = bdrv_child_check_perm(c, perm, shared, NULL, errp);
     if (ret < 0) {
         bdrv_child_abort_perm_update(c);
         return ret;
@@ -1753,7 +1762,7 @@ static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs,
          * because we're just taking a parent away, so we're loosening
          * restrictions. */
         bdrv_get_cumulative_perm(old_bs, &perm, &shared_perm);
-        bdrv_check_perm(old_bs, perm, shared_perm, &error_abort);
+        bdrv_check_perm(old_bs, perm, shared_perm, NULL, &error_abort);
         bdrv_set_perm(old_bs, perm, shared_perm);
     }
 
@@ -1762,7 +1771,7 @@ static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs,
     if (new_bs) {
         bdrv_get_cumulative_perm(new_bs, &perm, &shared_perm);
         if (check_new_perm) {
-            bdrv_check_perm(new_bs, perm, shared_perm, &error_abort);
+            bdrv_check_perm(new_bs, perm, shared_perm, NULL, &error_abort);
         }
         bdrv_set_perm(new_bs, perm, shared_perm);
     }
diff --git a/include/block/block_int.h b/include/block/block_int.h
index a57c0bf..fc83f7f 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -890,7 +890,7 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
 void bdrv_root_unref_child(BdrvChild *child);
 
 int bdrv_child_check_perm(BdrvChild *c, uint64_t perm, uint64_t shared,
-                          Error **errp);
+                          GSList *ignore_children, Error **errp);
 void bdrv_child_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared);
 void bdrv_child_abort_perm_update(BdrvChild *c);
 int bdrv_child_try_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared,
-- 
1.8.3.1

[Qemu-devel] [PULL 09/27] block: Handle permission errors in change_parent_backing_link()

[Kevin Wolf]

Instead of just trying to change parents by parent over to reference @to
instead of @from, and abort()ing whenever the permissions don't allow
this, do proper permission checking beforehand and pass any error to the
callers.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block.c | 50 ++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 44 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index a7b09d3..a310132 100644
--- a/block.c
+++ b/block.c
@@ -2933,21 +2933,53 @@ static bool should_update_child(BdrvChild *c, BlockDriverState *to)
 }
 
 static void change_parent_backing_link(BlockDriverState *from,
-                                       BlockDriverState *to)
+                                       BlockDriverState *to, Error **errp)
 {
     BdrvChild *c, *next;
+    GSList *list = NULL, *p;
+    uint64_t old_perm, old_shared;
+    uint64_t perm = 0, shared = BLK_PERM_ALL;
+    int ret;
+
+    /* Make sure that @from doesn't go away until we have successfully attached
+     * all of its parents to @to. */
+    bdrv_ref(from);
 
+    /* Put all parents into @list and calculate their cumulative permissions */
     QLIST_FOREACH_SAFE(c, &from->parents, next_parent, next) {
         if (!should_update_child(c, to)) {
             continue;
         }
+        list = g_slist_prepend(list, c);
+        perm |= c->perm;
+        shared &= c->shared_perm;
+    }
+
+    /* Check whether the required permissions can be granted on @to, ignoring
+     * all BdrvChild in @list so that they can't block themselves. */
+    ret = bdrv_check_update_perm(to, perm, shared, list, errp);
+    if (ret < 0) {
+        bdrv_abort_perm_update(to);
+        goto out;
+    }
+
+    /* Now actually perform the change. We performed the permission check for
+     * all elements of @list at once, so set the permissions all at once at the
+     * very end. */
+    for (p = list; p != NULL; p = p->next) {
+        c = p->data;
 
         bdrv_ref(to);
-        /* FIXME Are we sure that bdrv_replace_child() can't run into
-         * &error_abort because of permissions? */
-        bdrv_replace_child(c, to, true);
+        bdrv_replace_child_noperm(c, to);
         bdrv_unref(from);
     }
+
+    bdrv_get_cumulative_perm(to, &old_perm, &old_shared);
+    bdrv_set_perm(to, old_perm | perm, old_shared | shared);
+
+out:
+    g_slist_free(list);
+    bdrv_unref(from);
 }
 
 /*
@@ -2980,7 +3012,12 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
         goto out;
     }
 
-    change_parent_backing_link(bs_top, bs_new);
+    change_parent_backing_link(bs_top, bs_new, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        bdrv_set_backing_hd(bs_new, NULL, &error_abort);
+        goto out;
+    }
 
     /* bs_new is now referenced by its new parents, we don't need the
      * additional reference any more. */
@@ -2995,7 +3032,8 @@ void bdrv_replace_in_backing_chain(BlockDriverState *old, BlockDriverState *new)
 
     bdrv_ref(old);
 
-    change_parent_backing_link(old, new);
+    /* FIXME Proper error handling */
+    change_parent_backing_link(old, new, &error_abort);
 
     bdrv_unref(old);
 }
-- 
1.8.3.1

[Qemu-devel] [PULL 10/27] block: Fix error handling in bdrv_replace_in_backing_chain()

[Kevin Wolf]

When adding an Error parameter, bdrv_replace_in_backing_chain() would
become nothing more than a wrapper around change_parent_backing_link().
So make the latter public, renamed as bdrv_replace_node(), and remove
bdrv_replace_in_backing_chain().

Most of the callers just remove a node from the graph that they just
inserted, so they can use &error_abort, but completion of a mirror job
with 'replaces' set can actually fail.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block.c                   | 25 ++++++-------------------
 block/mirror.c            | 15 +++++++++------
 blockdev.c                |  2 +-
 include/block/block.h     |  4 ++--
 include/block/block_int.h |  4 ++--
 5 files changed, 20 insertions(+), 30 deletions(-)

diff --git a/block.c b/block.c
index a310132..dd9ded8 100644
--- a/block.c
+++ b/block.c
@@ -2932,8 +2932,8 @@ static bool should_update_child(BdrvChild *c, BlockDriverState *to)
     return true;
 }
 
-static void change_parent_backing_link(BlockDriverState *from,
-                                       BlockDriverState *to, Error **errp)
+void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
+                       Error **errp)
 {
     BdrvChild *c, *next;
     GSList *list = NULL, *p;
@@ -2941,6 +2941,9 @@ static void change_parent_backing_link(BlockDriverState *from,
     uint64_t perm = 0, shared = BLK_PERM_ALL;
     int ret;
 
+    assert(!atomic_read(&from->in_flight));
+    assert(!atomic_read(&to->in_flight));
+
     /* Make sure that @from doesn't go away until we have successfully attached
      * all of its parents to @to. */
     bdrv_ref(from);
@@ -3003,16 +3006,13 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
 {
     Error *local_err = NULL;
 
-    assert(!atomic_read(&bs_top->in_flight));
-    assert(!atomic_read(&bs_new->in_flight));
-
     bdrv_set_backing_hd(bs_new, bs_top, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         goto out;
     }
 
-    change_parent_backing_link(bs_top, bs_new, &local_err);
+    bdrv_replace_node(bs_top, bs_new, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         bdrv_set_backing_hd(bs_new, NULL, &error_abort);
@@ -3025,19 +3025,6 @@ out:
     bdrv_unref(bs_new);
 }
 
-void bdrv_replace_in_backing_chain(BlockDriverState *old, BlockDriverState *new)
-{
-    assert(!bdrv_requests_pending(old));
-    assert(!bdrv_requests_pending(new));
-
-    bdrv_ref(old);
-
-    /* FIXME Proper error handling */
-    change_parent_backing_link(old, new, &error_abort);
-
-    bdrv_unref(old);
-}
-
 static void bdrv_delete(BlockDriverState *bs)
 {
     assert(!bs->job);
diff --git a/block/mirror.c b/block/mirror.c
index f24dc51..a5d30ee 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -550,8 +550,12 @@ static void mirror_exit(BlockJob *job, void *opaque)
         /* The mirror job has no requests in flight any more, but we need to
          * drain potential other users of the BDS before changing the graph. */
         bdrv_drained_begin(target_bs);
-        bdrv_replace_in_backing_chain(to_replace, target_bs);
+        bdrv_replace_node(to_replace, target_bs, &local_err);
         bdrv_drained_end(target_bs);
+        if (local_err) {
+            error_report_err(local_err);
+            data->ret = -EPERM;
+        }
     }
     if (s->to_replace) {
         bdrv_op_unblock_all(s->to_replace, s->replace_blocker);
@@ -570,12 +574,11 @@ static void mirror_exit(BlockJob *job, void *opaque)
      * block the removal. */
     block_job_remove_all_bdrv(job);
     bdrv_child_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL);
-    bdrv_replace_in_backing_chain(mirror_top_bs, backing_bs(mirror_top_bs));
+    bdrv_replace_node(mirror_top_bs, backing_bs(mirror_top_bs), &error_abort);
 
     /* We just changed the BDS the job BB refers to (with either or both of the
-     * bdrv_replace_in_backing_chain() calls), so switch the BB back so the
-     * cleanup does the right thing. We don't need any permissions any more
-     * now. */
+     * bdrv_replace_node() calls), so switch the BB back so the cleanup does
+     * the right thing. We don't need any permissions any more now. */
     blk_remove_bs(job->blk);
     blk_set_perm(job->blk, 0, BLK_PERM_ALL, &error_abort);
     blk_insert_bs(job->blk, mirror_top_bs, &error_abort);
@@ -1234,7 +1237,7 @@ fail:
     }
 
     bdrv_child_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL);
-    bdrv_replace_in_backing_chain(mirror_top_bs, backing_bs(mirror_top_bs));
+    bdrv_replace_node(mirror_top_bs, backing_bs(mirror_top_bs), &error_abort);
 }
 
 void mirror_start(const char *job_id, BlockDriverState *bs,
diff --git a/blockdev.c b/blockdev.c
index af67ce4..f1f49bd 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1806,7 +1806,7 @@ static void external_snapshot_abort(BlkActionState *common)
                              DO_UPCAST(ExternalSnapshotState, common, common);
     if (state->new_bs) {
         if (state->overlay_appended) {
-            bdrv_replace_in_backing_chain(state->new_bs, state->old_bs);
+            bdrv_replace_node(state->new_bs, state->old_bs, &error_abort);
         }
     }
 }
diff --git a/include/block/block.h b/include/block/block.h
index c7c4a3a..5149260 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -238,8 +238,8 @@ int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp);
 BlockDriverState *bdrv_new(void);
 void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
                  Error **errp);
-void bdrv_replace_in_backing_chain(BlockDriverState *old,
-                                   BlockDriverState *new);
+void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
+                       Error **errp);
 
 int bdrv_parse_cache_mode(const char *mode, int *flags, bool *writethrough);
 int bdrv_parse_discard_flags(const char *mode, int *flags);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index fc83f7f..6c699ac 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -441,8 +441,8 @@ typedef struct BdrvAioNotifier {
 } BdrvAioNotifier;
 
 struct BdrvChildRole {
-    /* If true, bdrv_replace_in_backing_chain() doesn't change the node this
-     * BdrvChild points to. */
+    /* If true, bdrv_replace_node() doesn't change the node this BdrvChild
+     * points to. */
     bool stay_at_node;
 
     void (*inherit_options)(int *child_flags, QDict *child_options,
-- 
1.8.3.1

[Qemu-devel] [PULL 11/27] sheepdog: Defuse time bomb in sd_open() error handling

[Kevin Wolf]

From: Markus Armbruster <armbru@redhat.com>

When qemu_opts_absorb_qdict() fails, sd_open() closes stdin, because
sd->fd is still zero.  Fortunately, qemu_opts_absorb_qdict() can't
fail, because:

1. it only fails when qemu_opt_parse() fails, and
2. the only member of runtime_opts.desc[] is a QEMU_OPT_STRING, and
3. qemu_opt_parse() can't fail for QEMU_OPT_STRING.

Defuse this ticking time bomb by jumping behind the file descriptor
cleanup on error.

Also do that for the error paths where sd->fd is still -1.  The file
descriptor cleanup happens to do nothing then, but let's not rely on
that here.

While there, rename label out to err, because it's on the error path,
not the normal path out of the function.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/sheepdog.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 7434710..c3ee4ce 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1392,7 +1392,7 @@ static int sd_open(BlockDriverState *bs, QDict *options, int flags,
     if (local_err) {
         error_propagate(errp, local_err);
         ret = -EINVAL;
-        goto out;
+        goto err_no_fd;
     }
 
     filename = qemu_opt_get(opts, "filename");
@@ -1412,17 +1412,17 @@ static int sd_open(BlockDriverState *bs, QDict *options, int flags,
     }
     if (ret < 0) {
         error_setg(errp, "Can't parse filename");
-        goto out;
+        goto err_no_fd;
     }
     s->fd = get_sheep_fd(s, errp);
     if (s->fd < 0) {
         ret = s->fd;
-        goto out;
+        goto err_no_fd;
     }
 
     ret = find_vdi_name(s, vdi, snapid, tag, &vid, true, errp);
     if (ret) {
-        goto out;
+        goto err;
     }
 
     /*
@@ -1443,7 +1443,7 @@ static int sd_open(BlockDriverState *bs, QDict *options, int flags,
     fd = connect_to_sdog(s, errp);
     if (fd < 0) {
         ret = fd;
-        goto out;
+        goto err;
     }
 
     buf = g_malloc(SD_INODE_SIZE);
@@ -1454,7 +1454,7 @@ static int sd_open(BlockDriverState *bs, QDict *options, int flags,
 
     if (ret) {
         error_setg(errp, "Can't read snapshot inode");
-        goto out;
+        goto err;
     }
 
     memcpy(&s->inode, buf, sizeof(s->inode));
@@ -1466,12 +1466,12 @@ static int sd_open(BlockDriverState *bs, QDict *options, int flags,
     qemu_opts_del(opts);
     g_free(buf);
     return 0;
-out:
+
+err:
     aio_set_fd_handler(bdrv_get_aio_context(bs), s->fd,
                        false, NULL, NULL, NULL, NULL);
-    if (s->fd >= 0) {
-        closesocket(s->fd);
-    }
+    closesocket(s->fd);
+err_no_fd:
     qemu_opts_del(opts);
     g_free(buf);
     return ret;
-- 
1.8.3.1

[Qemu-devel] [PULL 12/27] sheepdog: Fix error handling in sd_snapshot_delete()

[Kevin Wolf]

From: Markus Armbruster <armbru@redhat.com>

As a bdrv_snapshot_delete() method, sd_snapshot_delete() must set an
error and return negative errno on failure.  It sometimes returns -1,
and sometimes neglects to set an error.  It also prints error messages
with error_report().  Fix all that.

Moreover, its handling of an attempt to delete a nonexistent snapshot
is wrong: it error_report()s and succeeds.  Fix it to set an error and
return -ENOENT instead.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/sheepdog.c | 41 +++++++++++++++++++----------------------
 1 file changed, 19 insertions(+), 22 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index c3ee4ce..0a0803e 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -2405,18 +2405,15 @@ out:
 
 #define NR_BATCHED_DISCARD 128
 
-static bool remove_objects(BDRVSheepdogState *s)
+static int remove_objects(BDRVSheepdogState *s, Error **errp)
 {
     int fd, i = 0, nr_objs = 0;
-    Error *local_err = NULL;
-    int ret = 0;
-    bool result = true;
+    int ret;
     SheepdogInode *inode = &s->inode;
 
-    fd = connect_to_sdog(s, &local_err);
+    fd = connect_to_sdog(s, errp);
     if (fd < 0) {
-        error_report_err(local_err);
-        return false;
+        return fd;
     }
 
     nr_objs = count_data_objs(inode);
@@ -2446,15 +2443,15 @@ static bool remove_objects(BDRVSheepdogState *s)
                                     data_vdi_id[start_idx]),
                            false, s->cache_flags);
         if (ret < 0) {
-            error_report("failed to discard snapshot inode.");
-            result = false;
+            error_setg(errp, "Failed to discard snapshot inode");
             goto out;
         }
     }
 
+    ret = 0;
 out:
     closesocket(fd);
-    return result;
+    return ret;
 }
 
 static int sd_snapshot_delete(BlockDriverState *bs,
@@ -2464,7 +2461,6 @@ static int sd_snapshot_delete(BlockDriverState *bs,
 {
     unsigned long snap_id = 0;
     char snap_tag[SD_MAX_VDI_TAG_LEN];
-    Error *local_err = NULL;
     int fd, ret;
     char buf[SD_MAX_VDI_LEN + SD_MAX_VDI_TAG_LEN];
     BDRVSheepdogState *s = bs->opaque;
@@ -2477,8 +2473,9 @@ static int sd_snapshot_delete(BlockDriverState *bs,
     };
     SheepdogVdiRsp *rsp = (SheepdogVdiRsp *)&hdr;
 
-    if (!remove_objects(s)) {
-        return -1;
+    ret = remove_objects(s, errp);
+    if (ret) {
+        return ret;
     }
 
     memset(buf, 0, sizeof(buf));
@@ -2498,36 +2495,36 @@ static int sd_snapshot_delete(BlockDriverState *bs,
         pstrcpy(buf + SD_MAX_VDI_LEN, SD_MAX_VDI_TAG_LEN, snap_tag);
     }
 
-    ret = find_vdi_name(s, s->name, snap_id, snap_tag, &vid, true,
-                        &local_err);
+    ret = find_vdi_name(s, s->name, snap_id, snap_tag, &vid, true, errp);
     if (ret) {
         return ret;
     }
 
-    fd = connect_to_sdog(s, &local_err);
+    fd = connect_to_sdog(s, errp);
     if (fd < 0) {
-        error_report_err(local_err);
-        return -1;
+        return fd;
     }
 
     ret = do_req(fd, s->bs, (SheepdogReq *)&hdr,
                  buf, &wlen, &rlen);
     closesocket(fd);
     if (ret) {
+        error_setg_errno(errp, -ret, "Couldn't send request to server");
         return ret;
     }
 
     switch (rsp->result) {
     case SD_RES_NO_VDI:
-        error_report("%s was already deleted", s->name);
+        error_setg(errp, "Can't find the snapshot");
+        return -ENOENT;
     case SD_RES_SUCCESS:
         break;
     default:
-        error_report("%s, %s", sd_strerror(rsp->result), s->name);
-        return -1;
+        error_setg(errp, "%s", sd_strerror(rsp->result));
+        return -EIO;
     }
 
-    return ret;
+    return 0;
 }
 
 static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
-- 
1.8.3.1

[Qemu-devel] [PULL 13/27] sheepdog: Fix error handling sd_create()

[Kevin Wolf]

From: Markus Armbruster <armbru@redhat.com>

As a bdrv_create() method, sd_create() must set an error and return
negative errno on failure.  It prints the error instead of setting it
when connect_to_sdog() fails.  Fix that.

While there, return the value of connect_to_sdog() like we do
elsewhere, instead of -EIO.  No functional change, as
connect_to_sdog() returns no other error code.

Many more suspicious uses of error_report() and error_report_err()
remain in other functions.  Left for another day.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/sheepdog.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 0a0803e..be3db1f 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1829,14 +1829,12 @@ static int sd_create(const char *filename, QemuOpts *opts,
     if (s->inode.block_size_shift == 0) {
         SheepdogVdiReq hdr;
         SheepdogClusterRsp *rsp = (SheepdogClusterRsp *)&hdr;
-        Error *local_err = NULL;
         int fd;
         unsigned int wlen = 0, rlen = 0;
 
-        fd = connect_to_sdog(s, &local_err);
+        fd = connect_to_sdog(s, errp);
         if (fd < 0) {
-            error_report_err(local_err);
-            ret = -EIO;
+            ret = fd;
             goto out;
         }
 
-- 
1.8.3.1

[Qemu-devel] [PULL 14/27] sheepdog: Mark sd_snapshot_delete() lossage FIXME

[Kevin Wolf]

From: Markus Armbruster <armbru@redhat.com>

sd_snapshot_delete() should delete the snapshot whose ID matches
@snapshot_id and whose name matches @name.  But that's not what it
does.  If @snapshot_id is a valid ID, it deletes the snapshot with
that ID, else it deletes the snapshot with that name.  It doesn't use
@name at all.  Add suitable FIXME comments, so someone who actually
knows Sheepdog can fix it.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/sheepdog.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index be3db1f..187bcd8 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -2457,6 +2457,10 @@ static int sd_snapshot_delete(BlockDriverState *bs,
                               const char *name,
                               Error **errp)
 {
+    /*
+     * FIXME should delete the snapshot matching both @snapshot_id and
+     * @name, but @name not used here
+     */
     unsigned long snap_id = 0;
     char snap_tag[SD_MAX_VDI_TAG_LEN];
     int fd, ret;
@@ -2481,6 +2485,11 @@ static int sd_snapshot_delete(BlockDriverState *bs,
     pstrcpy(buf, SD_MAX_VDI_LEN, s->name);
     ret = qemu_strtoul(snapshot_id, NULL, 10, &snap_id);
     if (ret || snap_id > UINT32_MAX) {
+        /*
+         * FIXME Since qemu_strtoul() returns -EINVAL when
+         * @snapshot_id is null, @snapshot_id is mandatory.  Correct
+         * would be to require at least one of @snapshot_id and @name.
+         */
         error_setg(errp, "Invalid snapshot ID: %s",
                          snapshot_id ? snapshot_id : "<null>");
         return -EINVAL;
@@ -2489,6 +2498,7 @@ static int sd_snapshot_delete(BlockDriverState *bs,
     if (snap_id) {
         hdr.snapid = (uint32_t) snap_id;
     } else {
+        /* FIXME I suspect we should use @name here */
         pstrcpy(snap_tag, sizeof(snap_tag), snapshot_id);
         pstrcpy(buf + SD_MAX_VDI_LEN, SD_MAX_VDI_TAG_LEN, snap_tag);
     }
-- 
1.8.3.1

[Qemu-devel] [PULL 15/27] sheepdog: Fix snapshot ID parsing in _open(), _create, _goto()

[Kevin Wolf]

From: Markus Armbruster <armbru@redhat.com>

sd_parse_uri() and sd_snapshot_goto() screw up error checking after
strtoul(), and truncate long tag names silently.  Fix by replacing
those parts by new sd_parse_snapid_or_tag(), which checks more
carefully.

sd_snapshot_delete() also parses snapshot IDs, but is currently too
broken for me to touch.  Mark TODO.

Two calls of strtol() without error checking remain in
parse_redundancy().  Mark them FIXME.

More silent truncation of configuration strings remains elsewhere.
Not marked.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/sheepdog.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 55 insertions(+), 11 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 187bcd8..d3d3731 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -914,6 +914,49 @@ static int get_sheep_fd(BDRVSheepdogState *s, Error **errp)
     return fd;
 }
 
+/*
+ * Parse numeric snapshot ID in @str
+ * If @str can't be parsed as number, return false.
+ * Else, if the number is zero or too large, set *@snapid to zero and
+ * return true.
+ * Else, set *@snapid to the number and return true.
+ */
+static bool sd_parse_snapid(const char *str, uint32_t *snapid)
+{
+    unsigned long ul;
+    int ret;
+
+    ret = qemu_strtoul(str, NULL, 10, &ul);
+    if (ret == -ERANGE) {
+        ul = ret = 0;
+    }
+    if (ret) {
+        return false;
+    }
+    if (ul > UINT32_MAX) {
+        ul = 0;
+    }
+
+    *snapid = ul;
+    return true;
+}
+
+static bool sd_parse_snapid_or_tag(const char *str,
+                                   uint32_t *snapid, char tag[])
+{
+    if (!sd_parse_snapid(str, snapid)) {
+        *snapid = 0;
+        if (g_strlcpy(tag, str, SD_MAX_VDI_TAG_LEN) >= SD_MAX_VDI_TAG_LEN) {
+            return false;
+        }
+    } else if (!*snapid) {
+        return false;
+    } else {
+        tag[0] = 0;
+    }
+    return true;
+}
+
 static int sd_parse_uri(BDRVSheepdogState *s, const char *filename,
                         char *vdi, uint32_t *snapid, char *tag)
 {
@@ -965,9 +1008,9 @@ static int sd_parse_uri(BDRVSheepdogState *s, const char *filename,
 
     /* snapshot tag */
     if (uri->fragment) {
-        *snapid = strtoul(uri->fragment, NULL, 10);
-        if (*snapid == 0) {
-            pstrcpy(tag, SD_MAX_VDI_TAG_LEN, uri->fragment);
+        if (!sd_parse_snapid_or_tag(uri->fragment, snapid, tag)) {
+            ret = -EINVAL;
+            goto out;
         }
     } else {
         *snapid = CURRENT_VDI_ID; /* search current vdi */
@@ -1685,6 +1728,7 @@ static int parse_redundancy(BDRVSheepdogState *s, const char *opt)
     }
 
     copy = strtol(n1, NULL, 10);
+    /* FIXME fix error checking by switching to qemu_strtol() */
     if (copy > SD_MAX_COPIES || copy < 1) {
         return -EINVAL;
     }
@@ -1699,6 +1743,7 @@ static int parse_redundancy(BDRVSheepdogState *s, const char *opt)
     }
 
     parity = strtol(n2, NULL, 10);
+    /* FIXME fix error checking by switching to qemu_strtol() */
     if (parity >= SD_EC_MAX_STRIP || parity < 1) {
         return -EINVAL;
     }
@@ -2365,19 +2410,16 @@ static int sd_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
     BDRVSheepdogState *old_s;
     char tag[SD_MAX_VDI_TAG_LEN];
     uint32_t snapid = 0;
-    int ret = 0;
+    int ret;
+
+    if (!sd_parse_snapid_or_tag(snapshot_id, &snapid, tag)) {
+        return -EINVAL;
+    }
 
     old_s = g_new(BDRVSheepdogState, 1);
 
     memcpy(old_s, s, sizeof(BDRVSheepdogState));
 
-    snapid = strtoul(snapshot_id, NULL, 10);
-    if (snapid) {
-        tag[0] = 0;
-    } else {
-        pstrcpy(tag, sizeof(tag), snapshot_id);
-    }
-
     ret = reload_inode(s, snapid, tag);
     if (ret) {
         goto out;
@@ -2483,6 +2525,7 @@ static int sd_snapshot_delete(BlockDriverState *bs,
     memset(buf, 0, sizeof(buf));
     memset(snap_tag, 0, sizeof(snap_tag));
     pstrcpy(buf, SD_MAX_VDI_LEN, s->name);
+    /* TODO Use sd_parse_snapid() once this mess is cleaned up */
     ret = qemu_strtoul(snapshot_id, NULL, 10, &snap_id);
     if (ret || snap_id > UINT32_MAX) {
         /*
@@ -2499,6 +2542,7 @@ static int sd_snapshot_delete(BlockDriverState *bs,
         hdr.snapid = (uint32_t) snap_id;
     } else {
         /* FIXME I suspect we should use @name here */
+        /* FIXME don't truncate silently */
         pstrcpy(snap_tag, sizeof(snap_tag), snapshot_id);
         pstrcpy(buf + SD_MAX_VDI_LEN, SD_MAX_VDI_TAG_LEN, snap_tag);
     }
-- 
1.8.3.1

[Qemu-devel] [PULL 16/27] sheepdog: Don't truncate long VDI name in _open(), _create()

[Kevin Wolf]

From: Markus Armbruster <armbru@redhat.com>

sd_parse_uri() truncates long VDI names silently.  Reject them
instead.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/sheepdog.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index d3d3731..fed7264 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -985,7 +985,10 @@ static int sd_parse_uri(BDRVSheepdogState *s, const char *filename,
         ret = -EINVAL;
         goto out;
     }
-    pstrcpy(vdi, SD_MAX_VDI_LEN, uri->path + 1);
+    if (g_strlcpy(vdi, uri->path + 1, SD_MAX_VDI_LEN) >= SD_MAX_VDI_LEN) {
+        ret = -EINVAL;
+        goto out;
+    }
 
     qp = query_params_parse(uri->query);
     if (qp->n > 1 || (s->is_unix && !qp->n) || (!s->is_unix && qp->n)) {
-- 
1.8.3.1

[Qemu-devel] [PULL 17/27] sheepdog: Report errors in pseudo-filename more usefully

[Kevin Wolf]

From: Markus Armbruster <armbru@redhat.com>

Errors in the pseudo-filename are all reported with the same laconic
"Can't parse filename" message.

Add real error reporting, such as:

    $ qemu-system-x86_64 --drive driver=sheepdog,filename=sheepdog:///
    qemu-system-x86_64: --drive driver=sheepdog,filename=sheepdog:///: missing file path in URI
    $ qemu-system-x86_64 --drive driver=sheepdog,filename=sheepgod:///vdi
    qemu-system-x86_64: --drive driver=sheepdog,filename=sheepgod:///vdi: URI scheme must be 'sheepdog', 'sheepdog+tcp', or 'sheepdog+unix'
    $ qemu-system-x86_64 --drive driver=sheepdog,filename=sheepdog+unix:///vdi?socke=sheepdog.sock
    qemu-system-x86_64: --drive driver=sheepdog,filename=sheepdog+unix:///vdi?socke=sheepdog.sock: unexpected query parameters

The code to translate legacy syntax to URI fails to escape URI
meta-characters.  The new error messages are misleading then.  Replace
them by the old "Can't parse filename" message.  "Internal error"
would be more honest.  Anyway, no worse than before.  Also add a FIXME
comment.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/sheepdog.c | 88 +++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 59 insertions(+), 29 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index fed7264..161932d 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -957,16 +957,18 @@ static bool sd_parse_snapid_or_tag(const char *str,
     return true;
 }
 
-static int sd_parse_uri(BDRVSheepdogState *s, const char *filename,
-                        char *vdi, uint32_t *snapid, char *tag)
+static void sd_parse_uri(BDRVSheepdogState *s, const char *filename,
+                         char *vdi, uint32_t *snapid, char *tag,
+                         Error **errp)
 {
+    Error *err = NULL;
     URI *uri;
     QueryParams *qp = NULL;
-    int ret = 0;
 
     uri = uri_parse(filename);
     if (!uri) {
-        return -EINVAL;
+        error_setg(&err, "invalid URI");
+        goto out;
     }
 
     /* transport */
@@ -977,34 +979,46 @@ static int sd_parse_uri(BDRVSheepdogState *s, const char *filename,
     } else if (!strcmp(uri->scheme, "sheepdog+unix")) {
         s->is_unix = true;
     } else {
-        ret = -EINVAL;
+        error_setg(&err, "URI scheme must be 'sheepdog', 'sheepdog+tcp',"
+                   " or 'sheepdog+unix'");
         goto out;
     }
 
     if (uri->path == NULL || !strcmp(uri->path, "/")) {
-        ret = -EINVAL;
+        error_setg(&err, "missing file path in URI");
         goto out;
     }
     if (g_strlcpy(vdi, uri->path + 1, SD_MAX_VDI_LEN) >= SD_MAX_VDI_LEN) {
-        ret = -EINVAL;
+        error_setg(&err, "VDI name is too long");
         goto out;
     }
 
     qp = query_params_parse(uri->query);
-    if (qp->n > 1 || (s->is_unix && !qp->n) || (!s->is_unix && qp->n)) {
-        ret = -EINVAL;
-        goto out;
-    }
 
     if (s->is_unix) {
         /* sheepdog+unix:///vdiname?socket=path */
-        if (uri->server || uri->port || strcmp(qp->p[0].name, "socket")) {
-            ret = -EINVAL;
+        if (uri->server || uri->port) {
+            error_setg(&err, "URI scheme %s doesn't accept a server address",
+                       uri->scheme);
+            goto out;
+        }
+        if (!qp->n) {
+            error_setg(&err,
+                       "URI scheme %s requires query parameter 'socket'",
+                       uri->scheme);
+            goto out;
+        }
+        if (qp->n != 1 || strcmp(qp->p[0].name, "socket")) {
+            error_setg(&err, "unexpected query parameters");
             goto out;
         }
         s->host_spec = g_strdup(qp->p[0].value);
     } else {
         /* sheepdog[+tcp]://[host:port]/vdiname */
+        if (qp->n) {
+            error_setg(&err, "unexpected query parameters");
+            goto out;
+        }
         s->host_spec = g_strdup_printf("%s:%d", uri->server ?: SD_DEFAULT_ADDR,
                                        uri->port ?: SD_DEFAULT_PORT);
     }
@@ -1012,7 +1026,8 @@ static int sd_parse_uri(BDRVSheepdogState *s, const char *filename,
     /* snapshot tag */
     if (uri->fragment) {
         if (!sd_parse_snapid_or_tag(uri->fragment, snapid, tag)) {
-            ret = -EINVAL;
+            error_setg(&err, "'%s' is not a valid snapshot ID",
+                       uri->fragment);
             goto out;
         }
     } else {
@@ -1020,11 +1035,11 @@ static int sd_parse_uri(BDRVSheepdogState *s, const char *filename,
     }
 
 out:
+    error_propagate(errp, err);
     if (qp) {
         query_params_free(qp);
     }
     uri_free(uri);
-    return ret;
 }
 
 /*
@@ -1044,12 +1059,14 @@ out:
  * You can run VMs outside the Sheepdog cluster by specifying
  * `hostname' and `port' (experimental).
  */
-static int parse_vdiname(BDRVSheepdogState *s, const char *filename,
-                         char *vdi, uint32_t *snapid, char *tag)
+static void parse_vdiname(BDRVSheepdogState *s, const char *filename,
+                          char *vdi, uint32_t *snapid, char *tag,
+                          Error **errp)
 {
+    Error *err = NULL;
     char *p, *q, *uri;
     const char *host_spec, *vdi_spec;
-    int nr_sep, ret;
+    int nr_sep;
 
     strstart(filename, "sheepdog:", &filename);
     p = q = g_strdup(filename);
@@ -1084,12 +1101,23 @@ static int parse_vdiname(BDRVSheepdogState *s, const char *filename,
 
     uri = g_strdup_printf("sheepdog://%s/%s", host_spec, vdi_spec);
 
-    ret = sd_parse_uri(s, uri, vdi, snapid, tag);
+    /*
+     * FIXME We to escape URI meta-characters, e.g. "x?y=z"
+     * produces "sheepdog://x?y=z".  Because of that ...
+     */
+    sd_parse_uri(s, uri, vdi, snapid, tag, &err);
+    if (err) {
+        /*
+         * ... this can fail, but the error message is misleading.
+         * Replace it by the traditional useless one until the
+         * escaping is fixed.
+         */
+        error_free(err);
+        error_setg(errp, "Can't parse filename");
+    }
 
     g_free(q);
     g_free(uri);
-
-    return ret;
 }
 
 static int find_vdi_name(BDRVSheepdogState *s, const char *filename,
@@ -1452,12 +1480,13 @@ static int sd_open(BlockDriverState *bs, QDict *options, int flags,
     memset(tag, 0, sizeof(tag));
 
     if (strstr(filename, "://")) {
-        ret = sd_parse_uri(s, filename, vdi, &snapid, tag);
+        sd_parse_uri(s, filename, vdi, &snapid, tag, &local_err);
     } else {
-        ret = parse_vdiname(s, filename, vdi, &snapid, tag);
+        parse_vdiname(s, filename, vdi, &snapid, tag, &local_err);
     }
-    if (ret < 0) {
-        error_setg(errp, "Can't parse filename");
+    if (local_err) {
+        error_propagate(errp, local_err);
+        ret = -EINVAL;
         goto err_no_fd;
     }
     s->fd = get_sheep_fd(s, errp);
@@ -1785,6 +1814,7 @@ static int parse_block_size_shift(BDRVSheepdogState *s, QemuOpts *opt)
 static int sd_create(const char *filename, QemuOpts *opts,
                      Error **errp)
 {
+    Error *err = NULL;
     int ret = 0;
     uint32_t vid = 0;
     char *backing_file = NULL;
@@ -1799,12 +1829,12 @@ static int sd_create(const char *filename, QemuOpts *opts,
 
     memset(tag, 0, sizeof(tag));
     if (strstr(filename, "://")) {
-        ret = sd_parse_uri(s, filename, s->name, &snapid, tag);
+        sd_parse_uri(s, filename, s->name, &snapid, tag, &err);
     } else {
-        ret = parse_vdiname(s, filename, s->name, &snapid, tag);
+        parse_vdiname(s, filename, s->name, &snapid, tag, &err);
     }
-    if (ret < 0) {
-        error_setg(errp, "Can't parse filename");
+    if (err) {
+        error_propagate(errp, err);
         goto out;
     }
 
-- 
1.8.3.1

[Qemu-devel] [PULL 18/27] sheepdog: Use SocketAddress and socket_connect()

[Kevin Wolf]

From: Markus Armbruster <armbru@redhat.com>

sd_parse_uri() builds a string from host and port parts for
inet_connect().  inet_connect() parses it into host, port and options.
Whether this gets exactly the same host, port and no options for all
inputs is not obvious.

Cut out the string middleman and build a SocketAddress for
socket_connect() instead.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/sheepdog.c | 53 ++++++++++++++++++++++++++++++-----------------------
 1 file changed, 30 insertions(+), 23 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 161932d..9b1e121 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -374,8 +374,7 @@ struct BDRVSheepdogState {
     uint32_t cache_flags;
     bool discard_supported;
 
-    char *host_spec;
-    bool is_unix;
+    SocketAddress *addr;
     int fd;
 
     CoMutex lock;
@@ -532,16 +531,12 @@ static int connect_to_sdog(BDRVSheepdogState *s, Error **errp)
 {
     int fd;
 
-    if (s->is_unix) {
-        fd = unix_connect(s->host_spec, errp);
-    } else {
-        fd = inet_connect(s->host_spec, errp);
+    fd = socket_connect(s->addr, errp, NULL, NULL);
 
-        if (fd >= 0) {
-            int ret = socket_set_nodelay(fd);
-            if (ret < 0) {
-                error_report("%s", strerror(errno));
-            }
+    if (s->addr->type == SOCKET_ADDRESS_KIND_INET && fd >= 0) {
+        int ret = socket_set_nodelay(fd);
+        if (ret < 0) {
+            error_report("%s", strerror(errno));
         }
     }
 
@@ -820,8 +815,7 @@ static void coroutine_fn aio_read_response(void *opaque)
     case AIOCB_DISCARD_OBJ:
         switch (rsp.result) {
         case SD_RES_INVALID_PARMS:
-            error_report("sheep(%s) doesn't support discard command",
-                         s->host_spec);
+            error_report("server doesn't support discard command");
             rsp.result = SD_RES_SUCCESS;
             s->discard_supported = false;
             break;
@@ -962,8 +956,10 @@ static void sd_parse_uri(BDRVSheepdogState *s, const char *filename,
                          Error **errp)
 {
     Error *err = NULL;
-    URI *uri;
     QueryParams *qp = NULL;
+    SocketAddress *addr = NULL;
+    bool is_unix;
+    URI *uri;
 
     uri = uri_parse(filename);
     if (!uri) {
@@ -973,11 +969,11 @@ static void sd_parse_uri(BDRVSheepdogState *s, const char *filename,
 
     /* transport */
     if (!strcmp(uri->scheme, "sheepdog")) {
-        s->is_unix = false;
+        is_unix = false;
     } else if (!strcmp(uri->scheme, "sheepdog+tcp")) {
-        s->is_unix = false;
+        is_unix = false;
     } else if (!strcmp(uri->scheme, "sheepdog+unix")) {
-        s->is_unix = true;
+        is_unix = true;
     } else {
         error_setg(&err, "URI scheme must be 'sheepdog', 'sheepdog+tcp',"
                    " or 'sheepdog+unix'");
@@ -994,8 +990,9 @@ static void sd_parse_uri(BDRVSheepdogState *s, const char *filename,
     }
 
     qp = query_params_parse(uri->query);
+    addr = g_new0(SocketAddress, 1);
 
-    if (s->is_unix) {
+    if (is_unix) {
         /* sheepdog+unix:///vdiname?socket=path */
         if (uri->server || uri->port) {
             error_setg(&err, "URI scheme %s doesn't accept a server address",
@@ -1012,15 +1009,20 @@ static void sd_parse_uri(BDRVSheepdogState *s, const char *filename,
             error_setg(&err, "unexpected query parameters");
             goto out;
         }
-        s->host_spec = g_strdup(qp->p[0].value);
+        addr->type = SOCKET_ADDRESS_KIND_UNIX;
+        addr->u.q_unix.data = g_new0(UnixSocketAddress, 1);
+        addr->u.q_unix.data->path = g_strdup(qp->p[0].value);
     } else {
         /* sheepdog[+tcp]://[host:port]/vdiname */
         if (qp->n) {
             error_setg(&err, "unexpected query parameters");
             goto out;
         }
-        s->host_spec = g_strdup_printf("%s:%d", uri->server ?: SD_DEFAULT_ADDR,
-                                       uri->port ?: SD_DEFAULT_PORT);
+        addr->type = SOCKET_ADDRESS_KIND_INET;
+        addr->u.inet.data = g_new0(InetSocketAddress, 1);
+        addr->u.inet.data->host = g_strdup(uri->server ?: SD_DEFAULT_ADDR);
+        addr->u.inet.data->port = g_strdup_printf("%d",
+                                        uri->port ?: SD_DEFAULT_PORT);
     }
 
     /* snapshot tag */
@@ -1035,7 +1037,12 @@ static void sd_parse_uri(BDRVSheepdogState *s, const char *filename,
     }
 
 out:
-    error_propagate(errp, err);
+    if (err) {
+        error_propagate(errp, err);
+        qapi_free_SocketAddress(addr);
+    } else {
+        s->addr = addr;
+    }
     if (qp) {
         query_params_free(qp);
     }
@@ -1998,7 +2005,7 @@ static void sd_close(BlockDriverState *bs)
     aio_set_fd_handler(bdrv_get_aio_context(bs), s->fd,
                        false, NULL, NULL, NULL, NULL);
     closesocket(s->fd);
-    g_free(s->host_spec);
+    qapi_free_SocketAddress(s->addr);
 }
 
 static int64_t sd_getlength(BlockDriverState *bs)
-- 
1.8.3.1

[Qemu-devel] [PULL 19/27] sheepdog: Implement bdrv_parse_filename()

[Kevin Wolf]

From: Markus Armbruster <armbru@redhat.com>

This permits configuration with driver-specific options in addition to
pseudo-filename parsed as URI.  For instance,

    --drive driver=sheepdog,host=fido,vdi=dolly

instead of

    --drive driver=sheepdog,file=sheepdog://fido/dolly

It's also a first step towards supporting blockdev-add.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/sheepdog.c | 230 +++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 174 insertions(+), 56 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 9b1e121..89e98ed 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -14,6 +14,8 @@
 
 #include "qemu/osdep.h"
 #include "qapi/error.h"
+#include "qapi/qmp/qdict.h"
+#include "qapi/qmp/qint.h"
 #include "qemu/uri.h"
 #include "qemu/error-report.h"
 #include "qemu/sockets.h"
@@ -526,6 +528,25 @@ static void sd_aio_setup(SheepdogAIOCB *acb, BDRVSheepdogState *s,
     QLIST_INSERT_HEAD(&s->inflight_aiocb_head, acb, aiocb_siblings);
 }
 
+static SocketAddress *sd_socket_address(const char *path,
+                                        const char *host, const char *port)
+{
+    SocketAddress *addr = g_new0(SocketAddress, 1);
+
+    if (path) {
+        addr->type = SOCKET_ADDRESS_KIND_UNIX;
+        addr->u.q_unix.data = g_new0(UnixSocketAddress, 1);
+        addr->u.q_unix.data->path = g_strdup(path);
+    } else {
+        addr->type = SOCKET_ADDRESS_KIND_INET;
+        addr->u.inet.data = g_new0(InetSocketAddress, 1);
+        addr->u.inet.data->host = g_strdup(host ?: SD_DEFAULT_ADDR);
+        addr->u.inet.data->port = g_strdup(port ?: stringify(SD_DEFAULT_PORT));
+    }
+
+    return addr;
+}
+
 /* Return -EIO in case of error, file descriptor on success */
 static int connect_to_sdog(BDRVSheepdogState *s, Error **errp)
 {
@@ -951,17 +972,37 @@ static bool sd_parse_snapid_or_tag(const char *str,
     return true;
 }
 
-static void sd_parse_uri(BDRVSheepdogState *s, const char *filename,
-                         char *vdi, uint32_t *snapid, char *tag,
+typedef struct {
+    const char *path;           /* non-null iff transport is tcp */
+    const char *host;           /* valid when transport is tcp */
+    int port;                   /* valid when transport is tcp */
+    char vdi[SD_MAX_VDI_LEN];
+    char tag[SD_MAX_VDI_TAG_LEN];
+    uint32_t snap_id;
+    /* Remainder is only for sd_config_done() */
+    URI *uri;
+    QueryParams *qp;
+} SheepdogConfig;
+
+static void sd_config_done(SheepdogConfig *cfg)
+{
+    if (cfg->qp) {
+        query_params_free(cfg->qp);
+    }
+    uri_free(cfg->uri);
+}
+
+static void sd_parse_uri(SheepdogConfig *cfg, const char *filename,
                          Error **errp)
 {
     Error *err = NULL;
     QueryParams *qp = NULL;
-    SocketAddress *addr = NULL;
     bool is_unix;
     URI *uri;
 
-    uri = uri_parse(filename);
+    memset(cfg, 0, sizeof(*cfg));
+
+    cfg->uri = uri = uri_parse(filename);
     if (!uri) {
         error_setg(&err, "invalid URI");
         goto out;
@@ -984,13 +1025,13 @@ static void sd_parse_uri(BDRVSheepdogState *s, const char *filename,
         error_setg(&err, "missing file path in URI");
         goto out;
     }
-    if (g_strlcpy(vdi, uri->path + 1, SD_MAX_VDI_LEN) >= SD_MAX_VDI_LEN) {
+    if (g_strlcpy(cfg->vdi, uri->path + 1, SD_MAX_VDI_LEN)
+        >= SD_MAX_VDI_LEN) {
         error_setg(&err, "VDI name is too long");
         goto out;
     }
 
-    qp = query_params_parse(uri->query);
-    addr = g_new0(SocketAddress, 1);
+    cfg->qp = qp = query_params_parse(uri->query);
 
     if (is_unix) {
         /* sheepdog+unix:///vdiname?socket=path */
@@ -1009,44 +1050,34 @@ static void sd_parse_uri(BDRVSheepdogState *s, const char *filename,
             error_setg(&err, "unexpected query parameters");
             goto out;
         }
-        addr->type = SOCKET_ADDRESS_KIND_UNIX;
-        addr->u.q_unix.data = g_new0(UnixSocketAddress, 1);
-        addr->u.q_unix.data->path = g_strdup(qp->p[0].value);
+        cfg->path = qp->p[0].value;
     } else {
         /* sheepdog[+tcp]://[host:port]/vdiname */
         if (qp->n) {
             error_setg(&err, "unexpected query parameters");
             goto out;
         }
-        addr->type = SOCKET_ADDRESS_KIND_INET;
-        addr->u.inet.data = g_new0(InetSocketAddress, 1);
-        addr->u.inet.data->host = g_strdup(uri->server ?: SD_DEFAULT_ADDR);
-        addr->u.inet.data->port = g_strdup_printf("%d",
-                                        uri->port ?: SD_DEFAULT_PORT);
+        cfg->host = uri->server;
+        cfg->port = uri->port;
     }
 
     /* snapshot tag */
     if (uri->fragment) {
-        if (!sd_parse_snapid_or_tag(uri->fragment, snapid, tag)) {
+        if (!sd_parse_snapid_or_tag(uri->fragment,
+                                    &cfg->snap_id, cfg->tag)) {
             error_setg(&err, "'%s' is not a valid snapshot ID",
                        uri->fragment);
             goto out;
         }
     } else {
-        *snapid = CURRENT_VDI_ID; /* search current vdi */
+        cfg->snap_id = CURRENT_VDI_ID; /* search current vdi */
     }
 
 out:
     if (err) {
         error_propagate(errp, err);
-        qapi_free_SocketAddress(addr);
-    } else {
-        s->addr = addr;
-    }
-    if (qp) {
-        query_params_free(qp);
+        sd_config_done(cfg);
     }
-    uri_free(uri);
 }
 
 /*
@@ -1066,8 +1097,7 @@ out:
  * You can run VMs outside the Sheepdog cluster by specifying
  * `hostname' and `port' (experimental).
  */
-static void parse_vdiname(BDRVSheepdogState *s, const char *filename,
-                          char *vdi, uint32_t *snapid, char *tag,
+static void parse_vdiname(SheepdogConfig *cfg, const char *filename,
                           Error **errp)
 {
     Error *err = NULL;
@@ -1112,7 +1142,7 @@ static void parse_vdiname(BDRVSheepdogState *s, const char *filename,
      * FIXME We to escape URI meta-characters, e.g. "x?y=z"
      * produces "sheepdog://x?y=z".  Because of that ...
      */
-    sd_parse_uri(s, uri, vdi, snapid, tag, &err);
+    sd_parse_uri(cfg, uri, &err);
     if (err) {
         /*
          * ... this can fail, but the error message is misleading.
@@ -1127,6 +1157,43 @@ static void parse_vdiname(BDRVSheepdogState *s, const char *filename,
     g_free(uri);
 }
 
+static void sd_parse_filename(const char *filename, QDict *options,
+                              Error **errp)
+{
+    Error *err = NULL;
+    SheepdogConfig cfg;
+    char buf[32];
+
+    if (strstr(filename, "://")) {
+        sd_parse_uri(&cfg, filename, &err);
+    } else {
+        parse_vdiname(&cfg, filename, &err);
+    }
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+
+    if (cfg.host) {
+        qdict_set_default_str(options, "host", cfg.host);
+    }
+    if (cfg.port) {
+        snprintf(buf, sizeof(buf), "%d", cfg.port);
+        qdict_set_default_str(options, "port", buf);
+    }
+    if (cfg.path) {
+        qdict_set_default_str(options, "path", cfg.path);
+    }
+    qdict_set_default_str(options, "vdi", cfg.vdi);
+    qdict_set_default_str(options, "tag", cfg.tag);
+    if (cfg.snap_id) {
+        snprintf(buf, sizeof(buf), "%d", cfg.snap_id);
+        qdict_set_default_str(options, "snap-id", buf);
+    }
+
+    sd_config_done(&cfg);
+}
+
 static int find_vdi_name(BDRVSheepdogState *s, const char *filename,
                          uint32_t snapid, const char *tag, uint32_t *vid,
                          bool lock, Error **errp)
@@ -1438,15 +1505,33 @@ static void sd_attach_aio_context(BlockDriverState *bs,
                        co_read_response, NULL, NULL, s);
 }
 
-/* TODO Convert to fine grained options */
 static QemuOptsList runtime_opts = {
     .name = "sheepdog",
     .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
     .desc = {
         {
-            .name = "filename",
+            .name = "host",
+            .type = QEMU_OPT_STRING,
+        },
+        {
+            .name = "port",
+            .type = QEMU_OPT_STRING,
+        },
+        {
+            .name = "path",
+            .type = QEMU_OPT_STRING,
+        },
+        {
+            .name = "vdi",
+            .type = QEMU_OPT_STRING,
+        },
+        {
+            .name = "snap-id",
+            .type = QEMU_OPT_NUMBER,
+        },
+        {
+            .name = "tag",
             .type = QEMU_OPT_STRING,
-            .help = "URL to the sheepdog image",
         },
         { /* end of list */ }
     },
@@ -1458,12 +1543,11 @@ static int sd_open(BlockDriverState *bs, QDict *options, int flags,
     int ret, fd;
     uint32_t vid = 0;
     BDRVSheepdogState *s = bs->opaque;
-    char vdi[SD_MAX_VDI_LEN], tag[SD_MAX_VDI_TAG_LEN];
-    uint32_t snapid;
+    const char *host, *port, *path, *vdi, *snap_id_str, *tag;
+    uint64_t snap_id;
     char *buf = NULL;
     QemuOpts *opts;
     Error *local_err = NULL;
-    const char *filename;
 
     s->bs = bs;
     s->aio_context = bdrv_get_aio_context(bs);
@@ -1476,33 +1560,63 @@ static int sd_open(BlockDriverState *bs, QDict *options, int flags,
         goto err_no_fd;
     }
 
-    filename = qemu_opt_get(opts, "filename");
+    host = qemu_opt_get(opts, "host");
+    port = qemu_opt_get(opts, "port");
+    path = qemu_opt_get(opts, "path");
+    vdi = qemu_opt_get(opts, "vdi");
+    snap_id_str = qemu_opt_get(opts, "snap-id");
+    snap_id = qemu_opt_get_number(opts, "snap-id", CURRENT_VDI_ID);
+    tag = qemu_opt_get(opts, "tag");
 
-    QLIST_INIT(&s->inflight_aio_head);
-    QLIST_INIT(&s->failed_aio_head);
-    QLIST_INIT(&s->inflight_aiocb_head);
-    s->fd = -1;
+    if ((host || port) && path) {
+        error_setg(errp, "can't use 'path' together with 'host' or 'port'");
+        ret = -EINVAL;
+        goto err_no_fd;
+    }
+
+    if (!vdi) {
+        error_setg(errp, "parameter 'vdi' is missing");
+        ret = -EINVAL;
+        goto err_no_fd;
+    }
+    if (strlen(vdi) >= SD_MAX_VDI_LEN) {
+        error_setg(errp, "value of parameter 'vdi' is too long");
+        ret = -EINVAL;
+        goto err_no_fd;
+    }
 
-    memset(vdi, 0, sizeof(vdi));
-    memset(tag, 0, sizeof(tag));
+    if (snap_id > UINT32_MAX) {
+        snap_id = 0;
+    }
+    if (snap_id_str && !snap_id) {
+        error_setg(errp, "'snap-id=%s' is not a valid snapshot ID",
+                   snap_id_str);
+        ret = -EINVAL;
+        goto err_no_fd;
+    }
 
-    if (strstr(filename, "://")) {
-        sd_parse_uri(s, filename, vdi, &snapid, tag, &local_err);
-    } else {
-        parse_vdiname(s, filename, vdi, &snapid, tag, &local_err);
+    if (!tag) {
+        tag = "";
     }
-    if (local_err) {
-        error_propagate(errp, local_err);
+    if (tag && strlen(tag) >= SD_MAX_VDI_TAG_LEN) {
+        error_setg(errp, "value of parameter 'tag' is too long");
         ret = -EINVAL;
         goto err_no_fd;
     }
+
+    s->addr = sd_socket_address(path, host, port);
+
+    QLIST_INIT(&s->inflight_aio_head);
+    QLIST_INIT(&s->failed_aio_head);
+    QLIST_INIT(&s->inflight_aiocb_head);
+
     s->fd = get_sheep_fd(s, errp);
     if (s->fd < 0) {
         ret = s->fd;
         goto err_no_fd;
     }
 
-    ret = find_vdi_name(s, vdi, snapid, tag, &vid, true, errp);
+    ret = find_vdi_name(s, vdi, (uint32_t)snap_id, tag, &vid, true, errp);
     if (ret) {
         goto err;
     }
@@ -1517,7 +1631,7 @@ static int sd_open(BlockDriverState *bs, QDict *options, int flags,
     }
     s->discard_supported = true;
 
-    if (snapid || tag[0] != '\0') {
+    if (snap_id || tag[0]) {
         DPRINTF("%" PRIx32 " snapshot inode was open.\n", vid);
         s->is_snapshot = true;
     }
@@ -1827,24 +1941,28 @@ static int sd_create(const char *filename, QemuOpts *opts,
     char *backing_file = NULL;
     char *buf = NULL;
     BDRVSheepdogState *s;
-    char tag[SD_MAX_VDI_TAG_LEN];
-    uint32_t snapid;
+    SheepdogConfig cfg;
     uint64_t max_vdi_size;
     bool prealloc = false;
 
     s = g_new0(BDRVSheepdogState, 1);
 
-    memset(tag, 0, sizeof(tag));
     if (strstr(filename, "://")) {
-        sd_parse_uri(s, filename, s->name, &snapid, tag, &err);
+        sd_parse_uri(&cfg, filename, &err);
     } else {
-        parse_vdiname(s, filename, s->name, &snapid, tag, &err);
+        parse_vdiname(&cfg, filename, &err);
     }
     if (err) {
         error_propagate(errp, err);
         goto out;
     }
 
+    buf = cfg.port ? g_strdup_printf("%d", cfg.port) : NULL;
+    s->addr = sd_socket_address(cfg.path, cfg.host, buf);
+    g_free(buf);
+    strcpy(s->name, cfg.vdi);
+    sd_config_done(&cfg);
+
     s->inode.vdi_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
                                  BDRV_SECTOR_SIZE);
     backing_file = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE);
@@ -2921,7 +3039,7 @@ static BlockDriver bdrv_sheepdog = {
     .format_name    = "sheepdog",
     .protocol_name  = "sheepdog",
     .instance_size  = sizeof(BDRVSheepdogState),
-    .bdrv_needs_filename = true,
+    .bdrv_parse_filename    = sd_parse_filename,
     .bdrv_file_open = sd_open,
     .bdrv_reopen_prepare    = sd_reopen_prepare,
     .bdrv_reopen_commit     = sd_reopen_commit,
@@ -2957,7 +3075,7 @@ static BlockDriver bdrv_sheepdog_tcp = {
     .format_name    = "sheepdog",
     .protocol_name  = "sheepdog+tcp",
     .instance_size  = sizeof(BDRVSheepdogState),
-    .bdrv_needs_filename = true,
+    .bdrv_parse_filename    = sd_parse_filename,
     .bdrv_file_open = sd_open,
     .bdrv_reopen_prepare    = sd_reopen_prepare,
     .bdrv_reopen_commit     = sd_reopen_commit,
@@ -2993,7 +3111,7 @@ static BlockDriver bdrv_sheepdog_unix = {
     .format_name    = "sheepdog",
     .protocol_name  = "sheepdog+unix",
     .instance_size  = sizeof(BDRVSheepdogState),
-    .bdrv_needs_filename = true,
+    .bdrv_parse_filename    = sd_parse_filename,
     .bdrv_file_open = sd_open,
     .bdrv_reopen_prepare    = sd_reopen_prepare,
     .bdrv_reopen_commit     = sd_reopen_commit,
-- 
1.8.3.1

[Qemu-devel] [PULL 20/27] gluster: Drop assumptions on SocketTransport names

[Kevin Wolf]

From: Markus Armbruster <armbru@redhat.com>

qemu_gluster_glfs_init() passes the names of QAPI enumeration type
SocketTransport to glfs_set_volfile_server().  Works, because they
were chosen to match.  But the coupling is artificial.  Use the
appropriate literal strings instead.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Niels de Vos <ndevos@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/gluster.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/block/gluster.c b/block/gluster.c
index 56b4abe..7236d59 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -412,8 +412,7 @@ static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
 
     for (server = gconf->server; server; server = server->next) {
         if (server->value->type  == GLUSTER_TRANSPORT_UNIX) {
-            ret = glfs_set_volfile_server(glfs,
-                                   GlusterTransport_lookup[server->value->type],
+            ret = glfs_set_volfile_server(glfs, "unix",
                                    server->value->u.q_unix.path, 0);
         } else {
             if (parse_uint_full(server->value->u.tcp.port, &port, 10) < 0 ||
@@ -423,8 +422,7 @@ static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
                 errno = EINVAL;
                 goto out;
             }
-            ret = glfs_set_volfile_server(glfs,
-                                   GlusterTransport_lookup[server->value->type],
+            ret = glfs_set_volfile_server(glfs, "tcp",
                                    server->value->u.tcp.host,
                                    (int)port);
         }
-- 
1.8.3.1

[Qemu-devel] [PULL 21/27] gluster: Don't duplicate qapi-util.c's qapi_enum_parse()

[Kevin Wolf]

From: Markus Armbruster <armbru@redhat.com>

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Niels de Vos <ndevos@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/gluster.c | 30 +++++++++---------------------
 1 file changed, 9 insertions(+), 21 deletions(-)

diff --git a/block/gluster.c b/block/gluster.c
index 7236d59..6fbcf9e 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -12,6 +12,7 @@
 #include "block/block_int.h"
 #include "qapi/error.h"
 #include "qapi/qmp/qerror.h"
+#include "qapi/util.h"
 #include "qemu/uri.h"
 #include "qemu/error-report.h"
 #include "qemu/cutils.h"
@@ -472,23 +473,6 @@ out:
     return NULL;
 }
 
-static int qapi_enum_parse(const char *opt)
-{
-    int i;
-
-    if (!opt) {
-        return GLUSTER_TRANSPORT__MAX;
-    }
-
-    for (i = 0; i < GLUSTER_TRANSPORT__MAX; i++) {
-        if (!strcmp(opt, GlusterTransport_lookup[i])) {
-            return i;
-        }
-    }
-
-    return i;
-}
-
 /*
  * Convert the json formatted command line into qapi.
 */
@@ -546,16 +530,20 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
 
         ptr = qemu_opt_get(opts, GLUSTER_OPT_TYPE);
         gsconf = g_new0(GlusterServer, 1);
-        gsconf->type = qapi_enum_parse(ptr);
+        gsconf->type = qapi_enum_parse(GlusterTransport_lookup, ptr,
+                                       GLUSTER_TRANSPORT__MAX,
+                                       GLUSTER_TRANSPORT__MAX,
+                                       &local_err);
         if (!ptr) {
             error_setg(&local_err, QERR_MISSING_PARAMETER, GLUSTER_OPT_TYPE);
             error_append_hint(&local_err, GERR_INDEX_HINT, i);
             goto out;
 
         }
-        if (gsconf->type == GLUSTER_TRANSPORT__MAX) {
-            error_setg(&local_err, QERR_INVALID_PARAMETER_VALUE,
-                       GLUSTER_OPT_TYPE, "tcp or unix");
+        if (local_err) {
+            error_append_hint(&local_err,
+                              "Parameter '%s' may be 'tcp' or 'unix'\n",
+                              GLUSTER_OPT_TYPE);
             error_append_hint(&local_err, GERR_INDEX_HINT, i);
             goto out;
         }
-- 
1.8.3.1

[Qemu-devel] [PULL 22/27] gluster: Plug memory leaks in qemu_gluster_parse_json()

[Kevin Wolf]

From: Markus Armbruster <armbru@redhat.com>

To reproduce, run

    $ valgrind qemu-system-x86_64 --nodefaults -S --drive driver=gluster,volume=testvol,path=/a/b/c,server.0.type=xxx

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Niels de Vos <ndevos@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/gluster.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/block/gluster.c b/block/gluster.c
index 6fbcf9e..991f18f 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -480,7 +480,7 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
                                   QDict *options, Error **errp)
 {
     QemuOpts *opts;
-    GlusterServer *gsconf;
+    GlusterServer *gsconf = NULL;
     GlusterServerList *curr = NULL;
     QDict *backing_options = NULL;
     Error *local_err = NULL;
@@ -529,17 +529,16 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
         }
 
         ptr = qemu_opt_get(opts, GLUSTER_OPT_TYPE);
-        gsconf = g_new0(GlusterServer, 1);
-        gsconf->type = qapi_enum_parse(GlusterTransport_lookup, ptr,
-                                       GLUSTER_TRANSPORT__MAX,
-                                       GLUSTER_TRANSPORT__MAX,
-                                       &local_err);
         if (!ptr) {
             error_setg(&local_err, QERR_MISSING_PARAMETER, GLUSTER_OPT_TYPE);
             error_append_hint(&local_err, GERR_INDEX_HINT, i);
             goto out;
 
         }
+        gsconf = g_new0(GlusterServer, 1);
+        gsconf->type = qapi_enum_parse(GlusterTransport_lookup, ptr,
+                                       GLUSTER_TRANSPORT__MAX, -1,
+                                       &local_err);
         if (local_err) {
             error_append_hint(&local_err,
                               "Parameter '%s' may be 'tcp' or 'unix'\n",
@@ -626,8 +625,10 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
             curr->next->value = gsconf;
             curr = curr->next;
         }
+        gsconf = NULL;
 
-        qdict_del(backing_options, str);
+        QDECREF(backing_options);
+        backing_options = NULL;
         g_free(str);
         str = NULL;
     }
@@ -636,11 +637,10 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
 
 out:
     error_propagate(errp, local_err);
+    qapi_free_GlusterServer(gsconf);
     qemu_opts_del(opts);
-    if (str) {
-        qdict_del(backing_options, str);
-        g_free(str);
-    }
+    g_free(str);
+    QDECREF(backing_options);
     errno = EINVAL;
     return -errno;
 }
-- 
1.8.3.1

[Qemu-devel] [PULL 23/27] qapi-schema: Rename GlusterServer to SocketAddressFlat

[Kevin Wolf]

From: Markus Armbruster <armbru@redhat.com>

As its documentation says, it's not specific to Gluster.  Rename it,
as I'm going to use it for something else.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/gluster.c      | 38 +++++++++++++++++++-------------------
 qapi-schema.json     | 38 ++++++++++++++++++++++++++++++++++++++
 qapi/block-core.json | 46 +---------------------------------------------
 3 files changed, 58 insertions(+), 64 deletions(-)

diff --git a/block/gluster.c b/block/gluster.c
index 991f18f..64b0217 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -321,7 +321,7 @@ static int parse_volume_options(BlockdevOptionsGluster *gconf, char *path)
 static int qemu_gluster_parse_uri(BlockdevOptionsGluster *gconf,
                                   const char *filename)
 {
-    GlusterServer *gsconf;
+    SocketAddressFlat *gsconf;
     URI *uri;
     QueryParams *qp = NULL;
     bool is_unix = false;
@@ -332,19 +332,19 @@ static int qemu_gluster_parse_uri(BlockdevOptionsGluster *gconf,
         return -EINVAL;
     }
 
-    gconf->server = g_new0(GlusterServerList, 1);
-    gconf->server->value = gsconf = g_new0(GlusterServer, 1);
+    gconf->server = g_new0(SocketAddressFlatList, 1);
+    gconf->server->value = gsconf = g_new0(SocketAddressFlat, 1);
 
     /* transport */
     if (!uri->scheme || !strcmp(uri->scheme, "gluster")) {
-        gsconf->type = GLUSTER_TRANSPORT_TCP;
+        gsconf->type = SOCKET_ADDRESS_FLAT_TYPE_TCP;
     } else if (!strcmp(uri->scheme, "gluster+tcp")) {
-        gsconf->type = GLUSTER_TRANSPORT_TCP;
+        gsconf->type = SOCKET_ADDRESS_FLAT_TYPE_TCP;
     } else if (!strcmp(uri->scheme, "gluster+unix")) {
-        gsconf->type = GLUSTER_TRANSPORT_UNIX;
+        gsconf->type = SOCKET_ADDRESS_FLAT_TYPE_UNIX;
         is_unix = true;
     } else if (!strcmp(uri->scheme, "gluster+rdma")) {
-        gsconf->type = GLUSTER_TRANSPORT_TCP;
+        gsconf->type = SOCKET_ADDRESS_FLAT_TYPE_TCP;
         error_report("Warning: rdma feature is not supported, falling "
                      "back to tcp");
     } else {
@@ -396,7 +396,7 @@ static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
     struct glfs *glfs;
     int ret;
     int old_errno;
-    GlusterServerList *server;
+    SocketAddressFlatList *server;
     unsigned long long port;
 
     glfs = glfs_find_preopened(gconf->volume);
@@ -412,7 +412,7 @@ static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
     glfs_set_preopened(gconf->volume, glfs);
 
     for (server = gconf->server; server; server = server->next) {
-        if (server->value->type  == GLUSTER_TRANSPORT_UNIX) {
+        if (server->value->type  == SOCKET_ADDRESS_FLAT_TYPE_UNIX) {
             ret = glfs_set_volfile_server(glfs, "unix",
                                    server->value->u.q_unix.path, 0);
         } else {
@@ -443,7 +443,7 @@ static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
         error_setg(errp, "Gluster connection for volume %s, path %s failed"
                          " to connect", gconf->volume, gconf->path);
         for (server = gconf->server; server; server = server->next) {
-            if (server->value->type  == GLUSTER_TRANSPORT_UNIX) {
+            if (server->value->type  == SOCKET_ADDRESS_FLAT_TYPE_UNIX) {
                 error_append_hint(errp, "hint: failed on socket %s ",
                                   server->value->u.q_unix.path);
             } else {
@@ -480,8 +480,8 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
                                   QDict *options, Error **errp)
 {
     QemuOpts *opts;
-    GlusterServer *gsconf = NULL;
-    GlusterServerList *curr = NULL;
+    SocketAddressFlat *gsconf = NULL;
+    SocketAddressFlatList *curr = NULL;
     QDict *backing_options = NULL;
     Error *local_err = NULL;
     char *str = NULL;
@@ -535,9 +535,9 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
             goto out;
 
         }
-        gsconf = g_new0(GlusterServer, 1);
-        gsconf->type = qapi_enum_parse(GlusterTransport_lookup, ptr,
-                                       GLUSTER_TRANSPORT__MAX, -1,
+        gsconf = g_new0(SocketAddressFlat, 1);
+        gsconf->type = qapi_enum_parse(SocketAddressFlatType_lookup, ptr,
+                                       SOCKET_ADDRESS_FLAT_TYPE__MAX, -1,
                                        &local_err);
         if (local_err) {
             error_append_hint(&local_err,
@@ -548,7 +548,7 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
         }
         qemu_opts_del(opts);
 
-        if (gsconf->type == GLUSTER_TRANSPORT_TCP) {
+        if (gsconf->type == SOCKET_ADDRESS_FLAT_TYPE_TCP) {
             /* create opts info from runtime_tcp_opts list */
             opts = qemu_opts_create(&runtime_tcp_opts, NULL, 0, &error_abort);
             qemu_opts_absorb_qdict(opts, backing_options, &local_err);
@@ -617,11 +617,11 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
         }
 
         if (gconf->server == NULL) {
-            gconf->server = g_new0(GlusterServerList, 1);
+            gconf->server = g_new0(SocketAddressFlatList, 1);
             gconf->server->value = gsconf;
             curr = gconf->server;
         } else {
-            curr->next = g_new0(GlusterServerList, 1);
+            curr->next = g_new0(SocketAddressFlatList, 1);
             curr->next->value = gsconf;
             curr = curr->next;
         }
@@ -637,7 +637,7 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
 
 out:
     error_propagate(errp, local_err);
-    qapi_free_GlusterServer(gsconf);
+    qapi_free_SocketAddressFlat(gsconf);
     qemu_opts_del(opts);
     g_free(str);
     QDECREF(backing_options);
diff --git a/qapi-schema.json b/qapi-schema.json
index 6febfa7..15a8f7d 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -4101,6 +4101,44 @@
     'fd': 'String' } }
 
 ##
+# @SocketAddressFlatType:
+#
+# Available SocketAddressFlat types
+#
+# @tcp:   Internet address
+#
+# @unix:  Unix domain socket
+#
+# Since: 2.9
+##
+{ 'enum': 'SocketAddressFlatType',
+  'data': [ 'unix', 'tcp' ] }
+
+##
+# @SocketAddressFlat:
+#
+# Captures the address of a socket
+#
+# @type:       Transport type
+#
+# This is similar to SocketAddress, only distinction:
+#
+# 1. SocketAddressFlat is a flat union, SocketAddress is a simple union.
+#    A flat union is nicer than simple because it avoids nesting
+#    (i.e. more {}) on the wire.
+#
+# 2. SocketAddressFlat supports only types 'unix' and 'tcp', because
+#    that's what its current users need.
+#
+# Since: 2.9
+##
+{ 'union': 'SocketAddressFlat',
+  'base': { 'type': 'SocketAddressFlatType' },
+  'discriminator': 'type',
+  'data': { 'unix': 'UnixSocketAddress',
+            'tcp': 'InetSocketAddress' } }
+
+##
 # @getfd:
 #
 # Receive a file descriptor via SCM rights and assign it a name
diff --git a/qapi/block-core.json b/qapi/block-core.json
index bc0ccd6..d63be0a 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2546,50 +2546,6 @@
             '*read-pattern': 'QuorumReadPattern' } }
 
 ##
-# @GlusterTransport:
-#
-# An enumeration of Gluster transport types
-#
-# @tcp:   TCP   - Transmission Control Protocol
-#
-# @unix:  UNIX  - Unix domain socket
-#
-# Since: 2.7
-##
-{ 'enum': 'GlusterTransport',
-  'data': [ 'unix', 'tcp' ] }
-
-
-##
-# @GlusterServer:
-#
-# Captures the address of a socket
-#
-# Details for connecting to a gluster server
-#
-# @type:       Transport type used for gluster connection
-#
-# This is similar to SocketAddress, only distinction:
-#
-# 1. GlusterServer is a flat union, SocketAddress is a simple union.
-#    A flat union is nicer than simple because it avoids nesting
-#    (i.e. more {}) on the wire.
-#
-# 2. GlusterServer lacks case 'fd', since gluster doesn't let you
-#    pass in a file descriptor.
-#
-# GlusterServer is actually not Gluster-specific, its a
-# compatibility evolved into an alternate for SocketAddress.
-#
-# Since: 2.7
-##
-{ 'union': 'GlusterServer',
-  'base': { 'type': 'GlusterTransport' },
-  'discriminator': 'type',
-  'data': { 'unix': 'UnixSocketAddress',
-            'tcp': 'InetSocketAddress' } }
-
-##
 # @BlockdevOptionsGluster:
 #
 # Driver specific block device options for Gluster
@@ -2610,7 +2566,7 @@
 { 'struct': 'BlockdevOptionsGluster',
   'data': { 'volume': 'str',
             'path': 'str',
-            'server': ['GlusterServer'],
+            'server': ['SocketAddressFlat'],
             '*debug': 'int',
             '*logfile': 'str' } }
 
-- 
1.8.3.1

[Qemu-devel] [PULL 24/27] qapi-schema: Rename SocketAddressFlat's variant tcp to inet

[Kevin Wolf]

From: Markus Armbruster <armbru@redhat.com>

QAPI type SocketAddressFlat differs from SocketAddress pointlessly:
the discriminator value for variant InetSocketAddress is 'tcp' instead
of 'inet'.  Rename.

The type is so far only used by the Gluster block drivers.  Take care
to keep 'tcp' working in things like -drive's file.server.0.type=tcp.
The "gluster+tcp" URI scheme in pseudo-filenames stays the same.
blockdev-add changes, but it has changed incompatibly since 2.8
already.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/gluster.c  | 59 +++++++++++++++++++++++++++++---------------------------
 qapi-schema.json |  8 ++++----
 2 files changed, 35 insertions(+), 32 deletions(-)

diff --git a/block/gluster.c b/block/gluster.c
index 64b0217..a577dae 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -152,7 +152,7 @@ static QemuOptsList runtime_type_opts = {
         {
             .name = GLUSTER_OPT_TYPE,
             .type = QEMU_OPT_STRING,
-            .help = "tcp|unix",
+            .help = "inet|unix",
         },
         { /* end of list */ }
     },
@@ -171,14 +171,14 @@ static QemuOptsList runtime_unix_opts = {
     },
 };
 
-static QemuOptsList runtime_tcp_opts = {
-    .name = "gluster_tcp",
-    .head = QTAILQ_HEAD_INITIALIZER(runtime_tcp_opts.head),
+static QemuOptsList runtime_inet_opts = {
+    .name = "gluster_inet",
+    .head = QTAILQ_HEAD_INITIALIZER(runtime_inet_opts.head),
     .desc = {
         {
             .name = GLUSTER_OPT_TYPE,
             .type = QEMU_OPT_STRING,
-            .help = "tcp|unix",
+            .help = "inet|unix",
         },
         {
             .name = GLUSTER_OPT_HOST,
@@ -337,14 +337,14 @@ static int qemu_gluster_parse_uri(BlockdevOptionsGluster *gconf,
 
     /* transport */
     if (!uri->scheme || !strcmp(uri->scheme, "gluster")) {
-        gsconf->type = SOCKET_ADDRESS_FLAT_TYPE_TCP;
+        gsconf->type = SOCKET_ADDRESS_FLAT_TYPE_INET;
     } else if (!strcmp(uri->scheme, "gluster+tcp")) {
-        gsconf->type = SOCKET_ADDRESS_FLAT_TYPE_TCP;
+        gsconf->type = SOCKET_ADDRESS_FLAT_TYPE_INET;
     } else if (!strcmp(uri->scheme, "gluster+unix")) {
         gsconf->type = SOCKET_ADDRESS_FLAT_TYPE_UNIX;
         is_unix = true;
     } else if (!strcmp(uri->scheme, "gluster+rdma")) {
-        gsconf->type = SOCKET_ADDRESS_FLAT_TYPE_TCP;
+        gsconf->type = SOCKET_ADDRESS_FLAT_TYPE_INET;
         error_report("Warning: rdma feature is not supported, falling "
                      "back to tcp");
     } else {
@@ -374,11 +374,11 @@ static int qemu_gluster_parse_uri(BlockdevOptionsGluster *gconf,
         }
         gsconf->u.q_unix.path = g_strdup(qp->p[0].value);
     } else {
-        gsconf->u.tcp.host = g_strdup(uri->server ? uri->server : "localhost");
+        gsconf->u.inet.host = g_strdup(uri->server ? uri->server : "localhost");
         if (uri->port) {
-            gsconf->u.tcp.port = g_strdup_printf("%d", uri->port);
+            gsconf->u.inet.port = g_strdup_printf("%d", uri->port);
         } else {
-            gsconf->u.tcp.port = g_strdup_printf("%d", GLUSTER_DEFAULT_PORT);
+            gsconf->u.inet.port = g_strdup_printf("%d", GLUSTER_DEFAULT_PORT);
         }
     }
 
@@ -416,15 +416,15 @@ static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
             ret = glfs_set_volfile_server(glfs, "unix",
                                    server->value->u.q_unix.path, 0);
         } else {
-            if (parse_uint_full(server->value->u.tcp.port, &port, 10) < 0 ||
+            if (parse_uint_full(server->value->u.inet.port, &port, 10) < 0 ||
                 port > 65535) {
                 error_setg(errp, "'%s' is not a valid port number",
-                           server->value->u.tcp.port);
+                           server->value->u.inet.port);
                 errno = EINVAL;
                 goto out;
             }
             ret = glfs_set_volfile_server(glfs, "tcp",
-                                   server->value->u.tcp.host,
+                                   server->value->u.inet.host,
                                    (int)port);
         }
 
@@ -448,8 +448,8 @@ static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
                                   server->value->u.q_unix.path);
             } else {
                 error_append_hint(errp, "hint: failed on host %s and port %s ",
-                                  server->value->u.tcp.host,
-                                  server->value->u.tcp.port);
+                                  server->value->u.inet.host,
+                                  server->value->u.inet.port);
             }
         }
 
@@ -536,21 +536,24 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
 
         }
         gsconf = g_new0(SocketAddressFlat, 1);
+        if (!strcmp(ptr, "tcp")) {
+            ptr = "inet";       /* accept legacy "tcp" */
+        }
         gsconf->type = qapi_enum_parse(SocketAddressFlatType_lookup, ptr,
                                        SOCKET_ADDRESS_FLAT_TYPE__MAX, -1,
                                        &local_err);
         if (local_err) {
             error_append_hint(&local_err,
-                              "Parameter '%s' may be 'tcp' or 'unix'\n",
+                              "Parameter '%s' may be 'inet' or 'unix'\n",
                               GLUSTER_OPT_TYPE);
             error_append_hint(&local_err, GERR_INDEX_HINT, i);
             goto out;
         }
         qemu_opts_del(opts);
 
-        if (gsconf->type == SOCKET_ADDRESS_FLAT_TYPE_TCP) {
-            /* create opts info from runtime_tcp_opts list */
-            opts = qemu_opts_create(&runtime_tcp_opts, NULL, 0, &error_abort);
+        if (gsconf->type == SOCKET_ADDRESS_FLAT_TYPE_INET) {
+            /* create opts info from runtime_inet_opts list */
+            opts = qemu_opts_create(&runtime_inet_opts, NULL, 0, &error_abort);
             qemu_opts_absorb_qdict(opts, backing_options, &local_err);
             if (local_err) {
                 goto out;
@@ -563,7 +566,7 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
                 error_append_hint(&local_err, GERR_INDEX_HINT, i);
                 goto out;
             }
-            gsconf->u.tcp.host = g_strdup(ptr);
+            gsconf->u.inet.host = g_strdup(ptr);
             ptr = qemu_opt_get(opts, GLUSTER_OPT_PORT);
             if (!ptr) {
                 error_setg(&local_err, QERR_MISSING_PARAMETER,
@@ -571,28 +574,28 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
                 error_append_hint(&local_err, GERR_INDEX_HINT, i);
                 goto out;
             }
-            gsconf->u.tcp.port = g_strdup(ptr);
+            gsconf->u.inet.port = g_strdup(ptr);
 
             /* defend for unsupported fields in InetSocketAddress,
              * i.e. @ipv4, @ipv6  and @to
              */
             ptr = qemu_opt_get(opts, GLUSTER_OPT_TO);
             if (ptr) {
-                gsconf->u.tcp.has_to = true;
+                gsconf->u.inet.has_to = true;
             }
             ptr = qemu_opt_get(opts, GLUSTER_OPT_IPV4);
             if (ptr) {
-                gsconf->u.tcp.has_ipv4 = true;
+                gsconf->u.inet.has_ipv4 = true;
             }
             ptr = qemu_opt_get(opts, GLUSTER_OPT_IPV6);
             if (ptr) {
-                gsconf->u.tcp.has_ipv6 = true;
+                gsconf->u.inet.has_ipv6 = true;
             }
-            if (gsconf->u.tcp.has_to) {
+            if (gsconf->u.inet.has_to) {
                 error_setg(&local_err, "Parameter 'to' not supported");
                 goto out;
             }
-            if (gsconf->u.tcp.has_ipv4 || gsconf->u.tcp.has_ipv6) {
+            if (gsconf->u.inet.has_ipv4 || gsconf->u.inet.has_ipv6) {
                 error_setg(&local_err, "Parameters 'ipv4/ipv6' not supported");
                 goto out;
             }
@@ -669,7 +672,7 @@ static struct glfs *qemu_gluster_init(BlockdevOptionsGluster *gconf,
                              "file.volume=testvol,file.path=/path/a.qcow2"
                              "[,file.debug=9]"
                              "[,file.logfile=/path/filename.log],"
-                             "file.server.0.type=tcp,"
+                             "file.server.0.type=inet,"
                              "file.server.0.host=1.2.3.4,"
                              "file.server.0.port=24007,"
                              "file.server.1.transport=unix,"
diff --git a/qapi-schema.json b/qapi-schema.json
index 15a8f7d..32b4a4b 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -4105,14 +4105,14 @@
 #
 # Available SocketAddressFlat types
 #
-# @tcp:   Internet address
+# @inet:  Internet address
 #
 # @unix:  Unix domain socket
 #
 # Since: 2.9
 ##
 { 'enum': 'SocketAddressFlatType',
-  'data': [ 'unix', 'tcp' ] }
+  'data': [ 'unix', 'inet' ] }
 
 ##
 # @SocketAddressFlat:
@@ -4127,7 +4127,7 @@
 #    A flat union is nicer than simple because it avoids nesting
 #    (i.e. more {}) on the wire.
 #
-# 2. SocketAddressFlat supports only types 'unix' and 'tcp', because
+# 2. SocketAddressFlat supports only types 'unix' and 'inet', because
 #    that's what its current users need.
 #
 # Since: 2.9
@@ -4136,7 +4136,7 @@
   'base': { 'type': 'SocketAddressFlatType' },
   'discriminator': 'type',
   'data': { 'unix': 'UnixSocketAddress',
-            'tcp': 'InetSocketAddress' } }
+            'inet': 'InetSocketAddress' } }
 
 ##
 # @getfd:
-- 
1.8.3.1

[Qemu-devel] [PULL 25/27] sheepdog: Support blockdev-add

[Kevin Wolf]

From: Markus Armbruster <armbru@redhat.com>

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qapi/block-core.json | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index d63be0a..9bb7f4a 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2124,6 +2124,7 @@
 # @ssh: Since 2.8
 # @iscsi: Since 2.9
 # @rbd: Since 2.9
+# @sheepdog: Since 2.9
 #
 # Since: 2.0
 ##
@@ -2132,8 +2133,8 @@
             'dmg', 'file', 'ftp', 'ftps', 'gluster', 'host_cdrom',
             'host_device', 'http', 'https', 'iscsi', 'luks', 'nbd', 'nfs',
             'null-aio', 'null-co', 'parallels', 'qcow', 'qcow2', 'qed',
-            'quorum', 'raw', 'rbd', 'replication', 'ssh', 'vdi', 'vhdx', 'vmdk',
-            'vpc', 'vvfat' ] }
+            'quorum', 'raw', 'rbd', 'replication', 'sheepdog', 'ssh',
+            'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] }
 
 ##
 # @BlockdevOptionsFile:
@@ -2692,6 +2693,26 @@
             '*password-secret': 'str' } }
 
 ##
+# @BlockdevOptionsSheepdog:
+#
+# Driver specific block device options for sheepdog
+#
+# @vdi:         Virtual disk image name
+# @addr:        The Sheepdog server to connect to
+# @snap-id:     Snapshot ID
+# @tag:         Snapshot tag name
+#
+# Only one of @snap-id and @tag may be present.
+#
+# Since: 2.9
+##
+{ 'struct': 'BlockdevOptionsSheepdog',
+  'data': { 'addr': 'SocketAddressFlat',
+            'vdi': 'str',
+            '*snap-id': 'uint32',
+            '*tag': 'str' } }
+
+##
 # @ReplicationMode:
 #
 # An enumeration of replication modes.
@@ -2891,7 +2912,7 @@
       'raw':        'BlockdevOptionsRaw',
       'rbd':        'BlockdevOptionsRbd',
       'replication':'BlockdevOptionsReplication',
-# TODO sheepdog: Wait for structured options
+      'sheepdog':   'BlockdevOptionsSheepdog',
       'ssh':        'BlockdevOptionsSsh',
       'vdi':        'BlockdevOptionsGenericFormat',
       'vhdx':       'BlockdevOptionsGenericFormat',
-- 
1.8.3.1

[Qemu-devel] [PULL 26/27] block: Don't use error_abort in blk_new_open

[Kevin Wolf]

From: Fam Zheng <famz@redhat.com>

We have an errp and bdrv_root_attach_child can fail permission check,
error_abort is not the best choice here.

Signed-off-by: Fam Zheng <famz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/block-backend.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index daa7908..5742c09 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -213,7 +213,12 @@ BlockBackend *blk_new_open(const char *filename, const char *reference,
     }
 
     blk->root = bdrv_root_attach_child(bs, "root", &child_root,
-                                       perm, BLK_PERM_ALL, blk, &error_abort);
+                                       perm, BLK_PERM_ALL, blk, errp);
+    if (!blk->root) {
+        bdrv_unref(bs);
+        blk_unref(blk);
+        return NULL;
+    }
 
     return blk;
 }
-- 
1.8.3.1

[Qemu-devel] [PULL 27/27] commit: Don't use error_abort in commit_start

[Kevin Wolf]

From: Fam Zheng <famz@redhat.com>

bdrv_set_backing_hd failure needn't be abort. Since we already have
error parameter, use it.

Signed-off-by: Fam Zheng <famz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/commit.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/block/commit.c b/block/commit.c
index e57c1cf..9c41988 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -316,8 +316,20 @@ void commit_start(const char *job_id, BlockDriverState *bs,
         goto fail;
     }
 
-    bdrv_set_backing_hd(commit_top_bs, top, &error_abort);
-    bdrv_set_backing_hd(overlay_bs, commit_top_bs, &error_abort);
+    bdrv_set_backing_hd(commit_top_bs, top, &local_err);
+    if (local_err) {
+        bdrv_unref(commit_top_bs);
+        commit_top_bs = NULL;
+        error_propagate(errp, local_err);
+        goto fail;
+    }
+    bdrv_set_backing_hd(overlay_bs, commit_top_bs, &local_err);
+    if (local_err) {
+        bdrv_unref(commit_top_bs);
+        commit_top_bs = NULL;
+        error_propagate(errp, local_err);
+        goto fail;
+    }
 
     s->commit_top_bs = commit_top_bs;
     bdrv_unref(commit_top_bs);
-- 
1.8.3.1

Re: [Qemu-devel] [PULL 00/27] Block layer fixes for 2.9.0-rc0

[Peter Maydell]

On 7 March 2017 at 16:40, Kevin Wolf <kwolf@redhat.com> wrote:
> The following changes since commit ff79d5e939c38677a575e3493eb9b4d36eb21865:
>
>   Merge remote-tracking branch 'remotes/xtensa/tags/20170306-xtensa' into staging (2017-03-07 09:57:14 +0000)
>
> are available in the git repository at:
>
>
>   git://repo.or.cz/qemu/kevin.git tags/for-upstream
>
> for you to fetch changes up to b69f00dde490e88d55f5ee731545e690b2dc61f8:
>
>   commit: Don't use error_abort in commit_start (2017-03-07 14:53:29 +0100)
>
> ----------------------------------------------------------------
> Block layer fixes for 2.9.0-rc0
>

Applied, thanks.

-- PMM