From b247767aacdc2cfba94050c901aec268be94cc2d Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Fri, 3 Mar 2017 16:54:21 +0100 Subject: [PATCH 01/27] commit: Fix error handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Fam Zheng Reviewed-by: Philippe Mathieu-Daudé --- block/commit.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/block/commit.c b/block/commit.c index 22a0a4db98..e57c1cffa2 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; } From 7d9fcb391c7898fb1417d84eba41d3d48bd76605 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 2 Mar 2017 17:48:14 +0100 Subject: [PATCH 02/27] mirror: Fix permission problem with 'replaces' 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 Reviewed-by: Fam Zheng Reviewed-by: Eric Blake --- block/mirror.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/block/mirror.c b/block/mirror.c index 57f26c33a4..c9185b38a8 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 From 0bf74767ff7f0ae23615162807961c5d7e73507c Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Mon, 6 Mar 2017 16:03:00 +0100 Subject: [PATCH 03/27] mirror: Fix permissions for removing mirror_top_bs 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 Reviewed-by: Fam Zheng Reviewed-by: Eric Blake --- block/mirror.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/block/mirror.c b/block/mirror.c index c9185b38a8..001b5f0a15 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)); } From 88f9d1b3d2fa8df276b2f75dcbd7fb78ed08be20 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Mon, 6 Mar 2017 16:12:44 +0100 Subject: [PATCH 04/27] mirror: Fix error path for dirty bitmap creation mirror_top_bs must be removed from the graph again when creating the dirty bitmap fails. Signed-off-by: Kevin Wolf Reviewed-by: Fam Zheng Reviewed-by: Eric Blake --- block/mirror.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/block/mirror.c b/block/mirror.c index 001b5f0a15..f24dc51385 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() */ From 067acf28d1d726059f994356f25e054ce2926acf Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 2 Mar 2017 15:26:18 +0100 Subject: [PATCH 05/27] block: Fix blockdev-snapshot error handling 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 Reviewed-by: Fam Zheng Reviewed-by: Eric Blake --- blockdev.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/blockdev.c b/blockdev.c index 8eb4e84fe0..af67ce4e56 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); } } From d0ac038025bb6e7886c2324258d24e71215b5d35 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Wed, 1 Mar 2017 17:30:41 +0100 Subject: [PATCH 06/27] block: Factor out should_update_child() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Kevin Wolf Reviewed-by: Fam Zheng Reviewed-by: Eric Blake Reviewed-by: Philippe Mathieu-Daudé --- block.c | 42 +++++++++++++++++++++++++++--------------- 1 file changed, 27 insertions(+), 15 deletions(-) diff --git a/block.c b/block.c index f293ccb5af..6dc02b85aa 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 From 8ee039951dea9a809e4745c42aebb4a7cec4bbbb Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Mon, 6 Mar 2017 13:45:28 +0100 Subject: [PATCH 07/27] block: Factor out bdrv_replace_child_noperm() Signed-off-by: Kevin Wolf Reviewed-by: Fam Zheng Reviewed-by: Eric Blake --- block.c | 40 ++++++++++++++++++++++++++-------------- 1 file changed, 26 insertions(+), 14 deletions(-) diff --git a/block.c b/block.c index 6dc02b85aa..d4570c85b3 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,13 +1726,6 @@ static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs, child->role->detach(child); } QLIST_REMOVE(child, next_parent); - - /* Update permissions for old node. This is guaranteed to succeed - * 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_set_perm(old_bs, perm, shared_perm); } child->bs = new_bs; @@ -1744,15 +1736,35 @@ static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs, 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. */ + bdrv_get_cumulative_perm(old_bs, &perm, &shared_perm); + bdrv_check_perm(old_bs, perm, shared_perm, &error_abort); + bdrv_set_perm(old_bs, perm, shared_perm); + } + + bdrv_replace_child_noperm(child, 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_set_perm(new_bs, perm, shared_perm); - - if (child->role->attach) { - child->role->attach(child); - } } } From 46181129eac9a56d9a948667282dd03d5015f096 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Mon, 6 Mar 2017 15:00:13 +0100 Subject: [PATCH 08/27] block: Ignore multiple children in bdrv_check_update_perm() 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 Reviewed-by: Fam Zheng Reviewed-by: Eric Blake --- 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 d4570c85b3..a7b09d32c3 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 a57c0bfb55..fc83f7f2ce 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, From 234ac1a9025bcfcc532449f72a97b3d4754d466c Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 2 Mar 2017 18:43:00 +0100 Subject: [PATCH 09/27] block: Handle permission errors in change_parent_backing_link() 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 Reviewed-by: Fam Zheng Reviewed-by: Eric Blake --- block.c | 50 ++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 44 insertions(+), 6 deletions(-) diff --git a/block.c b/block.c index a7b09d32c3..a3101329c0 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); } From 5fe31c25cce3b09e989e40439667cd4961aba969 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Mon, 6 Mar 2017 16:20:51 +0100 Subject: [PATCH 10/27] block: Fix error handling in bdrv_replace_in_backing_chain() 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 Reviewed-by: Fam Zheng Reviewed-by: Eric Blake --- 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 a3101329c0..dd9ded81b9 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 f24dc51385..a5d30ee575 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 af67ce4e56..f1f49bd3ca 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 c7c4a3ac3a..5149260827 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 fc83f7f2ce..6c699ac9c3 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, From cbc488ee2aaad06e7ef69936788e0dbd204cf0a0 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Mon, 6 Mar 2017 20:00:35 +0100 Subject: [PATCH 11/27] sheepdog: Defuse time bomb in sd_open() error handling 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 Reviewed-by: Eric Blake Signed-off-by: Kevin Wolf --- block/sheepdog.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/block/sheepdog.c b/block/sheepdog.c index 743471043e..c3ee4ce512 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; From e25cad6921ef78c8e2b7a4f59b03545bd675a9ad Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Mon, 6 Mar 2017 20:00:36 +0100 Subject: [PATCH 12/27] sheepdog: Fix error handling in sd_snapshot_delete() 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 Reviewed-by: Eric Blake Signed-off-by: Kevin Wolf --- block/sheepdog.c | 41 +++++++++++++++++++---------------------- 1 file changed, 19 insertions(+), 22 deletions(-) diff --git a/block/sheepdog.c b/block/sheepdog.c index c3ee4ce512..0a0803ee58 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) From 48d7c4af06393b974b0a515ac9d1cc7346dbae23 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Mon, 6 Mar 2017 20:00:37 +0100 Subject: [PATCH 13/27] sheepdog: Fix error handling sd_create() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Eric Blake Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- block/sheepdog.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/block/sheepdog.c b/block/sheepdog.c index 0a0803ee58..be3db1f150 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; } From a0dc0e2bfe543404c07258b2a2c4f9d53c0430b1 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Mon, 6 Mar 2017 20:00:38 +0100 Subject: [PATCH 14/27] sheepdog: Mark sd_snapshot_delete() lossage FIXME 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 Reviewed-by: Eric Blake Signed-off-by: Kevin Wolf --- block/sheepdog.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/block/sheepdog.c b/block/sheepdog.c index be3db1f150..187bcd8236 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 : ""); 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); } From 89e2a31d337f96ab8d5b7bdfe4bcce0a25181ed1 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Mon, 6 Mar 2017 20:00:39 +0100 Subject: [PATCH 15/27] sheepdog: Fix snapshot ID parsing in _open(), _create, _goto() 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 Reviewed-by: Eric Blake Signed-off-by: Kevin Wolf --- block/sheepdog.c | 66 ++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 55 insertions(+), 11 deletions(-) diff --git a/block/sheepdog.c b/block/sheepdog.c index 187bcd8236..d3d373196c 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); } From daa0b0d4b17d25d7d46326bac8efa449300e3711 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Mon, 6 Mar 2017 20:00:40 +0100 Subject: [PATCH 16/27] sheepdog: Don't truncate long VDI name in _open(), _create() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit sd_parse_uri() truncates long VDI names silently. Reject them instead. Signed-off-by: Markus Armbruster Reviewed-by: Philippe Mathieu-Daudé Signed-off-by: Kevin Wolf --- block/sheepdog.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/block/sheepdog.c b/block/sheepdog.c index d3d373196c..fed7264cec 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)) { From 36bcac16fdd6ecb75314db06171f54dcd400ab8c Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Mon, 6 Mar 2017 20:00:41 +0100 Subject: [PATCH 17/27] sheepdog: Report errors in pseudo-filename more usefully MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- block/sheepdog.c | 88 ++++++++++++++++++++++++++++++++---------------- 1 file changed, 59 insertions(+), 29 deletions(-) diff --git a/block/sheepdog.c b/block/sheepdog.c index fed7264cec..161932d681 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; } From 8ecc2f9eab17e203e8a64726b7806a9442b451c0 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Mon, 6 Mar 2017 20:00:42 +0100 Subject: [PATCH 18/27] sheepdog: Use SocketAddress and socket_connect() 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 Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- block/sheepdog.c | 53 +++++++++++++++++++++++++++--------------------- 1 file changed, 30 insertions(+), 23 deletions(-) diff --git a/block/sheepdog.c b/block/sheepdog.c index 161932d681..9b1e12162d 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) From 831acdc95e9c0faaf0bd0ced50f168afd7ad92ac Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Mon, 6 Mar 2017 20:00:43 +0100 Subject: [PATCH 19/27] sheepdog: Implement bdrv_parse_filename() 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 Reviewed-by: Eric Blake Signed-off-by: Kevin Wolf --- block/sheepdog.c | 234 +++++++++++++++++++++++++++++++++++------------ 1 file changed, 176 insertions(+), 58 deletions(-) diff --git a/block/sheepdog.c b/block/sheepdog.c index 9b1e12162d..89e98edab6 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; + sd_config_done(cfg); } - if (qp) { - query_params_free(qp); - } - 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"); + + 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; + } + + 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 (!tag) { + tag = ""; + } + 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 = -1; - memset(vdi, 0, sizeof(vdi)); - memset(tag, 0, sizeof(tag)); - - if (strstr(filename, "://")) { - sd_parse_uri(s, filename, vdi, &snapid, tag, &local_err); - } else { - parse_vdiname(s, filename, vdi, &snapid, tag, &local_err); - } - if (local_err) { - error_propagate(errp, local_err); - ret = -EINVAL; - goto err_no_fd; - } 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, From fc29458dee2b4206eba90ed210342105e1556fd0 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Mon, 6 Mar 2017 20:00:44 +0100 Subject: [PATCH 20/27] gluster: Drop assumptions on SocketTransport names 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 Reviewed-by: Niels de Vos Signed-off-by: Kevin Wolf --- block/gluster.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/block/gluster.c b/block/gluster.c index 56b4abe3a7..7236d592ae 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); } From e152ef7a98c584e8d5de88d7a5294ab6a8e7059f Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Mon, 6 Mar 2017 20:00:45 +0100 Subject: [PATCH 21/27] gluster: Don't duplicate qapi-util.c's qapi_enum_parse() Signed-off-by: Markus Armbruster Reviewed-by: Niels de Vos Signed-off-by: Kevin Wolf --- block/gluster.c | 30 +++++++++--------------------- 1 file changed, 9 insertions(+), 21 deletions(-) diff --git a/block/gluster.c b/block/gluster.c index 7236d592ae..6fbcf9e1e8 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; } From 85a82e852d01c2cd26d3126c5900a5bd1c464609 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Mon, 6 Mar 2017 20:00:46 +0100 Subject: [PATCH 22/27] gluster: Plug memory leaks in qemu_gluster_parse_json() 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 Reviewed-by: Niels de Vos Signed-off-by: Kevin Wolf --- block/gluster.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/block/gluster.c b/block/gluster.c index 6fbcf9e1e8..991f18fad6 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; } From 2b733709d7a29273406d341f60efe3b5d69ea362 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Mon, 6 Mar 2017 20:00:47 +0100 Subject: [PATCH 23/27] qapi-schema: Rename GlusterServer to SocketAddressFlat 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 Reviewed-by: Eric Blake Signed-off-by: Kevin Wolf --- 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 991f18fad6..64b0217797 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 6febfa7b90..15a8f7d3b0 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -4100,6 +4100,44 @@ 'vsock': 'VsockSocketAddress', '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: # diff --git a/qapi/block-core.json b/qapi/block-core.json index bc0ccd615c..d63be0ae2d 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -2545,50 +2545,6 @@ '*rewrite-corrupted': 'bool', '*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: # @@ -2610,7 +2566,7 @@ { 'struct': 'BlockdevOptionsGluster', 'data': { 'volume': 'str', 'path': 'str', - 'server': ['GlusterServer'], + 'server': ['SocketAddressFlat'], '*debug': 'int', '*logfile': 'str' } } From c5f1ae3ae7b19591c78a61e46cf812b0b968fef0 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Mon, 6 Mar 2017 20:00:48 +0100 Subject: [PATCH 24/27] qapi-schema: Rename SocketAddressFlat's variant tcp to inet 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 Reviewed-by: Eric Blake Signed-off-by: Kevin Wolf --- 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 64b0217797..a577daef10 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 15a8f7d3b0..32b4a4b782 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: From d282f34e82bf8f243dd1f7103963fec1d4129672 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Mon, 6 Mar 2017 20:00:49 +0100 Subject: [PATCH 25/27] sheepdog: Support blockdev-add Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Signed-off-by: Kevin Wolf --- 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 d63be0ae2d..9bb7f4a17b 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: @@ -2691,6 +2692,26 @@ '*auth-supported': ['RbdAuthMethod'], '*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: # @@ -2891,7 +2912,7 @@ 'raw': 'BlockdevOptionsRaw', 'rbd': 'BlockdevOptionsRbd', 'replication':'BlockdevOptionsReplication', -# TODO sheepdog: Wait for structured options + 'sheepdog': 'BlockdevOptionsSheepdog', 'ssh': 'BlockdevOptionsSsh', 'vdi': 'BlockdevOptionsGenericFormat', 'vhdx': 'BlockdevOptionsGenericFormat', From 50bfbe93b2ca0ab795f3de305bec5ab1df620be4 Mon Sep 17 00:00:00 2001 From: Fam Zheng Date: Tue, 7 Mar 2017 19:07:21 +0800 Subject: [PATCH 26/27] block: Don't use error_abort in blk_new_open 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 Signed-off-by: Kevin Wolf --- 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 daa7908d01..5742c09c2c 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; } From b69f00dde490e88d55f5ee731545e690b2dc61f8 Mon Sep 17 00:00:00 2001 From: Fam Zheng Date: Tue, 7 Mar 2017 19:07:22 +0800 Subject: [PATCH 27/27] commit: Don't use error_abort in commit_start bdrv_set_backing_hd failure needn't be abort. Since we already have error parameter, use it. Signed-off-by: Fam Zheng Signed-off-by: Kevin Wolf --- block/commit.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/block/commit.c b/block/commit.c index e57c1cffa2..9c4198837f 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);