* [Qemu-devel] [PATCH V5 1/7] block: Add bs->node_name to hold the name of a bs node of the bs graph.
2013-12-12 15:33 [Qemu-devel] [PATCH V5 0/7] Giving names to BlockDriverState graph nodes Benoît Canet
@ 2013-12-12 15:33 ` Benoît Canet
2014-01-21 3:10 ` Fam Zheng
2013-12-12 15:33 ` [Qemu-devel] [PATCH V5 2/7] block: Allow the user to define "node-name" option Benoît Canet
` (8 subsequent siblings)
9 siblings, 1 reply; 28+ messages in thread
From: Benoît Canet @ 2013-12-12 15:33 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, Benoît Canet, jcody, armbru, stefanha
Add the minimum of code to prepare for the following patches.
Signed-off-by: Benoit Canet <benoit@irqsave.net>
---
block.c | 57 +++++++++++++++++++++++++++++++++++------------
include/block/block.h | 1 +
include/block/block_int.h | 9 +++++++-
3 files changed, 52 insertions(+), 15 deletions(-)
diff --git a/block.c b/block.c
index 64e7d22..481d566 100644
--- a/block.c
+++ b/block.c
@@ -90,6 +90,9 @@ static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs,
static QTAILQ_HEAD(, BlockDriverState) bdrv_states =
QTAILQ_HEAD_INITIALIZER(bdrv_states);
+static QTAILQ_HEAD(, BlockDriverState) graph_bdrv_states =
+ QTAILQ_HEAD_INITIALIZER(graph_bdrv_states);
+
static QLIST_HEAD(, BlockDriver) bdrv_drivers =
QLIST_HEAD_INITIALIZER(bdrv_drivers);
@@ -327,7 +330,7 @@ BlockDriverState *bdrv_new(const char *device_name)
QLIST_INIT(&bs->dirty_bitmaps);
pstrcpy(bs->device_name, sizeof(bs->device_name), device_name);
if (device_name[0] != '\0') {
- QTAILQ_INSERT_TAIL(&bdrv_states, bs, list);
+ QTAILQ_INSERT_TAIL(&bdrv_states, bs, device_list);
}
bdrv_iostatus_disable(bs);
notifier_list_init(&bs->close_notifiers);
@@ -1501,7 +1504,7 @@ void bdrv_close_all(void)
{
BlockDriverState *bs;
- QTAILQ_FOREACH(bs, &bdrv_states, list) {
+ QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
bdrv_close(bs);
}
}
@@ -1530,7 +1533,7 @@ static bool bdrv_requests_pending(BlockDriverState *bs)
static bool bdrv_requests_pending_all(void)
{
BlockDriverState *bs;
- QTAILQ_FOREACH(bs, &bdrv_states, list) {
+ QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
if (bdrv_requests_pending(bs)) {
return true;
}
@@ -1557,7 +1560,7 @@ void bdrv_drain_all(void)
BlockDriverState *bs;
while (busy) {
- QTAILQ_FOREACH(bs, &bdrv_states, list) {
+ QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
bdrv_start_throttled_reqs(bs);
}
@@ -1566,14 +1569,19 @@ void bdrv_drain_all(void)
}
}
-/* make a BlockDriverState anonymous by removing from bdrv_state list.
+/* make a BlockDriverState anonymous by removing from bdrv_state and
+ * graph_bdrv_state list.
Also, NULL terminate the device_name to prevent double remove */
void bdrv_make_anon(BlockDriverState *bs)
{
if (bs->device_name[0] != '\0') {
- QTAILQ_REMOVE(&bdrv_states, bs, list);
+ QTAILQ_REMOVE(&bdrv_states, bs, device_list);
}
bs->device_name[0] = '\0';
+ if (bs->node_name[0] != '\0') {
+ QTAILQ_REMOVE(&graph_bdrv_states, bs, node_list);
+ }
+ bs->node_name[0] = '\0';
}
static void bdrv_rebind(BlockDriverState *bs)
@@ -1627,7 +1635,12 @@ static void bdrv_move_feature_fields(BlockDriverState *bs_dest,
/* keep the same entry in bdrv_states */
pstrcpy(bs_dest->device_name, sizeof(bs_dest->device_name),
bs_src->device_name);
- bs_dest->list = bs_src->list;
+ bs_dest->device_list = bs_src->device_list;
+
+ /* keep the same entry in graph_bdrv_states
+ * We do want to swap name but don't want to swap linked list entries
+ */
+ bs_dest->node_list = bs_src->node_list;
}
/*
@@ -1952,7 +1965,7 @@ int bdrv_commit_all(void)
{
BlockDriverState *bs;
- QTAILQ_FOREACH(bs, &bdrv_states, list) {
+ QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
if (bs->drv && bs->backing_hd) {
int ret = bdrv_commit(bs);
if (ret < 0) {
@@ -3110,11 +3123,12 @@ void bdrv_iterate_format(void (*it)(void *opaque, const char *name),
}
}
+/* This function is to find block backend bs */
BlockDriverState *bdrv_find(const char *name)
{
BlockDriverState *bs;
- QTAILQ_FOREACH(bs, &bdrv_states, list) {
+ QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
if (!strcmp(name, bs->device_name)) {
return bs;
}
@@ -3122,19 +3136,34 @@ BlockDriverState *bdrv_find(const char *name)
return NULL;
}
+/* This function is to find a node in the bs graph */
+BlockDriverState *bdrv_find_node(const char *node_name)
+{
+ BlockDriverState *bs;
+
+ assert(node_name);
+
+ QTAILQ_FOREACH(bs, &graph_bdrv_states, node_list) {
+ if (!strcmp(node_name, bs->node_name)) {
+ return bs;
+ }
+ }
+ return NULL;
+}
+
BlockDriverState *bdrv_next(BlockDriverState *bs)
{
if (!bs) {
return QTAILQ_FIRST(&bdrv_states);
}
- return QTAILQ_NEXT(bs, list);
+ return QTAILQ_NEXT(bs, device_list);
}
void bdrv_iterate(void (*it)(void *opaque, BlockDriverState *bs), void *opaque)
{
BlockDriverState *bs;
- QTAILQ_FOREACH(bs, &bdrv_states, list) {
+ QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
it(opaque, bs);
}
}
@@ -3154,7 +3183,7 @@ int bdrv_flush_all(void)
BlockDriverState *bs;
int result = 0;
- QTAILQ_FOREACH(bs, &bdrv_states, list) {
+ QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
int ret = bdrv_flush(bs);
if (ret < 0 && !result) {
result = ret;
@@ -4278,7 +4307,7 @@ void bdrv_invalidate_cache_all(void)
{
BlockDriverState *bs;
- QTAILQ_FOREACH(bs, &bdrv_states, list) {
+ QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
bdrv_invalidate_cache(bs);
}
}
@@ -4287,7 +4316,7 @@ void bdrv_clear_incoming_migration_all(void)
{
BlockDriverState *bs;
- QTAILQ_FOREACH(bs, &bdrv_states, list) {
+ QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
bs->open_flags = bs->open_flags & ~(BDRV_O_INCOMING);
}
}
diff --git a/include/block/block.h b/include/block/block.h
index 36efaea..834abf9 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -374,6 +374,7 @@ void bdrv_lock_medium(BlockDriverState *bs, bool locked);
void bdrv_eject(BlockDriverState *bs, bool eject_flag);
const char *bdrv_get_format_name(BlockDriverState *bs);
BlockDriverState *bdrv_find(const char *name);
+BlockDriverState *bdrv_find_node(const char *node_name);
BlockDriverState *bdrv_next(BlockDriverState *bs);
void bdrv_iterate(void (*it)(void *opaque, BlockDriverState *bs),
void *opaque);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 8b132d7..bd5220f 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -325,11 +325,18 @@ struct BlockDriverState {
BlockdevOnError on_read_error, on_write_error;
bool iostatus_enabled;
BlockDeviceIoStatus iostatus;
+
+ /* the following member gives a name to every node on the bs graph. */
+ char node_name[32];
+ /* element of the list of named nodes building the graph */
+ QTAILQ_ENTRY(BlockDriverState) node_list;
+ /* Device name is the name associated with the "drive" the guest sees */
char device_name[32];
+ /* element of the list of "drives" the guest sees */
+ QTAILQ_ENTRY(BlockDriverState) device_list;
QLIST_HEAD(, BdrvDirtyBitmap) dirty_bitmaps;
int refcnt;
int in_use; /* users other than guest access, eg. block migration */
- QTAILQ_ENTRY(BlockDriverState) list;
QLIST_HEAD(, BdrvTrackedRequest) tracked_requests;
--
1.8.3.2
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH V5 1/7] block: Add bs->node_name to hold the name of a bs node of the bs graph.
2013-12-12 15:33 ` [Qemu-devel] [PATCH V5 1/7] block: Add bs->node_name to hold the name of a bs node of the bs graph Benoît Canet
@ 2014-01-21 3:10 ` Fam Zheng
0 siblings, 0 replies; 28+ messages in thread
From: Fam Zheng @ 2014-01-21 3:10 UTC (permalink / raw)
To: Benoît Canet; +Cc: kwolf, jcody, qemu-devel, stefanha, armbru
On Thu, 12/12 16:33, Benoît Canet wrote:
> Add the minimum of code to prepare for the following patches.
>
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
> block.c | 57 +++++++++++++++++++++++++++++++++++------------
> include/block/block.h | 1 +
> include/block/block_int.h | 9 +++++++-
> 3 files changed, 52 insertions(+), 15 deletions(-)
>
> diff --git a/block.c b/block.c
> index 64e7d22..481d566 100644
> --- a/block.c
> +++ b/block.c
> @@ -90,6 +90,9 @@ static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs,
> static QTAILQ_HEAD(, BlockDriverState) bdrv_states =
> QTAILQ_HEAD_INITIALIZER(bdrv_states);
>
> +static QTAILQ_HEAD(, BlockDriverState) graph_bdrv_states =
> + QTAILQ_HEAD_INITIALIZER(graph_bdrv_states);
> +
> static QLIST_HEAD(, BlockDriver) bdrv_drivers =
> QLIST_HEAD_INITIALIZER(bdrv_drivers);
>
> @@ -327,7 +330,7 @@ BlockDriverState *bdrv_new(const char *device_name)
> QLIST_INIT(&bs->dirty_bitmaps);
> pstrcpy(bs->device_name, sizeof(bs->device_name), device_name);
> if (device_name[0] != '\0') {
> - QTAILQ_INSERT_TAIL(&bdrv_states, bs, list);
> + QTAILQ_INSERT_TAIL(&bdrv_states, bs, device_list);
> }
> bdrv_iostatus_disable(bs);
> notifier_list_init(&bs->close_notifiers);
> @@ -1501,7 +1504,7 @@ void bdrv_close_all(void)
> {
> BlockDriverState *bs;
>
> - QTAILQ_FOREACH(bs, &bdrv_states, list) {
> + QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> bdrv_close(bs);
> }
> }
> @@ -1530,7 +1533,7 @@ static bool bdrv_requests_pending(BlockDriverState *bs)
> static bool bdrv_requests_pending_all(void)
> {
> BlockDriverState *bs;
> - QTAILQ_FOREACH(bs, &bdrv_states, list) {
> + QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> if (bdrv_requests_pending(bs)) {
> return true;
> }
> @@ -1557,7 +1560,7 @@ void bdrv_drain_all(void)
> BlockDriverState *bs;
>
> while (busy) {
> - QTAILQ_FOREACH(bs, &bdrv_states, list) {
> + QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> bdrv_start_throttled_reqs(bs);
> }
>
> @@ -1566,14 +1569,19 @@ void bdrv_drain_all(void)
> }
> }
>
> -/* make a BlockDriverState anonymous by removing from bdrv_state list.
> +/* make a BlockDriverState anonymous by removing from bdrv_state and
> + * graph_bdrv_state list.
> Also, NULL terminate the device_name to prevent double remove */
> void bdrv_make_anon(BlockDriverState *bs)
> {
> if (bs->device_name[0] != '\0') {
> - QTAILQ_REMOVE(&bdrv_states, bs, list);
> + QTAILQ_REMOVE(&bdrv_states, bs, device_list);
> }
> bs->device_name[0] = '\0';
> + if (bs->node_name[0] != '\0') {
> + QTAILQ_REMOVE(&graph_bdrv_states, bs, node_list);
> + }
> + bs->node_name[0] = '\0';
> }
>
> static void bdrv_rebind(BlockDriverState *bs)
> @@ -1627,7 +1635,12 @@ static void bdrv_move_feature_fields(BlockDriverState *bs_dest,
> /* keep the same entry in bdrv_states */
> pstrcpy(bs_dest->device_name, sizeof(bs_dest->device_name),
> bs_src->device_name);
> - bs_dest->list = bs_src->list;
> + bs_dest->device_list = bs_src->device_list;
> +
> + /* keep the same entry in graph_bdrv_states
> + * We do want to swap name but don't want to swap linked list entries
> + */
> + bs_dest->node_list = bs_src->node_list;
> }
>
> /*
> @@ -1952,7 +1965,7 @@ int bdrv_commit_all(void)
> {
> BlockDriverState *bs;
>
> - QTAILQ_FOREACH(bs, &bdrv_states, list) {
> + QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> if (bs->drv && bs->backing_hd) {
> int ret = bdrv_commit(bs);
> if (ret < 0) {
> @@ -3110,11 +3123,12 @@ void bdrv_iterate_format(void (*it)(void *opaque, const char *name),
> }
> }
>
> +/* This function is to find block backend bs */
> BlockDriverState *bdrv_find(const char *name)
> {
> BlockDriverState *bs;
>
> - QTAILQ_FOREACH(bs, &bdrv_states, list) {
> + QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> if (!strcmp(name, bs->device_name)) {
> return bs;
> }
> @@ -3122,19 +3136,34 @@ BlockDriverState *bdrv_find(const char *name)
> return NULL;
> }
>
> +/* This function is to find a node in the bs graph */
> +BlockDriverState *bdrv_find_node(const char *node_name)
> +{
> + BlockDriverState *bs;
> +
> + assert(node_name);
> +
> + QTAILQ_FOREACH(bs, &graph_bdrv_states, node_list) {
> + if (!strcmp(node_name, bs->node_name)) {
> + return bs;
> + }
> + }
> + return NULL;
> +}
> +
> BlockDriverState *bdrv_next(BlockDriverState *bs)
> {
> if (!bs) {
> return QTAILQ_FIRST(&bdrv_states);
> }
> - return QTAILQ_NEXT(bs, list);
> + return QTAILQ_NEXT(bs, device_list);
> }
>
> void bdrv_iterate(void (*it)(void *opaque, BlockDriverState *bs), void *opaque)
> {
> BlockDriverState *bs;
>
> - QTAILQ_FOREACH(bs, &bdrv_states, list) {
> + QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> it(opaque, bs);
> }
> }
> @@ -3154,7 +3183,7 @@ int bdrv_flush_all(void)
> BlockDriverState *bs;
> int result = 0;
>
> - QTAILQ_FOREACH(bs, &bdrv_states, list) {
> + QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> int ret = bdrv_flush(bs);
> if (ret < 0 && !result) {
> result = ret;
> @@ -4278,7 +4307,7 @@ void bdrv_invalidate_cache_all(void)
> {
> BlockDriverState *bs;
>
> - QTAILQ_FOREACH(bs, &bdrv_states, list) {
> + QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> bdrv_invalidate_cache(bs);
> }
> }
> @@ -4287,7 +4316,7 @@ void bdrv_clear_incoming_migration_all(void)
> {
> BlockDriverState *bs;
>
> - QTAILQ_FOREACH(bs, &bdrv_states, list) {
> + QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> bs->open_flags = bs->open_flags & ~(BDRV_O_INCOMING);
> }
> }
> diff --git a/include/block/block.h b/include/block/block.h
> index 36efaea..834abf9 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -374,6 +374,7 @@ void bdrv_lock_medium(BlockDriverState *bs, bool locked);
> void bdrv_eject(BlockDriverState *bs, bool eject_flag);
> const char *bdrv_get_format_name(BlockDriverState *bs);
> BlockDriverState *bdrv_find(const char *name);
> +BlockDriverState *bdrv_find_node(const char *node_name);
> BlockDriverState *bdrv_next(BlockDriverState *bs);
> void bdrv_iterate(void (*it)(void *opaque, BlockDriverState *bs),
> void *opaque);
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 8b132d7..bd5220f 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -325,11 +325,18 @@ struct BlockDriverState {
> BlockdevOnError on_read_error, on_write_error;
> bool iostatus_enabled;
> BlockDeviceIoStatus iostatus;
> +
> + /* the following member gives a name to every node on the bs graph. */
> + char node_name[32];
> + /* element of the list of named nodes building the graph */
> + QTAILQ_ENTRY(BlockDriverState) node_list;
> + /* Device name is the name associated with the "drive" the guest sees */
> char device_name[32];
> + /* element of the list of "drives" the guest sees */
> + QTAILQ_ENTRY(BlockDriverState) device_list;
> QLIST_HEAD(, BdrvDirtyBitmap) dirty_bitmaps;
> int refcnt;
> int in_use; /* users other than guest access, eg. block migration */
> - QTAILQ_ENTRY(BlockDriverState) list;
>
> QLIST_HEAD(, BdrvTrackedRequest) tracked_requests;
>
> --
> 1.8.3.2
>
>
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH V5 2/7] block: Allow the user to define "node-name" option.
2013-12-12 15:33 [Qemu-devel] [PATCH V5 0/7] Giving names to BlockDriverState graph nodes Benoît Canet
2013-12-12 15:33 ` [Qemu-devel] [PATCH V5 1/7] block: Add bs->node_name to hold the name of a bs node of the bs graph Benoît Canet
@ 2013-12-12 15:33 ` Benoît Canet
2014-01-21 3:15 ` Fam Zheng
2014-01-21 14:12 ` Kevin Wolf
2013-12-12 15:33 ` [Qemu-devel] [PATCH V5 3/7] qmp: Add a command to list the named BlockDriverState nodes Benoît Canet
` (7 subsequent siblings)
9 siblings, 2 replies; 28+ messages in thread
From: Benoît Canet @ 2013-12-12 15:33 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, Benoît Canet, jcody, armbru, stefanha
Signed-off-by: Benoit Canet <benoit@irqsave.net>
---
block.c | 38 ++++++++++++++++++++++++++++++++++++++
1 file changed, 38 insertions(+)
diff --git a/block.c b/block.c
index 481d566..1c57f0d 100644
--- a/block.c
+++ b/block.c
@@ -735,6 +735,39 @@ static int bdrv_open_flags(BlockDriverState *bs, int flags)
return open_flags;
}
+static int bdrv_get_node_name(BlockDriverState *bs,
+ QDict *options,
+ Error **errp)
+{
+ const char *node_name = NULL;
+
+ node_name = qdict_get_try_str(options, "node-name");
+
+ if (!node_name) {
+ return 0;
+ }
+
+ /* empty string node name is invalid */
+ if (node_name[0] == '\0') {
+ error_setg(errp, "Empty node name");
+ return -EINVAL;
+ }
+
+ /* takes care of avoiding duplicates node names */
+ if (bdrv_find_node(node_name)) {
+ error_setg(errp, "Duplicate node name");
+ return -EINVAL;
+ }
+
+ /* copy node name into the bs and insert it into the graph list */
+ pstrcpy(bs->node_name, sizeof(bs->node_name), node_name);
+ QTAILQ_INSERT_TAIL(&graph_bdrv_states, bs, node_list);
+
+ qdict_del(options, "node-name");
+
+ return 0;
+}
+
/*
* Common part for opening disk images and files
*
@@ -759,6 +792,11 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
trace_bdrv_open_common(bs, filename ?: "", flags, drv->format_name);
+ ret = bdrv_get_node_name(bs, options, errp);
+ if (ret < 0) {
+ return ret;
+ }
+
/* bdrv_open() with directly using a protocol as drv. This layer is already
* opened, so assign it to bs (while file becomes a closed BlockDriverState)
* and return immediately. */
--
1.8.3.2
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH V5 2/7] block: Allow the user to define "node-name" option.
2013-12-12 15:33 ` [Qemu-devel] [PATCH V5 2/7] block: Allow the user to define "node-name" option Benoît Canet
@ 2014-01-21 3:15 ` Fam Zheng
2014-01-21 14:12 ` Kevin Wolf
1 sibling, 0 replies; 28+ messages in thread
From: Fam Zheng @ 2014-01-21 3:15 UTC (permalink / raw)
To: Benoît Canet; +Cc: kwolf, jcody, qemu-devel, stefanha, armbru
On Thu, 12/12 16:33, Benoît Canet wrote:
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
> block.c | 38 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 38 insertions(+)
>
> diff --git a/block.c b/block.c
> index 481d566..1c57f0d 100644
> --- a/block.c
> +++ b/block.c
> @@ -735,6 +735,39 @@ static int bdrv_open_flags(BlockDriverState *bs, int flags)
> return open_flags;
> }
>
> +static int bdrv_get_node_name(BlockDriverState *bs,
> + QDict *options,
> + Error **errp)
This function actually assigns the node-name to bs, could you call it
"bdrv_set_node_name" or "bdrv_assign_node_name", and only pass in the node-name
and move the parsing of options to the caller?
Fam
> +{
> + const char *node_name = NULL;
> +
> + node_name = qdict_get_try_str(options, "node-name");
> +
> + if (!node_name) {
> + return 0;
> + }
> +
> + /* empty string node name is invalid */
> + if (node_name[0] == '\0') {
> + error_setg(errp, "Empty node name");
> + return -EINVAL;
> + }
> +
> + /* takes care of avoiding duplicates node names */
> + if (bdrv_find_node(node_name)) {
> + error_setg(errp, "Duplicate node name");
> + return -EINVAL;
> + }
> +
> + /* copy node name into the bs and insert it into the graph list */
> + pstrcpy(bs->node_name, sizeof(bs->node_name), node_name);
> + QTAILQ_INSERT_TAIL(&graph_bdrv_states, bs, node_list);
> +
> + qdict_del(options, "node-name");
> +
> + return 0;
> +}
> +
> /*
> * Common part for opening disk images and files
> *
> @@ -759,6 +792,11 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
>
> trace_bdrv_open_common(bs, filename ?: "", flags, drv->format_name);
>
> + ret = bdrv_get_node_name(bs, options, errp);
> + if (ret < 0) {
> + return ret;
> + }
> +
> /* bdrv_open() with directly using a protocol as drv. This layer is already
> * opened, so assign it to bs (while file becomes a closed BlockDriverState)
> * and return immediately. */
> --
> 1.8.3.2
>
>
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH V5 2/7] block: Allow the user to define "node-name" option.
2013-12-12 15:33 ` [Qemu-devel] [PATCH V5 2/7] block: Allow the user to define "node-name" option Benoît Canet
2014-01-21 3:15 ` Fam Zheng
@ 2014-01-21 14:12 ` Kevin Wolf
1 sibling, 0 replies; 28+ messages in thread
From: Kevin Wolf @ 2014-01-21 14:12 UTC (permalink / raw)
To: Benoît Canet; +Cc: famz, jcody, qemu-devel, armbru, stefanha
Am 12.12.2013 um 16:33 hat Benoît Canet geschrieben:
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
> block.c | 38 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 38 insertions(+)
I think you should add an optional 'node-name' field to
BlockdevOptionsBase in the QAPI schema, so that this can't only be set
on the command line, but also using blockdev-add in QMP.
Kevin
^ permalink raw reply [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH V5 3/7] qmp: Add a command to list the named BlockDriverState nodes.
2013-12-12 15:33 [Qemu-devel] [PATCH V5 0/7] Giving names to BlockDriverState graph nodes Benoît Canet
2013-12-12 15:33 ` [Qemu-devel] [PATCH V5 1/7] block: Add bs->node_name to hold the name of a bs node of the bs graph Benoît Canet
2013-12-12 15:33 ` [Qemu-devel] [PATCH V5 2/7] block: Allow the user to define "node-name" option Benoît Canet
@ 2013-12-12 15:33 ` Benoît Canet
2014-01-21 3:23 ` Fam Zheng
2013-12-12 15:33 ` [Qemu-devel] [PATCH V5 4/7] qmp: Allow to change password on named block driver states Benoît Canet
` (6 subsequent siblings)
9 siblings, 1 reply; 28+ messages in thread
From: Benoît Canet @ 2013-12-12 15:33 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, Benoît Canet, jcody, armbru, stefanha
Signed-off-by: Benoit Canet <benoit@irqsave.net>
---
block.c | 18 +++++++++
block/qapi.c | 109 +++++++++++++++++++++++++-------------------------
blockdev.c | 5 +++
include/block/block.h | 1 +
include/block/qapi.h | 1 +
qapi-schema.json | 16 +++++++-
qmp-commands.hx | 61 ++++++++++++++++++++++++++++
7 files changed, 155 insertions(+), 56 deletions(-)
diff --git a/block.c b/block.c
index 1c57f0d..78d13e5 100644
--- a/block.c
+++ b/block.c
@@ -32,6 +32,7 @@
#include "sysemu/sysemu.h"
#include "qemu/notify.h"
#include "block/coroutine.h"
+#include "block/qapi.h"
#include "qmp-commands.h"
#include "qemu/timer.h"
@@ -3189,6 +3190,23 @@ BlockDriverState *bdrv_find_node(const char *node_name)
return NULL;
}
+/* Put this QMP function here so it can access the static graph_bdrv_states. */
+BlockDeviceInfoList *bdrv_named_nodes_list(void)
+{
+ BlockDeviceInfoList *list, *entry;
+ BlockDriverState *bs;
+
+ list = NULL;
+ QTAILQ_FOREACH(bs, &graph_bdrv_states, node_list) {
+ entry = g_malloc0(sizeof(*entry));
+ entry->value = bdrv_block_device_info(bs);
+ entry->next = list;
+ list = entry;
+ }
+
+ return list;
+}
+
BlockDriverState *bdrv_next(BlockDriverState *bs)
{
if (!bs) {
diff --git a/block/qapi.c b/block/qapi.c
index a32cb79..556f7fb 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -29,6 +29,60 @@
#include "qapi/qmp-output-visitor.h"
#include "qapi/qmp/types.h"
+BlockDeviceInfo *bdrv_block_device_info(BlockDriverState *bs)
+{
+ BlockDeviceInfo *info = g_malloc0(sizeof(*info));
+
+ info->file = g_strdup(bs->filename);
+ info->ro = bs->read_only;
+ info->drv = g_strdup(bs->drv->format_name);
+ info->encrypted = bs->encrypted;
+ info->encryption_key_missing = bdrv_key_required(bs);
+
+ if (bs->node_name[0]) {
+ info->has_node_name = true;
+ info->node_name = g_strdup(bs->node_name);
+ }
+
+ if (bs->backing_file[0]) {
+ info->has_backing_file = true;
+ info->backing_file = g_strdup(bs->backing_file);
+ }
+
+ info->backing_file_depth = bdrv_get_backing_file_depth(bs);
+
+ if (bs->io_limits_enabled) {
+ ThrottleConfig cfg;
+ throttle_get_config(&bs->throttle_state, &cfg);
+ info->bps = cfg.buckets[THROTTLE_BPS_TOTAL].avg;
+ info->bps_rd = cfg.buckets[THROTTLE_BPS_READ].avg;
+ info->bps_wr = cfg.buckets[THROTTLE_BPS_WRITE].avg;
+
+ info->iops = cfg.buckets[THROTTLE_OPS_TOTAL].avg;
+ info->iops_rd = cfg.buckets[THROTTLE_OPS_READ].avg;
+ info->iops_wr = cfg.buckets[THROTTLE_OPS_WRITE].avg;
+
+ info->has_bps_max = cfg.buckets[THROTTLE_BPS_TOTAL].max;
+ info->bps_max = cfg.buckets[THROTTLE_BPS_TOTAL].max;
+ info->has_bps_rd_max = cfg.buckets[THROTTLE_BPS_READ].max;
+ info->bps_rd_max = cfg.buckets[THROTTLE_BPS_READ].max;
+ info->has_bps_wr_max = cfg.buckets[THROTTLE_BPS_WRITE].max;
+ info->bps_wr_max = cfg.buckets[THROTTLE_BPS_WRITE].max;
+
+ info->has_iops_max = cfg.buckets[THROTTLE_OPS_TOTAL].max;
+ info->iops_max = cfg.buckets[THROTTLE_OPS_TOTAL].max;
+ info->has_iops_rd_max = cfg.buckets[THROTTLE_OPS_READ].max;
+ info->iops_rd_max = cfg.buckets[THROTTLE_OPS_READ].max;
+ info->has_iops_wr_max = cfg.buckets[THROTTLE_OPS_WRITE].max;
+ info->iops_wr_max = cfg.buckets[THROTTLE_OPS_WRITE].max;
+
+ info->has_iops_size = cfg.op_size;
+ info->iops_size = cfg.op_size;
+ }
+
+ return info;
+}
+
/*
* Returns 0 on success, with *p_list either set to describe snapshot
* information, or NULL because there are no snapshots. Returns -errno on
@@ -211,60 +265,7 @@ void bdrv_query_info(BlockDriverState *bs,
if (bs->drv) {
info->has_inserted = true;
- info->inserted = g_malloc0(sizeof(*info->inserted));
- info->inserted->file = g_strdup(bs->filename);
- info->inserted->ro = bs->read_only;
- info->inserted->drv = g_strdup(bs->drv->format_name);
- info->inserted->encrypted = bs->encrypted;
- info->inserted->encryption_key_missing = bdrv_key_required(bs);
-
- if (bs->backing_file[0]) {
- info->inserted->has_backing_file = true;
- info->inserted->backing_file = g_strdup(bs->backing_file);
- }
-
- info->inserted->backing_file_depth = bdrv_get_backing_file_depth(bs);
-
- if (bs->io_limits_enabled) {
- ThrottleConfig cfg;
- throttle_get_config(&bs->throttle_state, &cfg);
- info->inserted->bps = cfg.buckets[THROTTLE_BPS_TOTAL].avg;
- info->inserted->bps_rd = cfg.buckets[THROTTLE_BPS_READ].avg;
- info->inserted->bps_wr = cfg.buckets[THROTTLE_BPS_WRITE].avg;
-
- info->inserted->iops = cfg.buckets[THROTTLE_OPS_TOTAL].avg;
- info->inserted->iops_rd = cfg.buckets[THROTTLE_OPS_READ].avg;
- info->inserted->iops_wr = cfg.buckets[THROTTLE_OPS_WRITE].avg;
-
- info->inserted->has_bps_max =
- cfg.buckets[THROTTLE_BPS_TOTAL].max;
- info->inserted->bps_max =
- cfg.buckets[THROTTLE_BPS_TOTAL].max;
- info->inserted->has_bps_rd_max =
- cfg.buckets[THROTTLE_BPS_READ].max;
- info->inserted->bps_rd_max =
- cfg.buckets[THROTTLE_BPS_READ].max;
- info->inserted->has_bps_wr_max =
- cfg.buckets[THROTTLE_BPS_WRITE].max;
- info->inserted->bps_wr_max =
- cfg.buckets[THROTTLE_BPS_WRITE].max;
-
- info->inserted->has_iops_max =
- cfg.buckets[THROTTLE_OPS_TOTAL].max;
- info->inserted->iops_max =
- cfg.buckets[THROTTLE_OPS_TOTAL].max;
- info->inserted->has_iops_rd_max =
- cfg.buckets[THROTTLE_OPS_READ].max;
- info->inserted->iops_rd_max =
- cfg.buckets[THROTTLE_OPS_READ].max;
- info->inserted->has_iops_wr_max =
- cfg.buckets[THROTTLE_OPS_WRITE].max;
- info->inserted->iops_wr_max =
- cfg.buckets[THROTTLE_OPS_WRITE].max;
-
- info->inserted->has_iops_size = cfg.op_size;
- info->inserted->iops_size = cfg.op_size;
- }
+ info->inserted = bdrv_block_device_info(bs);
bs0 = bs;
p_image_info = &info->inserted->image;
diff --git a/blockdev.c b/blockdev.c
index 44755e1..204ab40 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1940,6 +1940,11 @@ void qmp_drive_backup(const char *device, const char *target,
}
}
+BlockDeviceInfoList *qmp_query_named_block_nodes(Error **errp)
+{
+ return bdrv_named_nodes_list();
+}
+
#define DEFAULT_MIRROR_BUF_SIZE (10 << 20)
void qmp_drive_mirror(const char *device, const char *target,
diff --git a/include/block/block.h b/include/block/block.h
index 834abf9..8c10123 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -375,6 +375,7 @@ void bdrv_eject(BlockDriverState *bs, bool eject_flag);
const char *bdrv_get_format_name(BlockDriverState *bs);
BlockDriverState *bdrv_find(const char *name);
BlockDriverState *bdrv_find_node(const char *node_name);
+BlockDeviceInfoList *bdrv_named_nodes_list(void);
BlockDriverState *bdrv_next(BlockDriverState *bs);
void bdrv_iterate(void (*it)(void *opaque, BlockDriverState *bs),
void *opaque);
diff --git a/include/block/qapi.h b/include/block/qapi.h
index 9518ee4..e92c00d 100644
--- a/include/block/qapi.h
+++ b/include/block/qapi.h
@@ -29,6 +29,7 @@
#include "block/block.h"
#include "block/snapshot.h"
+BlockDeviceInfo *bdrv_block_device_info(BlockDriverState *bs);
int bdrv_query_snapshot_info_list(BlockDriverState *bs,
SnapshotInfoList **p_list,
Error **errp);
diff --git a/qapi-schema.json b/qapi-schema.json
index c3c939c..0dadd5d 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -810,6 +810,8 @@
#
# @file: the filename of the backing device
#
+# @node-name: #optional the name of the block driver node (Since 2.0)
+#
# @ro: true if the backing device was open read-only
#
# @drv: the name of the block format used to open the backing device. As of
@@ -857,10 +859,9 @@
#
# Since: 0.14.0
#
-# Notes: This interface is only found in @BlockInfo.
##
{ 'type': 'BlockDeviceInfo',
- 'data': { 'file': 'str', 'ro': 'bool', 'drv': 'str',
+ 'data': { 'file': 'str', '*node-name': 'str', 'ro': 'bool', 'drv': 'str',
'*backing_file': 'str', 'backing_file_depth': 'int',
'encrypted': 'bool', 'encryption_key_missing': 'bool',
'bps': 'int', 'bps_rd': 'int', 'bps_wr': 'int',
@@ -2008,6 +2009,17 @@
{ 'command': 'drive-backup', 'data': 'DriveBackup' }
##
+# @query-named-block-nodes
+#
+# Get the named block driver list
+#
+# Returns: the list of BlockDeviceInfo
+#
+# Since 2.0
+##
+{ 'command': 'query-named-block-nodes', 'returns': [ 'BlockDeviceInfo' ] }
+
+##
# @drive-mirror
#
# Start mirroring a block device's writes to a new destination.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index fba15cd..d644fe9 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -3295,3 +3295,64 @@ Example (2):
<- { "return": {} }
EQMP
+
+ {
+ .name = "query-named-block-nodes",
+ .args_type = "",
+ .mhandler.cmd_new = qmp_marshal_input_query_named_block_nodes,
+ },
+
+SQMP
+@query-named-block-nodes
+------------------------
+
+Return a list of BlockDeviceInfo for each named block driver node
+
+Example:
+
+-> { "execute": "query-named-block-nodes" }
+<- { "return": [ { "ro":false,
+ "drv":"qcow2",
+ "encrypted":false,
+ "file":"disks/test.qcow2",
+ "node-name": "my-node",
+ "backing_file_depth":1,
+ "bps":1000000,
+ "bps_rd":0,
+ "bps_wr":0,
+ "iops":1000000,
+ "iops_rd":0,
+ "iops_wr":0,
+ "bps_max": 8000000,
+ "bps_rd_max": 0,
+ "bps_wr_max": 0,
+ "iops_max": 0,
+ "iops_rd_max": 0,
+ "iops_wr_max": 0,
+ "iops_size": 0,
+ "image":{
+ "filename":"disks/test.qcow2",
+ "format":"qcow2",
+ "virtual-size":2048000,
+ "backing_file":"base.qcow2",
+ "full-backing-filename":"disks/base.qcow2",
+ "backing-filename-format:"qcow2",
+ "snapshots":[
+ {
+ "id": "1",
+ "name": "snapshot1",
+ "vm-state-size": 0,
+ "date-sec": 10000200,
+ "date-nsec": 12,
+ "vm-clock-sec": 206,
+ "vm-clock-nsec": 30
+ }
+ ],
+ "backing-image":{
+ "filename":"disks/base.qcow2",
+ "format":"qcow2",
+ "virtual-size":2048000
+ }
+ } } ] }
+
+EQMP
--
1.8.3.2
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH V5 3/7] qmp: Add a command to list the named BlockDriverState nodes.
2013-12-12 15:33 ` [Qemu-devel] [PATCH V5 3/7] qmp: Add a command to list the named BlockDriverState nodes Benoît Canet
@ 2014-01-21 3:23 ` Fam Zheng
0 siblings, 0 replies; 28+ messages in thread
From: Fam Zheng @ 2014-01-21 3:23 UTC (permalink / raw)
To: Benoît Canet; +Cc: kwolf, jcody, qemu-devel, stefanha, armbru
I think it's worth to at least mention the name of the command in the commit
message.
On Thu, 12/12 16:33, Benoît Canet wrote:
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
> block.c | 18 +++++++++
> block/qapi.c | 109 +++++++++++++++++++++++++-------------------------
> blockdev.c | 5 +++
> include/block/block.h | 1 +
> include/block/qapi.h | 1 +
> qapi-schema.json | 16 +++++++-
> qmp-commands.hx | 61 ++++++++++++++++++++++++++++
> 7 files changed, 155 insertions(+), 56 deletions(-)
>
> diff --git a/block.c b/block.c
> index 1c57f0d..78d13e5 100644
> --- a/block.c
> +++ b/block.c
> @@ -32,6 +32,7 @@
> #include "sysemu/sysemu.h"
> #include "qemu/notify.h"
> #include "block/coroutine.h"
> +#include "block/qapi.h"
> #include "qmp-commands.h"
> #include "qemu/timer.h"
>
> @@ -3189,6 +3190,23 @@ BlockDriverState *bdrv_find_node(const char *node_name)
> return NULL;
> }
>
> +/* Put this QMP function here so it can access the static graph_bdrv_states. */
> +BlockDeviceInfoList *bdrv_named_nodes_list(void)
> +{
> + BlockDeviceInfoList *list, *entry;
> + BlockDriverState *bs;
> +
> + list = NULL;
> + QTAILQ_FOREACH(bs, &graph_bdrv_states, node_list) {
> + entry = g_malloc0(sizeof(*entry));
> + entry->value = bdrv_block_device_info(bs);
> + entry->next = list;
> + list = entry;
> + }
> +
> + return list;
> +}
> +
> BlockDriverState *bdrv_next(BlockDriverState *bs)
> {
> if (!bs) {
> diff --git a/block/qapi.c b/block/qapi.c
> index a32cb79..556f7fb 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -29,6 +29,60 @@
> #include "qapi/qmp-output-visitor.h"
> #include "qapi/qmp/types.h"
>
> +BlockDeviceInfo *bdrv_block_device_info(BlockDriverState *bs)
> +{
> + BlockDeviceInfo *info = g_malloc0(sizeof(*info));
> +
> + info->file = g_strdup(bs->filename);
> + info->ro = bs->read_only;
> + info->drv = g_strdup(bs->drv->format_name);
> + info->encrypted = bs->encrypted;
> + info->encryption_key_missing = bdrv_key_required(bs);
> +
> + if (bs->node_name[0]) {
> + info->has_node_name = true;
> + info->node_name = g_strdup(bs->node_name);
> + }
> +
> + if (bs->backing_file[0]) {
> + info->has_backing_file = true;
> + info->backing_file = g_strdup(bs->backing_file);
> + }
> +
> + info->backing_file_depth = bdrv_get_backing_file_depth(bs);
> +
> + if (bs->io_limits_enabled) {
> + ThrottleConfig cfg;
> + throttle_get_config(&bs->throttle_state, &cfg);
> + info->bps = cfg.buckets[THROTTLE_BPS_TOTAL].avg;
> + info->bps_rd = cfg.buckets[THROTTLE_BPS_READ].avg;
> + info->bps_wr = cfg.buckets[THROTTLE_BPS_WRITE].avg;
> +
> + info->iops = cfg.buckets[THROTTLE_OPS_TOTAL].avg;
> + info->iops_rd = cfg.buckets[THROTTLE_OPS_READ].avg;
> + info->iops_wr = cfg.buckets[THROTTLE_OPS_WRITE].avg;
> +
> + info->has_bps_max = cfg.buckets[THROTTLE_BPS_TOTAL].max;
> + info->bps_max = cfg.buckets[THROTTLE_BPS_TOTAL].max;
> + info->has_bps_rd_max = cfg.buckets[THROTTLE_BPS_READ].max;
> + info->bps_rd_max = cfg.buckets[THROTTLE_BPS_READ].max;
> + info->has_bps_wr_max = cfg.buckets[THROTTLE_BPS_WRITE].max;
> + info->bps_wr_max = cfg.buckets[THROTTLE_BPS_WRITE].max;
> +
> + info->has_iops_max = cfg.buckets[THROTTLE_OPS_TOTAL].max;
> + info->iops_max = cfg.buckets[THROTTLE_OPS_TOTAL].max;
> + info->has_iops_rd_max = cfg.buckets[THROTTLE_OPS_READ].max;
> + info->iops_rd_max = cfg.buckets[THROTTLE_OPS_READ].max;
> + info->has_iops_wr_max = cfg.buckets[THROTTLE_OPS_WRITE].max;
> + info->iops_wr_max = cfg.buckets[THROTTLE_OPS_WRITE].max;
> +
> + info->has_iops_size = cfg.op_size;
> + info->iops_size = cfg.op_size;
> + }
> +
> + return info;
> +}
> +
> /*
> * Returns 0 on success, with *p_list either set to describe snapshot
> * information, or NULL because there are no snapshots. Returns -errno on
> @@ -211,60 +265,7 @@ void bdrv_query_info(BlockDriverState *bs,
>
> if (bs->drv) {
> info->has_inserted = true;
> - info->inserted = g_malloc0(sizeof(*info->inserted));
> - info->inserted->file = g_strdup(bs->filename);
> - info->inserted->ro = bs->read_only;
> - info->inserted->drv = g_strdup(bs->drv->format_name);
> - info->inserted->encrypted = bs->encrypted;
> - info->inserted->encryption_key_missing = bdrv_key_required(bs);
> -
> - if (bs->backing_file[0]) {
> - info->inserted->has_backing_file = true;
> - info->inserted->backing_file = g_strdup(bs->backing_file);
> - }
> -
> - info->inserted->backing_file_depth = bdrv_get_backing_file_depth(bs);
> -
> - if (bs->io_limits_enabled) {
> - ThrottleConfig cfg;
> - throttle_get_config(&bs->throttle_state, &cfg);
> - info->inserted->bps = cfg.buckets[THROTTLE_BPS_TOTAL].avg;
> - info->inserted->bps_rd = cfg.buckets[THROTTLE_BPS_READ].avg;
> - info->inserted->bps_wr = cfg.buckets[THROTTLE_BPS_WRITE].avg;
> -
> - info->inserted->iops = cfg.buckets[THROTTLE_OPS_TOTAL].avg;
> - info->inserted->iops_rd = cfg.buckets[THROTTLE_OPS_READ].avg;
> - info->inserted->iops_wr = cfg.buckets[THROTTLE_OPS_WRITE].avg;
> -
> - info->inserted->has_bps_max =
> - cfg.buckets[THROTTLE_BPS_TOTAL].max;
> - info->inserted->bps_max =
> - cfg.buckets[THROTTLE_BPS_TOTAL].max;
> - info->inserted->has_bps_rd_max =
> - cfg.buckets[THROTTLE_BPS_READ].max;
> - info->inserted->bps_rd_max =
> - cfg.buckets[THROTTLE_BPS_READ].max;
> - info->inserted->has_bps_wr_max =
> - cfg.buckets[THROTTLE_BPS_WRITE].max;
> - info->inserted->bps_wr_max =
> - cfg.buckets[THROTTLE_BPS_WRITE].max;
> -
> - info->inserted->has_iops_max =
> - cfg.buckets[THROTTLE_OPS_TOTAL].max;
> - info->inserted->iops_max =
> - cfg.buckets[THROTTLE_OPS_TOTAL].max;
> - info->inserted->has_iops_rd_max =
> - cfg.buckets[THROTTLE_OPS_READ].max;
> - info->inserted->iops_rd_max =
> - cfg.buckets[THROTTLE_OPS_READ].max;
> - info->inserted->has_iops_wr_max =
> - cfg.buckets[THROTTLE_OPS_WRITE].max;
> - info->inserted->iops_wr_max =
> - cfg.buckets[THROTTLE_OPS_WRITE].max;
> -
> - info->inserted->has_iops_size = cfg.op_size;
> - info->inserted->iops_size = cfg.op_size;
> - }
> + info->inserted = bdrv_block_device_info(bs);
>
> bs0 = bs;
> p_image_info = &info->inserted->image;
> diff --git a/blockdev.c b/blockdev.c
> index 44755e1..204ab40 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1940,6 +1940,11 @@ void qmp_drive_backup(const char *device, const char *target,
> }
> }
>
> +BlockDeviceInfoList *qmp_query_named_block_nodes(Error **errp)
> +{
> + return bdrv_named_nodes_list();
> +}
> +
> #define DEFAULT_MIRROR_BUF_SIZE (10 << 20)
>
> void qmp_drive_mirror(const char *device, const char *target,
> diff --git a/include/block/block.h b/include/block/block.h
> index 834abf9..8c10123 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -375,6 +375,7 @@ void bdrv_eject(BlockDriverState *bs, bool eject_flag);
> const char *bdrv_get_format_name(BlockDriverState *bs);
> BlockDriverState *bdrv_find(const char *name);
> BlockDriverState *bdrv_find_node(const char *node_name);
> +BlockDeviceInfoList *bdrv_named_nodes_list(void);
> BlockDriverState *bdrv_next(BlockDriverState *bs);
> void bdrv_iterate(void (*it)(void *opaque, BlockDriverState *bs),
> void *opaque);
> diff --git a/include/block/qapi.h b/include/block/qapi.h
> index 9518ee4..e92c00d 100644
> --- a/include/block/qapi.h
> +++ b/include/block/qapi.h
> @@ -29,6 +29,7 @@
> #include "block/block.h"
> #include "block/snapshot.h"
>
> +BlockDeviceInfo *bdrv_block_device_info(BlockDriverState *bs);
> int bdrv_query_snapshot_info_list(BlockDriverState *bs,
> SnapshotInfoList **p_list,
> Error **errp);
> diff --git a/qapi-schema.json b/qapi-schema.json
> index c3c939c..0dadd5d 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -810,6 +810,8 @@
> #
> # @file: the filename of the backing device
> #
> +# @node-name: #optional the name of the block driver node (Since 2.0)
> +#
> # @ro: true if the backing device was open read-only
> #
> # @drv: the name of the block format used to open the backing device. As of
> @@ -857,10 +859,9 @@
> #
> # Since: 0.14.0
> #
> -# Notes: This interface is only found in @BlockInfo.
> ##
> { 'type': 'BlockDeviceInfo',
> - 'data': { 'file': 'str', 'ro': 'bool', 'drv': 'str',
> + 'data': { 'file': 'str', '*node-name': 'str', 'ro': 'bool', 'drv': 'str',
> '*backing_file': 'str', 'backing_file_depth': 'int',
> 'encrypted': 'bool', 'encryption_key_missing': 'bool',
> 'bps': 'int', 'bps_rd': 'int', 'bps_wr': 'int',
> @@ -2008,6 +2009,17 @@
> { 'command': 'drive-backup', 'data': 'DriveBackup' }
>
> ##
> +# @query-named-block-nodes
> +#
> +# Get the named block driver list
> +#
> +# Returns: the list of BlockDeviceInfo
> +#
> +# Since 2.0
> +##
> +{ 'command': 'query-named-block-nodes', 'returns': [ 'BlockDeviceInfo' ] }
> +
> +##
> # @drive-mirror
> #
> # Start mirroring a block device's writes to a new destination.
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index fba15cd..d644fe9 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -3295,3 +3295,64 @@ Example (2):
> <- { "return": {} }
>
> EQMP
> +
> + {
> + .name = "query-named-block-nodes",
> + .args_type = "",
> + .mhandler.cmd_new = qmp_marshal_input_query_named_block_nodes,
> + },
> +
> +SQMP
> +@query-named-block-nodes
> +------------------------
> +
> +Return a list of BlockDeviceInfo for each named block driver node
How about "Return a list of BlockDeviceInfo for all the named block driver nodes"
But nothing to stop adding
Reviewed-by: Fam Zheng <famz@redhat.com>
> +
> +Example:
> +
> +-> { "execute": "query-named-block-nodes" }
> +<- { "return": [ { "ro":false,
> + "drv":"qcow2",
> + "encrypted":false,
> + "file":"disks/test.qcow2",
> + "node-name": "my-node",
> + "backing_file_depth":1,
> + "bps":1000000,
> + "bps_rd":0,
> + "bps_wr":0,
> + "iops":1000000,
> + "iops_rd":0,
> + "iops_wr":0,
> + "bps_max": 8000000,
> + "bps_rd_max": 0,
> + "bps_wr_max": 0,
> + "iops_max": 0,
> + "iops_rd_max": 0,
> + "iops_wr_max": 0,
> + "iops_size": 0,
> + "image":{
> + "filename":"disks/test.qcow2",
> + "format":"qcow2",
> + "virtual-size":2048000,
> + "backing_file":"base.qcow2",
> + "full-backing-filename":"disks/base.qcow2",
> + "backing-filename-format:"qcow2",
> + "snapshots":[
> + {
> + "id": "1",
> + "name": "snapshot1",
> + "vm-state-size": 0,
> + "date-sec": 10000200,
> + "date-nsec": 12,
> + "vm-clock-sec": 206,
> + "vm-clock-nsec": 30
> + }
> + ],
> + "backing-image":{
> + "filename":"disks/base.qcow2",
> + "format":"qcow2",
> + "virtual-size":2048000
> + }
> + } } ] }
> +
> +EQMP
> --
> 1.8.3.2
>
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH V5 4/7] qmp: Allow to change password on named block driver states.
2013-12-12 15:33 [Qemu-devel] [PATCH V5 0/7] Giving names to BlockDriverState graph nodes Benoît Canet
` (2 preceding siblings ...)
2013-12-12 15:33 ` [Qemu-devel] [PATCH V5 3/7] qmp: Add a command to list the named BlockDriverState nodes Benoît Canet
@ 2013-12-12 15:33 ` Benoît Canet
2014-01-21 3:27 ` Fam Zheng
2014-01-21 14:17 ` Kevin Wolf
2013-12-12 15:33 ` [Qemu-devel] [PATCH V5 5/7] block: Create authorizations mechanism for external snapshot and resize Benoît Canet
` (5 subsequent siblings)
9 siblings, 2 replies; 28+ messages in thread
From: Benoît Canet @ 2013-12-12 15:33 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, Benoît Canet, jcody, armbru, stefanha
There was two candidate ways to implement named node manipulation:
1)
{ 'command': 'block_passwd', 'data': {'*device': 'str',
'*node-name': 'str', 'password': 'str'}
}
2)
{ 'command': 'block_passwd', 'data': {'device': 'str',
'*device-is-node': 'bool',
'password': 'str'} }
Luiz proposed 1 and says 2 was an abuse of the QMP interface and proposed to
rewrite the QMP block interface for 2.0.
Luiz does not like in 1 the fact that 2 fields are optional but one of them must
be specified leading to an abuse of the QMP semantic.
Kevin argumented that 2 what a clear abuse of the device field and would not be
practical when reading fast some log file because the user would read "device"
and think that a device is manipulated when it's in fact a node name.
Documentation of 1 make it pretty clear what to do for the user.
Kevin argued that all bs are node including devices ones so 2 does not make
sense.
Kevin also argued that rewriting the QMP block interface would not make disapear
the current one.
Kevin pushed the argument that making the QAPI generator compatible with the
semantic of the operation would need a rewrite that no one has done yet.
A vote has been done on the list to elect the version to use and 1 won.
For reference the complete thread is:
"[Qemu-devel] [PATCH V4 4/7] qmp: Allow to change password on names block driver
states."
Signed-off-by: Benoit Canet <benoit@irqsave.net>
---
block.c | 32 ++++++++++++++++++++++++++++++++
blockdev.c | 13 +++++++++----
hmp.c | 2 +-
include/block/block.h | 3 +++
qapi-schema.json | 9 +++++++--
qmp-commands.hx | 3 ++-
6 files changed, 54 insertions(+), 8 deletions(-)
diff --git a/block.c b/block.c
index 78d13e5..22190a4 100644
--- a/block.c
+++ b/block.c
@@ -3207,6 +3207,38 @@ BlockDeviceInfoList *bdrv_named_nodes_list(void)
return list;
}
+BlockDriverState *bdrv_lookup_bs(bool has_device, const char *device,
+ bool has_node_name, const char *node_name,
+ Error **errp)
+{
+ BlockDriverState *bs = NULL;
+
+ if (has_device == has_node_name) {
+ error_setg(errp, "Use either device or node-name but not both");
+ return NULL;
+ }
+
+ if (has_device) {
+ bs = bdrv_find(device);
+
+ if (!bs) {
+ error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+ return NULL;
+ }
+
+ return bs;
+ }
+
+ bs = bdrv_find_node(node_name);
+
+ if (!bs) {
+ error_set(errp, QERR_DEVICE_NOT_FOUND, node_name);
+ return NULL;
+ }
+
+ return bs;
+}
+
BlockDriverState *bdrv_next(BlockDriverState *bs)
{
if (!bs) {
diff --git a/blockdev.c b/blockdev.c
index 204ab40..838df50 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1474,14 +1474,19 @@ void qmp_eject(const char *device, bool has_force, bool force, Error **errp)
eject_device(bs, force, errp);
}
-void qmp_block_passwd(const char *device, const char *password, Error **errp)
+void qmp_block_passwd(bool has_device, const char *device,
+ bool has_node_name, const char *node_name,
+ const char *password, Error **errp)
{
+ Error *local_err = NULL;
BlockDriverState *bs;
int err;
- bs = bdrv_find(device);
- if (!bs) {
- error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+ bs = bdrv_lookup_bs(has_device, device,
+ has_node_name, node_name,
+ &local_err);
+ if (error_is_set(&local_err)) {
+ error_propagate(errp, local_err);
return;
}
diff --git a/hmp.c b/hmp.c
index 32ee285..3820fbe 100644
--- a/hmp.c
+++ b/hmp.c
@@ -870,7 +870,7 @@ void hmp_block_passwd(Monitor *mon, const QDict *qdict)
const char *password = qdict_get_str(qdict, "password");
Error *errp = NULL;
- qmp_block_passwd(device, password, &errp);
+ qmp_block_passwd(true, device, false, NULL, password, &errp);
hmp_handle_error(mon, &errp);
}
diff --git a/include/block/block.h b/include/block/block.h
index 8c10123..f7d8017 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -376,6 +376,9 @@ const char *bdrv_get_format_name(BlockDriverState *bs);
BlockDriverState *bdrv_find(const char *name);
BlockDriverState *bdrv_find_node(const char *node_name);
BlockDeviceInfoList *bdrv_named_nodes_list(void);
+BlockDriverState *bdrv_lookup_bs(bool has_device, const char *device,
+ bool has_node_name, const char *node_name,
+ Error **errp);
BlockDriverState *bdrv_next(BlockDriverState *bs);
void bdrv_iterate(void (*it)(void *opaque, BlockDriverState *bs),
void *opaque);
diff --git a/qapi-schema.json b/qapi-schema.json
index 0dadd5d..903fcb6 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1676,7 +1676,11 @@
# determine which ones are encrypted, set the passwords with this command, and
# then start the guest with the @cont command.
#
-# @device: the name of the device to set the password on
+# Either @device or @node-name must be set but not both.
+#
+# @device: #optional the name of the block backend device to set the password on
+#
+# @node-name: #optional graph node name to set the password on (Since 2.0)
#
# @password: the password to use for the device
#
@@ -1690,7 +1694,8 @@
#
# Since: 0.14.0
##
-{ 'command': 'block_passwd', 'data': {'device': 'str', 'password': 'str'} }
+{ 'command': 'block_passwd', 'data': {'*device': 'str',
+ '*node-name': 'str', 'password': 'str'} }
##
# @balloon:
diff --git a/qmp-commands.hx b/qmp-commands.hx
index d644fe9..1451c1a 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1452,7 +1452,7 @@ EQMP
{
.name = "block_passwd",
- .args_type = "device:B,password:s",
+ .args_type = "device:s?,node-name:s?,password:s",
.mhandler.cmd_new = qmp_marshal_input_block_passwd,
},
@@ -1465,6 +1465,7 @@ Set the password of encrypted block devices.
Arguments:
- "device": device name (json-string)
+- "node-name": name in the block driver state graph (json-string)
- "password": password (json-string)
Example:
--
1.8.3.2
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH V5 4/7] qmp: Allow to change password on named block driver states.
2013-12-12 15:33 ` [Qemu-devel] [PATCH V5 4/7] qmp: Allow to change password on named block driver states Benoît Canet
@ 2014-01-21 3:27 ` Fam Zheng
2014-01-21 14:17 ` Kevin Wolf
1 sibling, 0 replies; 28+ messages in thread
From: Fam Zheng @ 2014-01-21 3:27 UTC (permalink / raw)
To: Benoît Canet; +Cc: kwolf, jcody, qemu-devel, stefanha, armbru
On Thu, 12/12 16:33, Benoît Canet wrote:
> There was two candidate ways to implement named node manipulation:
>
> 1)
> { 'command': 'block_passwd', 'data': {'*device': 'str',
> '*node-name': 'str', 'password': 'str'}
> }
>
> 2)
>
> { 'command': 'block_passwd', 'data': {'device': 'str',
> '*device-is-node': 'bool',
> 'password': 'str'} }
>
> Luiz proposed 1 and says 2 was an abuse of the QMP interface and proposed to
> rewrite the QMP block interface for 2.0.
>
> Luiz does not like in 1 the fact that 2 fields are optional but one of them must
> be specified leading to an abuse of the QMP semantic.
>
> Kevin argumented that 2 what a clear abuse of the device field and would not be
> practical when reading fast some log file because the user would read "device"
> and think that a device is manipulated when it's in fact a node name.
> Documentation of 1 make it pretty clear what to do for the user.
>
> Kevin argued that all bs are node including devices ones so 2 does not make
> sense.
>
> Kevin also argued that rewriting the QMP block interface would not make disapear
> the current one.
>
> Kevin pushed the argument that making the QAPI generator compatible with the
> semantic of the operation would need a rewrite that no one has done yet.
>
> A vote has been done on the list to elect the version to use and 1 won.
>
> For reference the complete thread is:
> "[Qemu-devel] [PATCH V4 4/7] qmp: Allow to change password on names block driver
> states."
>
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
> block.c | 32 ++++++++++++++++++++++++++++++++
> blockdev.c | 13 +++++++++----
> hmp.c | 2 +-
> include/block/block.h | 3 +++
> qapi-schema.json | 9 +++++++--
> qmp-commands.hx | 3 ++-
> 6 files changed, 54 insertions(+), 8 deletions(-)
>
> diff --git a/block.c b/block.c
> index 78d13e5..22190a4 100644
> --- a/block.c
> +++ b/block.c
> @@ -3207,6 +3207,38 @@ BlockDeviceInfoList *bdrv_named_nodes_list(void)
> return list;
> }
>
> +BlockDriverState *bdrv_lookup_bs(bool has_device, const char *device,
> + bool has_node_name, const char *node_name,
> + Error **errp)
> +{
> + BlockDriverState *bs = NULL;
> +
> + if (has_device == has_node_name) {
> + error_setg(errp, "Use either device or node-name but not both");
> + return NULL;
> + }
> +
> + if (has_device) {
> + bs = bdrv_find(device);
> +
> + if (!bs) {
> + error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> + return NULL;
> + }
> +
> + return bs;
> + }
> +
> + bs = bdrv_find_node(node_name);
> +
> + if (!bs) {
> + error_set(errp, QERR_DEVICE_NOT_FOUND, node_name);
> + return NULL;
> + }
> +
> + return bs;
> +}
> +
> BlockDriverState *bdrv_next(BlockDriverState *bs)
> {
> if (!bs) {
> diff --git a/blockdev.c b/blockdev.c
> index 204ab40..838df50 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1474,14 +1474,19 @@ void qmp_eject(const char *device, bool has_force, bool force, Error **errp)
> eject_device(bs, force, errp);
> }
>
> -void qmp_block_passwd(const char *device, const char *password, Error **errp)
> +void qmp_block_passwd(bool has_device, const char *device,
> + bool has_node_name, const char *node_name,
> + const char *password, Error **errp)
> {
> + Error *local_err = NULL;
> BlockDriverState *bs;
> int err;
>
> - bs = bdrv_find(device);
> - if (!bs) {
> - error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> + bs = bdrv_lookup_bs(has_device, device,
> + has_node_name, node_name,
> + &local_err);
> + if (error_is_set(&local_err)) {
> + error_propagate(errp, local_err);
> return;
> }
>
> diff --git a/hmp.c b/hmp.c
> index 32ee285..3820fbe 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -870,7 +870,7 @@ void hmp_block_passwd(Monitor *mon, const QDict *qdict)
> const char *password = qdict_get_str(qdict, "password");
> Error *errp = NULL;
>
> - qmp_block_passwd(device, password, &errp);
> + qmp_block_passwd(true, device, false, NULL, password, &errp);
> hmp_handle_error(mon, &errp);
> }
>
> diff --git a/include/block/block.h b/include/block/block.h
> index 8c10123..f7d8017 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -376,6 +376,9 @@ const char *bdrv_get_format_name(BlockDriverState *bs);
> BlockDriverState *bdrv_find(const char *name);
> BlockDriverState *bdrv_find_node(const char *node_name);
> BlockDeviceInfoList *bdrv_named_nodes_list(void);
> +BlockDriverState *bdrv_lookup_bs(bool has_device, const char *device,
> + bool has_node_name, const char *node_name,
> + Error **errp);
> BlockDriverState *bdrv_next(BlockDriverState *bs);
> void bdrv_iterate(void (*it)(void *opaque, BlockDriverState *bs),
> void *opaque);
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 0dadd5d..903fcb6 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1676,7 +1676,11 @@
> # determine which ones are encrypted, set the passwords with this command, and
> # then start the guest with the @cont command.
> #
> -# @device: the name of the device to set the password on
> +# Either @device or @node-name must be set but not both.
> +#
> +# @device: #optional the name of the block backend device to set the password on
> +#
> +# @node-name: #optional graph node name to set the password on (Since 2.0)
> #
> # @password: the password to use for the device
> #
> @@ -1690,7 +1694,8 @@
> #
> # Since: 0.14.0
> ##
> -{ 'command': 'block_passwd', 'data': {'device': 'str', 'password': 'str'} }
> +{ 'command': 'block_passwd', 'data': {'*device': 'str',
> + '*node-name': 'str', 'password': 'str'} }
>
> ##
> # @balloon:
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index d644fe9..1451c1a 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -1452,7 +1452,7 @@ EQMP
>
> {
> .name = "block_passwd",
> - .args_type = "device:B,password:s",
> + .args_type = "device:s?,node-name:s?,password:s",
> .mhandler.cmd_new = qmp_marshal_input_block_passwd,
> },
>
> @@ -1465,6 +1465,7 @@ Set the password of encrypted block devices.
> Arguments:
>
> - "device": device name (json-string)
> +- "node-name": name in the block driver state graph (json-string)
> - "password": password (json-string)
>
> Example:
> --
> 1.8.3.2
>
>
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH V5 4/7] qmp: Allow to change password on named block driver states.
2013-12-12 15:33 ` [Qemu-devel] [PATCH V5 4/7] qmp: Allow to change password on named block driver states Benoît Canet
2014-01-21 3:27 ` Fam Zheng
@ 2014-01-21 14:17 ` Kevin Wolf
1 sibling, 0 replies; 28+ messages in thread
From: Kevin Wolf @ 2014-01-21 14:17 UTC (permalink / raw)
To: Benoît Canet; +Cc: famz, jcody, qemu-devel, armbru, stefanha
Am 12.12.2013 um 16:33 hat Benoît Canet geschrieben:
> There was two candidate ways to implement named node manipulation:
>
> 1)
> { 'command': 'block_passwd', 'data': {'*device': 'str',
> '*node-name': 'str', 'password': 'str'}
> }
>
> 2)
>
> { 'command': 'block_passwd', 'data': {'device': 'str',
> '*device-is-node': 'bool',
> 'password': 'str'} }
>
> Luiz proposed 1 and says 2 was an abuse of the QMP interface and proposed to
> rewrite the QMP block interface for 2.0.
>
> Luiz does not like in 1 the fact that 2 fields are optional but one of them must
> be specified leading to an abuse of the QMP semantic.
>
> Kevin argumented that 2 what a clear abuse of the device field and would not be
> practical when reading fast some log file because the user would read "device"
> and think that a device is manipulated when it's in fact a node name.
> Documentation of 1 make it pretty clear what to do for the user.
>
> Kevin argued that all bs are node including devices ones so 2 does not make
> sense.
>
> Kevin also argued that rewriting the QMP block interface would not make disapear
> the current one.
>
> Kevin pushed the argument that making the QAPI generator compatible with the
> semantic of the operation would need a rewrite that no one has done yet.
>
> A vote has been done on the list to elect the version to use and 1 won.
>
> For reference the complete thread is:
> "[Qemu-devel] [PATCH V4 4/7] qmp: Allow to change password on names block driver
> states."
>
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
> block.c | 32 ++++++++++++++++++++++++++++++++
> blockdev.c | 13 +++++++++----
> hmp.c | 2 +-
> include/block/block.h | 3 +++
> qapi-schema.json | 9 +++++++--
> qmp-commands.hx | 3 ++-
> 6 files changed, 54 insertions(+), 8 deletions(-)
>
> diff --git a/block.c b/block.c
> index 78d13e5..22190a4 100644
> --- a/block.c
> +++ b/block.c
> @@ -3207,6 +3207,38 @@ BlockDeviceInfoList *bdrv_named_nodes_list(void)
> return list;
> }
>
> +BlockDriverState *bdrv_lookup_bs(bool has_device, const char *device,
> + bool has_node_name, const char *node_name,
> + Error **errp)
This is a normal function without generated callers, so can we get rid
of has_device/has_node_name and just switch to passing NULL if it's not
present? That would make it more convenient to use this function in
other places.
Kevin
^ permalink raw reply [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH V5 5/7] block: Create authorizations mechanism for external snapshot and resize.
2013-12-12 15:33 [Qemu-devel] [PATCH V5 0/7] Giving names to BlockDriverState graph nodes Benoît Canet
` (3 preceding siblings ...)
2013-12-12 15:33 ` [Qemu-devel] [PATCH V5 4/7] qmp: Allow to change password on named block driver states Benoît Canet
@ 2013-12-12 15:33 ` Benoît Canet
2014-01-21 3:45 ` Fam Zheng
2013-12-12 15:33 ` [Qemu-devel] [PATCH V5 6/7] qmp: Allow block_resize to manipulate bs graph nodes Benoît Canet
` (4 subsequent siblings)
9 siblings, 1 reply; 28+ messages in thread
From: Benoît Canet @ 2013-12-12 15:33 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, Benoît Canet, jcody, armbru, stefanha
Signed-off-by: Benoit Canet <benoit@irqsave.net>
---
block.c | 65 ++++++++++++++++++++++++++++++++++++++++-------
block/blkverify.c | 2 +-
blockdev.c | 2 +-
include/block/block.h | 20 +++++++--------
include/block/block_int.h | 12 ++++++---
5 files changed, 77 insertions(+), 24 deletions(-)
diff --git a/block.c b/block.c
index 22190a4..57946b7 100644
--- a/block.c
+++ b/block.c
@@ -4992,21 +4992,68 @@ int bdrv_amend_options(BlockDriverState *bs, QEMUOptionParameter *options)
return bs->drv->bdrv_amend_options(bs, options);
}
-ExtSnapshotPerm bdrv_check_ext_snapshot(BlockDriverState *bs)
+/* Used to recurse on single child block filters.
+ * Single child block filter will store their child in bs->file.
+ */
+bool bdrv_generic_is_first_non_filter(BlockDriverState *bs,
+ BlockDriverState *candidate)
{
- if (bs->drv->bdrv_check_ext_snapshot) {
- return bs->drv->bdrv_check_ext_snapshot(bs);
+ if (!bs->drv) {
+ return false;
+ }
+
+ if (!bs->drv->authorizations[BS_IS_A_FILTER]) {
+ if (bs == candidate) {
+ return true;
+ } else {
+ return false;
+ }
+ }
+
+ if (!bs->drv->authorizations[BS_FILTER_PASS_DOWN]) {
+ return false;
}
- if (bs->file && bs->file->drv && bs->file->drv->bdrv_check_ext_snapshot) {
- return bs->file->drv->bdrv_check_ext_snapshot(bs);
+ if (!bs->file) {
+ return false;
+ }
+
+ return bdrv_recurse_is_first_non_filter(bs->file, candidate);
+}
+
+bool bdrv_recurse_is_first_non_filter(BlockDriverState *bs,
+ BlockDriverState *candidate)
+{
+ if (bs->drv && bs->drv->bdrv_recurse_is_first_non_filter) {
+ return bs->drv->bdrv_recurse_is_first_non_filter(bs, candidate);
}
- /* external snapshots are allowed by default */
- return EXT_SNAPSHOT_ALLOWED;
+ return bdrv_generic_is_first_non_filter(bs, candidate);
}
-ExtSnapshotPerm bdrv_check_ext_snapshot_forbidden(BlockDriverState *bs)
+/* This function check if the candidate is the first non filter bs down it's
+ * bs chain. Since we don't have pointers to parents it explore all bs chains
+ * from the top. Some filters can choose not to pass down the recursion.
+ */
+bool bdrv_is_first_non_filter(BlockDriverState *candidate)
{
- return EXT_SNAPSHOT_FORBIDDEN;
+ BlockDriverState *bs;
+
+ /* walk down the bs forest recursively */
+ QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
+ bool perm;
+
+ if (!bs->file) {
+ continue;
+ }
+
+ perm = bdrv_recurse_is_first_non_filter(bs->file, candidate);
+
+ /* candidate is the first non filter */
+ if (perm) {
+ return true;
+ }
+ }
+
+ return false;
}
diff --git a/block/blkverify.c b/block/blkverify.c
index 3c63528..853afa9 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -417,7 +417,7 @@ static BlockDriver bdrv_blkverify = {
.bdrv_aio_writev = blkverify_aio_writev,
.bdrv_aio_flush = blkverify_aio_flush,
- .bdrv_check_ext_snapshot = bdrv_check_ext_snapshot_forbidden,
+ .authorizations = { true, false },
};
static void bdrv_blkverify_init(void)
diff --git a/blockdev.c b/blockdev.c
index 838df50..ebb8f48 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1236,7 +1236,7 @@ static void external_snapshot_prepare(BlkTransactionState *common,
}
}
- if (bdrv_check_ext_snapshot(state->old_bs) != EXT_SNAPSHOT_ALLOWED) {
+ if (!bdrv_is_first_non_filter(state->old_bs)) {
error_set(errp, QERR_FEATURE_DISABLED, "snapshot");
return;
}
diff --git a/include/block/block.h b/include/block/block.h
index f7d8017..16812b0 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -283,16 +283,16 @@ int bdrv_amend_options(BlockDriverState *bs_new, QEMUOptionParameter *options);
/* external snapshots */
typedef enum {
- EXT_SNAPSHOT_ALLOWED,
- EXT_SNAPSHOT_FORBIDDEN,
-} ExtSnapshotPerm;
-
-/* return EXT_SNAPSHOT_ALLOWED if external snapshot is allowed
- * return EXT_SNAPSHOT_FORBIDDEN if external snapshot is forbidden
- */
-ExtSnapshotPerm bdrv_check_ext_snapshot(BlockDriverState *bs);
-/* helper used to forbid external snapshots like in blkverify */
-ExtSnapshotPerm bdrv_check_ext_snapshot_forbidden(BlockDriverState *bs);
+ BS_IS_A_FILTER,
+ BS_FILTER_PASS_DOWN,
+ BS_AUTHORIZATION_COUNT,
+} BsAuthorization;
+
+bool bdrv_generic_is_first_non_filter(BlockDriverState *bs,
+ BlockDriverState *candidate);
+bool bdrv_recurse_is_first_non_filter(BlockDriverState *bs,
+ BlockDriverState *candidate);
+bool bdrv_is_first_non_filter(BlockDriverState *candidate);
/* async block I/O */
typedef void BlockDriverDirtyHandler(BlockDriverState *bs, int64_t sector,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index bd5220f..7933ecc 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -69,10 +69,16 @@ struct BlockDriver {
const char *format_name;
int instance_size;
- /* if not defined external snapshots are allowed
- * future block filters will query their children to build the response
+ /* this table of boolean contains authorizations for the block operations */
+ bool authorizations[BS_AUTHORIZATION_COUNT];
+ /* for snapshots complex block filter like Quorum can implement the
+ * following recursive callback instead of BS_IS_A_FILTER.
+ * It's purpose is to recurse on the filter children while calling
+ * bdrv_recurse_is_first_non_filter on them.
+ * For a sample implentation look in the future Quorum block filter.
*/
- ExtSnapshotPerm (*bdrv_check_ext_snapshot)(BlockDriverState *bs);
+ bool (*bdrv_recurse_is_first_non_filter)(BlockDriverState *bs,
+ BlockDriverState *candidate);
int (*bdrv_probe)(const uint8_t *buf, int buf_size, const char *filename);
int (*bdrv_probe_device)(const char *filename);
--
1.8.3.2
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH V5 5/7] block: Create authorizations mechanism for external snapshot and resize.
2013-12-12 15:33 ` [Qemu-devel] [PATCH V5 5/7] block: Create authorizations mechanism for external snapshot and resize Benoît Canet
@ 2014-01-21 3:45 ` Fam Zheng
0 siblings, 0 replies; 28+ messages in thread
From: Fam Zheng @ 2014-01-21 3:45 UTC (permalink / raw)
To: Benoît Canet; +Cc: kwolf, jcody, qemu-devel, stefanha, armbru
On Thu, 12/12 16:33, Benoît Canet wrote:
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
> block.c | 65 ++++++++++++++++++++++++++++++++++++++++-------
> block/blkverify.c | 2 +-
> blockdev.c | 2 +-
> include/block/block.h | 20 +++++++--------
> include/block/block_int.h | 12 ++++++---
> 5 files changed, 77 insertions(+), 24 deletions(-)
>
> diff --git a/block.c b/block.c
> index 22190a4..57946b7 100644
> --- a/block.c
> +++ b/block.c
> @@ -4992,21 +4992,68 @@ int bdrv_amend_options(BlockDriverState *bs, QEMUOptionParameter *options)
> return bs->drv->bdrv_amend_options(bs, options);
> }
>
> -ExtSnapshotPerm bdrv_check_ext_snapshot(BlockDriverState *bs)
> +/* Used to recurse on single child block filters.
> + * Single child block filter will store their child in bs->file.
> + */
> +bool bdrv_generic_is_first_non_filter(BlockDriverState *bs,
> + BlockDriverState *candidate)
> {
> - if (bs->drv->bdrv_check_ext_snapshot) {
> - return bs->drv->bdrv_check_ext_snapshot(bs);
> + if (!bs->drv) {
> + return false;
> + }
> +
> + if (!bs->drv->authorizations[BS_IS_A_FILTER]) {
> + if (bs == candidate) {
> + return true;
> + } else {
> + return false;
> + }
> + }
> +
> + if (!bs->drv->authorizations[BS_FILTER_PASS_DOWN]) {
> + return false;
> }
>
> - if (bs->file && bs->file->drv && bs->file->drv->bdrv_check_ext_snapshot) {
> - return bs->file->drv->bdrv_check_ext_snapshot(bs);
> + if (!bs->file) {
> + return false;
> + }
> +
> + return bdrv_recurse_is_first_non_filter(bs->file, candidate);
> +}
> +
> +bool bdrv_recurse_is_first_non_filter(BlockDriverState *bs,
> + BlockDriverState *candidate)
> +{
> + if (bs->drv && bs->drv->bdrv_recurse_is_first_non_filter) {
> + return bs->drv->bdrv_recurse_is_first_non_filter(bs, candidate);
> }
>
> - /* external snapshots are allowed by default */
> - return EXT_SNAPSHOT_ALLOWED;
> + return bdrv_generic_is_first_non_filter(bs, candidate);
> }
>
> -ExtSnapshotPerm bdrv_check_ext_snapshot_forbidden(BlockDriverState *bs)
> +/* This function check if the candidate is the first non filter bs down it's
s/check/checks/
> + * bs chain. Since we don't have pointers to parents it explore all bs chains
> + * from the top. Some filters can choose not to pass down the recursion.
> + */
> +bool bdrv_is_first_non_filter(BlockDriverState *candidate)
> {
> - return EXT_SNAPSHOT_FORBIDDEN;
> + BlockDriverState *bs;
> +
> + /* walk down the bs forest recursively */
> + QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> + bool perm;
> +
> + if (!bs->file) {
> + continue;
> + }
> +
> + perm = bdrv_recurse_is_first_non_filter(bs->file, candidate);
> +
> + /* candidate is the first non filter */
> + if (perm) {
> + return true;
> + }
> + }
> +
> + return false;
> }
> diff --git a/block/blkverify.c b/block/blkverify.c
> index 3c63528..853afa9 100644
> --- a/block/blkverify.c
> +++ b/block/blkverify.c
> @@ -417,7 +417,7 @@ static BlockDriver bdrv_blkverify = {
> .bdrv_aio_writev = blkverify_aio_writev,
> .bdrv_aio_flush = blkverify_aio_flush,
>
> - .bdrv_check_ext_snapshot = bdrv_check_ext_snapshot_forbidden,
> + .authorizations = { true, false },
> };
>
> static void bdrv_blkverify_init(void)
> diff --git a/blockdev.c b/blockdev.c
> index 838df50..ebb8f48 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1236,7 +1236,7 @@ static void external_snapshot_prepare(BlkTransactionState *common,
> }
> }
>
> - if (bdrv_check_ext_snapshot(state->old_bs) != EXT_SNAPSHOT_ALLOWED) {
> + if (!bdrv_is_first_non_filter(state->old_bs)) {
> error_set(errp, QERR_FEATURE_DISABLED, "snapshot");
> return;
> }
> diff --git a/include/block/block.h b/include/block/block.h
> index f7d8017..16812b0 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -283,16 +283,16 @@ int bdrv_amend_options(BlockDriverState *bs_new, QEMUOptionParameter *options);
> /* external snapshots */
>
> typedef enum {
> - EXT_SNAPSHOT_ALLOWED,
> - EXT_SNAPSHOT_FORBIDDEN,
> -} ExtSnapshotPerm;
> -
> -/* return EXT_SNAPSHOT_ALLOWED if external snapshot is allowed
> - * return EXT_SNAPSHOT_FORBIDDEN if external snapshot is forbidden
> - */
> -ExtSnapshotPerm bdrv_check_ext_snapshot(BlockDriverState *bs);
> -/* helper used to forbid external snapshots like in blkverify */
> -ExtSnapshotPerm bdrv_check_ext_snapshot_forbidden(BlockDriverState *bs);
> + BS_IS_A_FILTER,
> + BS_FILTER_PASS_DOWN,
> + BS_AUTHORIZATION_COUNT,
> +} BsAuthorization;
> +
> +bool bdrv_generic_is_first_non_filter(BlockDriverState *bs,
> + BlockDriverState *candidate);
> +bool bdrv_recurse_is_first_non_filter(BlockDriverState *bs,
> + BlockDriverState *candidate);
> +bool bdrv_is_first_non_filter(BlockDriverState *candidate);
>
> /* async block I/O */
> typedef void BlockDriverDirtyHandler(BlockDriverState *bs, int64_t sector,
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index bd5220f..7933ecc 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -69,10 +69,16 @@ struct BlockDriver {
> const char *format_name;
> int instance_size;
>
> - /* if not defined external snapshots are allowed
> - * future block filters will query their children to build the response
> + /* this table of boolean contains authorizations for the block operations */
> + bool authorizations[BS_AUTHORIZATION_COUNT];
> + /* for snapshots complex block filter like Quorum can implement the
> + * following recursive callback instead of BS_IS_A_FILTER.
> + * It's purpose is to recurse on the filter children while calling
> + * bdrv_recurse_is_first_non_filter on them.
> + * For a sample implentation look in the future Quorum block filter.
s/implentation/implementation/
> */
> - ExtSnapshotPerm (*bdrv_check_ext_snapshot)(BlockDriverState *bs);
> + bool (*bdrv_recurse_is_first_non_filter)(BlockDriverState *bs,
> + BlockDriverState *candidate);
>
> int (*bdrv_probe)(const uint8_t *buf, int buf_size, const char *filename);
> int (*bdrv_probe_device)(const char *filename);
> --
> 1.8.3.2
>
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH V5 6/7] qmp: Allow block_resize to manipulate bs graph nodes.
2013-12-12 15:33 [Qemu-devel] [PATCH V5 0/7] Giving names to BlockDriverState graph nodes Benoît Canet
` (4 preceding siblings ...)
2013-12-12 15:33 ` [Qemu-devel] [PATCH V5 5/7] block: Create authorizations mechanism for external snapshot and resize Benoît Canet
@ 2013-12-12 15:33 ` Benoît Canet
2014-01-21 3:46 ` Fam Zheng
2013-12-12 15:34 ` [Qemu-devel] [PATCH V5 7/7] qmp: Allow to take external snapshots on bs graphs node Benoît Canet
` (3 subsequent siblings)
9 siblings, 1 reply; 28+ messages in thread
From: Benoît Canet @ 2013-12-12 15:33 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, Benoît Canet, jcody, armbru, stefanha
Signed-off-by: Benoit Canet <benoit@irqsave.net>
---
blockdev.c | 18 ++++++++++++++----
hmp.c | 2 +-
qapi-schema.json | 10 ++++++++--
qmp-commands.hx | 3 ++-
4 files changed, 25 insertions(+), 8 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index ebb8f48..374d03d 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1676,14 +1676,24 @@ int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
return 0;
}
-void qmp_block_resize(const char *device, int64_t size, Error **errp)
+void qmp_block_resize(bool has_device, const char *device,
+ bool has_node_name, const char *node_name,
+ int64_t size, Error **errp)
{
+ Error *local_err = NULL;
BlockDriverState *bs;
int ret;
- bs = bdrv_find(device);
- if (!bs) {
- error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+ bs = bdrv_lookup_bs(has_device, device,
+ has_node_name, node_name,
+ &local_err);
+ if (error_is_set(&local_err)) {
+ error_propagate(errp, local_err);
+ return;
+ }
+
+ if (!bdrv_is_first_non_filter(bs)) {
+ error_set(errp, QERR_FEATURE_DISABLED, "resize");
return;
}
diff --git a/hmp.c b/hmp.c
index 3820fbe..906ddb7 100644
--- a/hmp.c
+++ b/hmp.c
@@ -892,7 +892,7 @@ void hmp_block_resize(Monitor *mon, const QDict *qdict)
int64_t size = qdict_get_int(qdict, "size");
Error *errp = NULL;
- qmp_block_resize(device, size, &errp);
+ qmp_block_resize(true, device, false, NULL, size, &errp);
hmp_handle_error(mon, &errp);
}
diff --git a/qapi-schema.json b/qapi-schema.json
index 903fcb6..3977619 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1722,7 +1722,11 @@
#
# Resize a block image while a guest is running.
#
-# @device: the name of the device to get the image resized
+# Either @device or @node-name must be set but not both.
+#
+# @device: #optional the name of the device to get the image resized
+#
+# @node-name: #optional graph node name to get the image resized (Since 2.0)
#
# @size: new image size in bytes
#
@@ -1731,7 +1735,9 @@
#
# Since: 0.14.0
##
-{ 'command': 'block_resize', 'data': { 'device': 'str', 'size': 'int' }}
+{ 'command': 'block_resize', 'data': { '*device': 'str',
+ '*node-name': 'str',
+ 'size': 'int' }}
##
# @NewImageMode
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 1451c1a..5696b08 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -880,7 +880,7 @@ EQMP
{
.name = "block_resize",
- .args_type = "device:B,size:o",
+ .args_type = "device:s?,node-name:s?,size:o",
.mhandler.cmd_new = qmp_marshal_input_block_resize,
},
@@ -893,6 +893,7 @@ Resize a block image while a guest is running.
Arguments:
- "device": the device's ID, must be unique (json-string)
+- "node-name": the node name in the block driver state graph (json-string)
- "size": new size
Example:
--
1.8.3.2
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH V5 6/7] qmp: Allow block_resize to manipulate bs graph nodes.
2013-12-12 15:33 ` [Qemu-devel] [PATCH V5 6/7] qmp: Allow block_resize to manipulate bs graph nodes Benoît Canet
@ 2014-01-21 3:46 ` Fam Zheng
0 siblings, 0 replies; 28+ messages in thread
From: Fam Zheng @ 2014-01-21 3:46 UTC (permalink / raw)
To: Benoît Canet; +Cc: kwolf, jcody, qemu-devel, stefanha, armbru
On Thu, 12/12 16:33, Benoît Canet wrote:
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
> blockdev.c | 18 ++++++++++++++----
> hmp.c | 2 +-
> qapi-schema.json | 10 ++++++++--
> qmp-commands.hx | 3 ++-
> 4 files changed, 25 insertions(+), 8 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index ebb8f48..374d03d 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1676,14 +1676,24 @@ int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
> return 0;
> }
>
> -void qmp_block_resize(const char *device, int64_t size, Error **errp)
> +void qmp_block_resize(bool has_device, const char *device,
> + bool has_node_name, const char *node_name,
> + int64_t size, Error **errp)
> {
> + Error *local_err = NULL;
> BlockDriverState *bs;
> int ret;
>
> - bs = bdrv_find(device);
> - if (!bs) {
> - error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> + bs = bdrv_lookup_bs(has_device, device,
> + has_node_name, node_name,
> + &local_err);
> + if (error_is_set(&local_err)) {
> + error_propagate(errp, local_err);
> + return;
> + }
> +
> + if (!bdrv_is_first_non_filter(bs)) {
> + error_set(errp, QERR_FEATURE_DISABLED, "resize");
> return;
> }
>
> diff --git a/hmp.c b/hmp.c
> index 3820fbe..906ddb7 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -892,7 +892,7 @@ void hmp_block_resize(Monitor *mon, const QDict *qdict)
> int64_t size = qdict_get_int(qdict, "size");
> Error *errp = NULL;
>
> - qmp_block_resize(device, size, &errp);
> + qmp_block_resize(true, device, false, NULL, size, &errp);
> hmp_handle_error(mon, &errp);
> }
>
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 903fcb6..3977619 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1722,7 +1722,11 @@
> #
> # Resize a block image while a guest is running.
> #
> -# @device: the name of the device to get the image resized
> +# Either @device or @node-name must be set but not both.
> +#
> +# @device: #optional the name of the device to get the image resized
> +#
> +# @node-name: #optional graph node name to get the image resized (Since 2.0)
> #
> # @size: new image size in bytes
> #
> @@ -1731,7 +1735,9 @@
> #
> # Since: 0.14.0
> ##
> -{ 'command': 'block_resize', 'data': { 'device': 'str', 'size': 'int' }}
> +{ 'command': 'block_resize', 'data': { '*device': 'str',
> + '*node-name': 'str',
> + 'size': 'int' }}
>
> ##
> # @NewImageMode
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 1451c1a..5696b08 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -880,7 +880,7 @@ EQMP
>
> {
> .name = "block_resize",
> - .args_type = "device:B,size:o",
> + .args_type = "device:s?,node-name:s?,size:o",
> .mhandler.cmd_new = qmp_marshal_input_block_resize,
> },
>
> @@ -893,6 +893,7 @@ Resize a block image while a guest is running.
> Arguments:
>
> - "device": the device's ID, must be unique (json-string)
> +- "node-name": the node name in the block driver state graph (json-string)
> - "size": new size
>
> Example:
> --
> 1.8.3.2
>
>
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH V5 7/7] qmp: Allow to take external snapshots on bs graphs node.
2013-12-12 15:33 [Qemu-devel] [PATCH V5 0/7] Giving names to BlockDriverState graph nodes Benoît Canet
` (5 preceding siblings ...)
2013-12-12 15:33 ` [Qemu-devel] [PATCH V5 6/7] qmp: Allow block_resize to manipulate bs graph nodes Benoît Canet
@ 2013-12-12 15:34 ` Benoît Canet
2014-01-21 3:54 ` Fam Zheng
2014-01-21 14:28 ` Kevin Wolf
2014-01-16 17:18 ` [Qemu-devel] [PATCH V5 0/7] Giving names to BlockDriverState graph nodes Benoît Canet
` (2 subsequent siblings)
9 siblings, 2 replies; 28+ messages in thread
From: Benoît Canet @ 2013-12-12 15:34 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, Benoît Canet, jcody, armbru, stefanha
Signed-off-by: Benoit Canet <benoit@irqsave.net>
---
blockdev.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++-------
hmp.c | 4 +++-
qapi-schema.json | 13 ++++++++++---
qmp-commands.hx | 11 ++++++++++-
4 files changed, 71 insertions(+), 12 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 374d03d..1246544 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -940,14 +940,22 @@ static void blockdev_do_action(int kind, void *data, Error **errp)
qmp_transaction(&list, errp);
}
-void qmp_blockdev_snapshot_sync(const char *device, const char *snapshot_file,
+void qmp_blockdev_snapshot_sync(bool has_device, const char *device,
+ bool has_node_name, const char *node_name,
+ const char *snapshot_file,
+ bool has_snapshot_node_name,
+ const char *snapshot_node_name,
bool has_format, const char *format,
- bool has_mode, enum NewImageMode mode,
- Error **errp)
+ bool has_mode, NewImageMode mode, Error **errp)
{
BlockdevSnapshot snapshot = {
+ .has_device = has_device,
.device = (char *) device,
+ .has_node_name = has_node_name,
+ .node_name = (char *) node_name,
.snapshot_file = (char *) snapshot_file,
+ .has_snapshot_node_name = has_snapshot_node_name,
+ .snapshot_node_name = (char *) snapshot_node_name,
.has_format = has_format,
.format = (char *) format,
.has_mode = has_mode,
@@ -1185,8 +1193,14 @@ static void external_snapshot_prepare(BlkTransactionState *common,
{
BlockDriver *drv;
int flags, ret;
+ QDict *options = NULL;
Error *local_err = NULL;
+ bool has_device = false;
const char *device;
+ bool has_node_name = false;
+ const char *node_name;
+ bool has_snapshot_node_name = false;
+ const char *snapshot_node_name;
const char *new_image_file;
const char *format = "qcow2";
enum NewImageMode mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
@@ -1197,7 +1211,14 @@ static void external_snapshot_prepare(BlkTransactionState *common,
/* get parameters */
g_assert(action->kind == TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC);
+ has_device = action->blockdev_snapshot_sync->has_device;
device = action->blockdev_snapshot_sync->device;
+ has_node_name = action->blockdev_snapshot_sync->has_node_name;
+ node_name = action->blockdev_snapshot_sync->node_name;
+ has_snapshot_node_name =
+ action->blockdev_snapshot_sync->has_snapshot_node_name;
+ snapshot_node_name = action->blockdev_snapshot_sync->snapshot_node_name;
+
new_image_file = action->blockdev_snapshot_sync->snapshot_file;
if (action->blockdev_snapshot_sync->has_format) {
format = action->blockdev_snapshot_sync->format;
@@ -1213,9 +1234,21 @@ static void external_snapshot_prepare(BlkTransactionState *common,
return;
}
- state->old_bs = bdrv_find(device);
- if (!state->old_bs) {
- error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+ state->old_bs = bdrv_lookup_bs(has_device, device,
+ has_node_name, node_name,
+ &local_err);
+ if (error_is_set(&local_err)) {
+ error_propagate(errp, local_err);
+ return;
+ }
+
+ if (has_node_name && !has_snapshot_node_name) {
+ error_setg(errp, "New snapshot node name missing");
+ return;
+ }
+
+ if (has_snapshot_node_name && bdrv_find_node(snapshot_node_name)) {
+ error_setg(errp, "New snapshot node name already existing");
return;
}
@@ -1255,15 +1288,23 @@ static void external_snapshot_prepare(BlkTransactionState *common,
}
}
+ if (has_snapshot_node_name) {
+ options = qdict_new();
+ qdict_put(options, "node-name",
+ qstring_from_str(snapshot_node_name));
+ }
+
/* We will manually add the backing_hd field to the bs later */
state->new_bs = bdrv_new("");
/* TODO Inherit bs->options or only take explicit options with an
* extended QMP command? */
- ret = bdrv_open(state->new_bs, new_image_file, NULL,
+ ret = bdrv_open(state->new_bs, new_image_file, options,
flags | BDRV_O_NO_BACKING, drv, &local_err);
if (ret != 0) {
error_propagate(errp, local_err);
}
+
+ QDECREF(options);
}
static void external_snapshot_commit(BlkTransactionState *common)
diff --git a/hmp.c b/hmp.c
index 906ddb7..47dcf0c 100644
--- a/hmp.c
+++ b/hmp.c
@@ -971,7 +971,9 @@ void hmp_snapshot_blkdev(Monitor *mon, const QDict *qdict)
}
mode = reuse ? NEW_IMAGE_MODE_EXISTING : NEW_IMAGE_MODE_ABSOLUTE_PATHS;
- qmp_blockdev_snapshot_sync(device, filename, !!format, format,
+ qmp_blockdev_snapshot_sync(true, device, false, NULL,
+ filename, false, NULL,
+ !!format, format,
true, mode, &errp);
hmp_handle_error(mon, &errp);
}
diff --git a/qapi-schema.json b/qapi-schema.json
index 3977619..d7afb69 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1759,18 +1759,25 @@
##
# @BlockdevSnapshot
#
-# @device: the name of the device to generate the snapshot from.
+# Either @device or @node-name must be set but not both.
+#
+# @device: #optional the name of the device to generate the snapshot from.
+#
+# @node-name: #optional graph node name to generate the snapshot from (Since 2.0)
#
# @snapshot-file: the target of the new image. A new file will be created.
#
+# @snapshot-node-name: #optional the graph node name of the new image (Since 2.0)
+#
# @format: #optional the format of the snapshot image, default is 'qcow2'.
#
# @mode: #optional whether and how QEMU should create a new image, default is
# 'absolute-paths'.
##
{ 'type': 'BlockdevSnapshot',
- 'data': { 'device': 'str', 'snapshot-file': 'str', '*format': 'str',
- '*mode': 'NewImageMode' } }
+ 'data': { '*device': 'str', '*node-name': 'str',
+ 'snapshot-file': 'str', '*snapshot-node-name': 'str',
+ '*format': 'str', '*mode': 'NewImageMode' } }
##
# @BlockdevSnapshotInternal
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 5696b08..b62b0f5 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1038,7 +1038,9 @@ actions array:
- "data": a dictionary. The contents depend on the value
of "type". When "type" is "blockdev-snapshot-sync":
- "device": device name to snapshot (json-string)
+ - "node-name": graph node name to snapshot (json-string)
- "snapshot-file": name of new image file (json-string)
+ - "snapshot-node-name": graph node name of the new snapshot (json-string)
- "format": format of new image (json-string, optional)
- "mode": whether and how QEMU should create the snapshot file
(NewImageMode, optional, default "absolute-paths")
@@ -1053,6 +1055,11 @@ Example:
{ 'type': 'blockdev-snapshot-sync', 'data' : { "device": "ide-hd0",
"snapshot-file": "/some/place/my-image",
"format": "qcow2" } },
+ { 'type': 'blockdev-snapshot-sync', 'data' : { "node-name": "myfile",
+ "snapshot-file": "/some/place/my-image2",
+ "snapshot-node-name": "node3432",
+ "mode": "existing",
+ "format": "qcow2" } },
{ 'type': 'blockdev-snapshot-sync', 'data' : { "device": "ide-hd1",
"snapshot-file": "/some/place/my-image2",
"mode": "existing",
@@ -1066,7 +1073,7 @@ EQMP
{
.name = "blockdev-snapshot-sync",
- .args_type = "device:B,snapshot-file:s,format:s?,mode:s?",
+ .args_type = "device:s?,node-name:s?,snapshot-file:s,snapshot-node-name:s?,format:s?,mode:s?",
.mhandler.cmd_new = qmp_marshal_input_blockdev_snapshot_sync,
},
@@ -1083,7 +1090,9 @@ snapshot image, default is qcow2.
Arguments:
- "device": device name to snapshot (json-string)
+- "node-name": graph node name to snapshot (json-string)
- "snapshot-file": name of new image file (json-string)
+- "snapshot-node-name": graph node name of the new snapshot (json-string)
- "mode": whether and how QEMU should create the snapshot file
(NewImageMode, optional, default "absolute-paths")
- "format": format of new image (json-string, optional)
--
1.8.3.2
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH V5 7/7] qmp: Allow to take external snapshots on bs graphs node.
2013-12-12 15:34 ` [Qemu-devel] [PATCH V5 7/7] qmp: Allow to take external snapshots on bs graphs node Benoît Canet
@ 2014-01-21 3:54 ` Fam Zheng
2014-01-21 14:28 ` Kevin Wolf
1 sibling, 0 replies; 28+ messages in thread
From: Fam Zheng @ 2014-01-21 3:54 UTC (permalink / raw)
To: Benoît Canet; +Cc: kwolf, jcody, qemu-devel, stefanha, armbru
On Thu, 12/12 16:34, Benoît Canet wrote:
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
> blockdev.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++-------
> hmp.c | 4 +++-
> qapi-schema.json | 13 ++++++++++---
> qmp-commands.hx | 11 ++++++++++-
> 4 files changed, 71 insertions(+), 12 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 374d03d..1246544 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -940,14 +940,22 @@ static void blockdev_do_action(int kind, void *data, Error **errp)
> qmp_transaction(&list, errp);
> }
>
> -void qmp_blockdev_snapshot_sync(const char *device, const char *snapshot_file,
> +void qmp_blockdev_snapshot_sync(bool has_device, const char *device,
> + bool has_node_name, const char *node_name,
> + const char *snapshot_file,
> + bool has_snapshot_node_name,
> + const char *snapshot_node_name,
> bool has_format, const char *format,
> - bool has_mode, enum NewImageMode mode,
> - Error **errp)
> + bool has_mode, NewImageMode mode, Error **errp)
> {
> BlockdevSnapshot snapshot = {
> + .has_device = has_device,
> .device = (char *) device,
> + .has_node_name = has_node_name,
> + .node_name = (char *) node_name,
> .snapshot_file = (char *) snapshot_file,
> + .has_snapshot_node_name = has_snapshot_node_name,
> + .snapshot_node_name = (char *) snapshot_node_name,
> .has_format = has_format,
> .format = (char *) format,
> .has_mode = has_mode,
> @@ -1185,8 +1193,14 @@ static void external_snapshot_prepare(BlkTransactionState *common,
> {
> BlockDriver *drv;
> int flags, ret;
> + QDict *options = NULL;
> Error *local_err = NULL;
> + bool has_device = false;
> const char *device;
> + bool has_node_name = false;
> + const char *node_name;
> + bool has_snapshot_node_name = false;
> + const char *snapshot_node_name;
> const char *new_image_file;
> const char *format = "qcow2";
> enum NewImageMode mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
> @@ -1197,7 +1211,14 @@ static void external_snapshot_prepare(BlkTransactionState *common,
> /* get parameters */
> g_assert(action->kind == TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC);
>
> + has_device = action->blockdev_snapshot_sync->has_device;
> device = action->blockdev_snapshot_sync->device;
> + has_node_name = action->blockdev_snapshot_sync->has_node_name;
> + node_name = action->blockdev_snapshot_sync->node_name;
> + has_snapshot_node_name =
> + action->blockdev_snapshot_sync->has_snapshot_node_name;
> + snapshot_node_name = action->blockdev_snapshot_sync->snapshot_node_name;
> +
> new_image_file = action->blockdev_snapshot_sync->snapshot_file;
> if (action->blockdev_snapshot_sync->has_format) {
> format = action->blockdev_snapshot_sync->format;
> @@ -1213,9 +1234,21 @@ static void external_snapshot_prepare(BlkTransactionState *common,
> return;
> }
>
> - state->old_bs = bdrv_find(device);
> - if (!state->old_bs) {
> - error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> + state->old_bs = bdrv_lookup_bs(has_device, device,
> + has_node_name, node_name,
> + &local_err);
> + if (error_is_set(&local_err)) {
> + error_propagate(errp, local_err);
> + return;
> + }
> +
> + if (has_node_name && !has_snapshot_node_name) {
> + error_setg(errp, "New snapshot node name missing");
> + return;
> + }
> +
> + if (has_snapshot_node_name && bdrv_find_node(snapshot_node_name)) {
> + error_setg(errp, "New snapshot node name already existing");
> return;
> }
>
> @@ -1255,15 +1288,23 @@ static void external_snapshot_prepare(BlkTransactionState *common,
> }
> }
>
> + if (has_snapshot_node_name) {
> + options = qdict_new();
> + qdict_put(options, "node-name",
> + qstring_from_str(snapshot_node_name));
> + }
> +
> /* We will manually add the backing_hd field to the bs later */
> state->new_bs = bdrv_new("");
> /* TODO Inherit bs->options or only take explicit options with an
> * extended QMP command? */
> - ret = bdrv_open(state->new_bs, new_image_file, NULL,
> + ret = bdrv_open(state->new_bs, new_image_file, options,
> flags | BDRV_O_NO_BACKING, drv, &local_err);
> if (ret != 0) {
> error_propagate(errp, local_err);
> }
> +
> + QDECREF(options);
> }
>
> static void external_snapshot_commit(BlkTransactionState *common)
> diff --git a/hmp.c b/hmp.c
> index 906ddb7..47dcf0c 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -971,7 +971,9 @@ void hmp_snapshot_blkdev(Monitor *mon, const QDict *qdict)
> }
>
> mode = reuse ? NEW_IMAGE_MODE_EXISTING : NEW_IMAGE_MODE_ABSOLUTE_PATHS;
> - qmp_blockdev_snapshot_sync(device, filename, !!format, format,
> + qmp_blockdev_snapshot_sync(true, device, false, NULL,
> + filename, false, NULL,
> + !!format, format,
> true, mode, &errp);
> hmp_handle_error(mon, &errp);
> }
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 3977619..d7afb69 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1759,18 +1759,25 @@
> ##
> # @BlockdevSnapshot
> #
> -# @device: the name of the device to generate the snapshot from.
> +# Either @device or @node-name must be set but not both.
> +#
> +# @device: #optional the name of the device to generate the snapshot from.
> +#
> +# @node-name: #optional graph node name to generate the snapshot from (Since 2.0)
> #
> # @snapshot-file: the target of the new image. A new file will be created.
> #
> +# @snapshot-node-name: #optional the graph node name of the new image (Since 2.0)
> +#
> # @format: #optional the format of the snapshot image, default is 'qcow2'.
> #
> # @mode: #optional whether and how QEMU should create a new image, default is
> # 'absolute-paths'.
> ##
> { 'type': 'BlockdevSnapshot',
> - 'data': { 'device': 'str', 'snapshot-file': 'str', '*format': 'str',
> - '*mode': 'NewImageMode' } }
> + 'data': { '*device': 'str', '*node-name': 'str',
> + 'snapshot-file': 'str', '*snapshot-node-name': 'str',
> + '*format': 'str', '*mode': 'NewImageMode' } }
>
> ##
> # @BlockdevSnapshotInternal
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 5696b08..b62b0f5 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -1038,7 +1038,9 @@ actions array:
> - "data": a dictionary. The contents depend on the value
> of "type". When "type" is "blockdev-snapshot-sync":
> - "device": device name to snapshot (json-string)
> + - "node-name": graph node name to snapshot (json-string)
> - "snapshot-file": name of new image file (json-string)
> + - "snapshot-node-name": graph node name of the new snapshot (json-string)
> - "format": format of new image (json-string, optional)
> - "mode": whether and how QEMU should create the snapshot file
> (NewImageMode, optional, default "absolute-paths")
> @@ -1053,6 +1055,11 @@ Example:
> { 'type': 'blockdev-snapshot-sync', 'data' : { "device": "ide-hd0",
> "snapshot-file": "/some/place/my-image",
> "format": "qcow2" } },
> + { 'type': 'blockdev-snapshot-sync', 'data' : { "node-name": "myfile",
> + "snapshot-file": "/some/place/my-image2",
> + "snapshot-node-name": "node3432",
> + "mode": "existing",
> + "format": "qcow2" } },
> { 'type': 'blockdev-snapshot-sync', 'data' : { "device": "ide-hd1",
> "snapshot-file": "/some/place/my-image2",
> "mode": "existing",
> @@ -1066,7 +1073,7 @@ EQMP
>
> {
> .name = "blockdev-snapshot-sync",
> - .args_type = "device:B,snapshot-file:s,format:s?,mode:s?",
> + .args_type = "device:s?,node-name:s?,snapshot-file:s,snapshot-node-name:s?,format:s?,mode:s?",
> .mhandler.cmd_new = qmp_marshal_input_blockdev_snapshot_sync,
> },
>
> @@ -1083,7 +1090,9 @@ snapshot image, default is qcow2.
> Arguments:
>
> - "device": device name to snapshot (json-string)
> +- "node-name": graph node name to snapshot (json-string)
> - "snapshot-file": name of new image file (json-string)
> +- "snapshot-node-name": graph node name of the new snapshot (json-string)
> - "mode": whether and how QEMU should create the snapshot file
> (NewImageMode, optional, default "absolute-paths")
> - "format": format of new image (json-string, optional)
> --
> 1.8.3.2
>
>
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH V5 7/7] qmp: Allow to take external snapshots on bs graphs node.
2013-12-12 15:34 ` [Qemu-devel] [PATCH V5 7/7] qmp: Allow to take external snapshots on bs graphs node Benoît Canet
2014-01-21 3:54 ` Fam Zheng
@ 2014-01-21 14:28 ` Kevin Wolf
2014-01-22 21:33 ` Benoît Canet
1 sibling, 1 reply; 28+ messages in thread
From: Kevin Wolf @ 2014-01-21 14:28 UTC (permalink / raw)
To: Benoît Canet; +Cc: famz, jcody, qemu-devel, armbru, stefanha
Am 12.12.2013 um 16:34 hat Benoît Canet geschrieben:
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
> blockdev.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++-------
> hmp.c | 4 +++-
> qapi-schema.json | 13 ++++++++++---
> qmp-commands.hx | 11 ++++++++++-
> 4 files changed, 71 insertions(+), 12 deletions(-)
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 3977619..d7afb69 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1759,18 +1759,25 @@
> ##
> # @BlockdevSnapshot
> #
> -# @device: the name of the device to generate the snapshot from.
> +# Either @device or @node-name must be set but not both.
> +#
> +# @device: #optional the name of the device to generate the snapshot from.
> +#
> +# @node-name: #optional graph node name to generate the snapshot from (Since 2.0)
> #
> # @snapshot-file: the target of the new image. A new file will be created.
> #
> +# @snapshot-node-name: #optional the graph node name of the new image (Since 2.0)
> +#
I think we should document how this plays together with snapshot-file,
format and mode.
Perhaps this is actually different enough that it would justify creating
a new QMP command 'blockdev-snapshot' that never creates image files but
only ever takes existing nodes. The implementation of 'blockdev-snapshot-
sync' could then become a wrapper around it.
> # @format: #optional the format of the snapshot image, default is 'qcow2'.
> #
> # @mode: #optional whether and how QEMU should create a new image, default is
> # 'absolute-paths'.
> ##
> { 'type': 'BlockdevSnapshot',
> - 'data': { 'device': 'str', 'snapshot-file': 'str', '*format': 'str',
> - '*mode': 'NewImageMode' } }
> + 'data': { '*device': 'str', '*node-name': 'str',
> + 'snapshot-file': 'str', '*snapshot-node-name': 'str',
> + '*format': 'str', '*mode': 'NewImageMode' } }
>
> ##
> # @BlockdevSnapshotInternal
Kevin
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH V5 7/7] qmp: Allow to take external snapshots on bs graphs node.
2014-01-21 14:28 ` Kevin Wolf
@ 2014-01-22 21:33 ` Benoît Canet
2014-01-23 10:20 ` Kevin Wolf
0 siblings, 1 reply; 28+ messages in thread
From: Benoît Canet @ 2014-01-22 21:33 UTC (permalink / raw)
To: Kevin Wolf; +Cc: famz, jcody, qemu-devel, armbru, stefanha
Le Tuesday 21 Jan 2014 à 15:28:49 (+0100), Kevin Wolf a écrit :
> Am 12.12.2013 um 16:34 hat Benoît Canet geschrieben:
> > Signed-off-by: Benoit Canet <benoit@irqsave.net>
> > ---
> > blockdev.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++-------
> > hmp.c | 4 +++-
> > qapi-schema.json | 13 ++++++++++---
> > qmp-commands.hx | 11 ++++++++++-
> > 4 files changed, 71 insertions(+), 12 deletions(-)
>
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index 3977619..d7afb69 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -1759,18 +1759,25 @@
> > ##
> > # @BlockdevSnapshot
> > #
> > -# @device: the name of the device to generate the snapshot from.
> > +# Either @device or @node-name must be set but not both.
> > +#
> > +# @device: #optional the name of the device to generate the snapshot from.
> > +#
> > +# @node-name: #optional graph node name to generate the snapshot from (Since 2.0)
> > #
> > # @snapshot-file: the target of the new image. A new file will be created.
> > #
> > +# @snapshot-node-name: #optional the graph node name of the new image (Since 2.0)
> > +#
>
> I think we should document how this plays together with snapshot-file,
> format and mode.
What kind of interactions do you expect ?
The only kind of interaction I see is that setting node-name imply that the
user really want to manipulate the graph and that snapshot-node-name is
mandatory as a consequence.
>
> Perhaps this is actually different enough that it would justify creating
> a new QMP command 'blockdev-snapshot' that never creates image files but
> only ever takes existing nodes. The implementation of 'blockdev-snapshot-
> sync' could then become a wrapper around it.
What would be the benefits ?
Best regards
Benoît
>
> > # @format: #optional the format of the snapshot image, default is 'qcow2'.
> > #
> > # @mode: #optional whether and how QEMU should create a new image, default is
> > # 'absolute-paths'.
> > ##
> > { 'type': 'BlockdevSnapshot',
> > - 'data': { 'device': 'str', 'snapshot-file': 'str', '*format': 'str',
> > - '*mode': 'NewImageMode' } }
> > + 'data': { '*device': 'str', '*node-name': 'str',
> > + 'snapshot-file': 'str', '*snapshot-node-name': 'str',
> > + '*format': 'str', '*mode': 'NewImageMode' } }
> >
> > ##
> > # @BlockdevSnapshotInternal
>
> Kevin
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH V5 7/7] qmp: Allow to take external snapshots on bs graphs node.
2014-01-22 21:33 ` Benoît Canet
@ 2014-01-23 10:20 ` Kevin Wolf
0 siblings, 0 replies; 28+ messages in thread
From: Kevin Wolf @ 2014-01-23 10:20 UTC (permalink / raw)
To: Benoît Canet; +Cc: famz, jcody, qemu-devel, armbru, stefanha
Am 22.01.2014 um 22:33 hat Benoît Canet geschrieben:
> Le Tuesday 21 Jan 2014 à 15:28:49 (+0100), Kevin Wolf a écrit :
> > Am 12.12.2013 um 16:34 hat Benoît Canet geschrieben:
> > > Signed-off-by: Benoit Canet <benoit@irqsave.net>
> > > ---
> > > blockdev.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++-------
> > > hmp.c | 4 +++-
> > > qapi-schema.json | 13 ++++++++++---
> > > qmp-commands.hx | 11 ++++++++++-
> > > 4 files changed, 71 insertions(+), 12 deletions(-)
> >
> > > diff --git a/qapi-schema.json b/qapi-schema.json
> > > index 3977619..d7afb69 100644
> > > --- a/qapi-schema.json
> > > +++ b/qapi-schema.json
> > > @@ -1759,18 +1759,25 @@
> > > ##
> > > # @BlockdevSnapshot
> > > #
> > > -# @device: the name of the device to generate the snapshot from.
> > > +# Either @device or @node-name must be set but not both.
> > > +#
> > > +# @device: #optional the name of the device to generate the snapshot from.
> > > +#
> > > +# @node-name: #optional graph node name to generate the snapshot from (Since 2.0)
> > > #
> > > # @snapshot-file: the target of the new image. A new file will be created.
> > > #
> > > +# @snapshot-node-name: #optional the graph node name of the new image (Since 2.0)
> > > +#
> >
> > I think we should document how this plays together with snapshot-file,
> > format and mode.
>
> What kind of interactions do you expect ?
> The only kind of interaction I see is that setting node-name imply that the
> user really want to manipulate the graph and that snapshot-node-name is
> mandatory as a consequence.
Aha, I misunderstood. I thought you could pass the node name of an
existing block device that should be used as the new overlay image. But
in fact, you still create a new image and only assign the
snapshot-node-name to the newly created node.
Nothing wrong with the behaviour, it's just that device/node-name and
snapshot-file/snapshot-node-name suggested a parallelity that isn't
there. Perhaps someone has a good idea to improve the naming.
Kevin
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH V5 0/7] Giving names to BlockDriverState graph nodes
2013-12-12 15:33 [Qemu-devel] [PATCH V5 0/7] Giving names to BlockDriverState graph nodes Benoît Canet
` (6 preceding siblings ...)
2013-12-12 15:34 ` [Qemu-devel] [PATCH V5 7/7] qmp: Allow to take external snapshots on bs graphs node Benoît Canet
@ 2014-01-16 17:18 ` Benoît Canet
2014-01-21 4:04 ` Fam Zheng
2014-01-21 14:33 ` Kevin Wolf
9 siblings, 0 replies; 28+ messages in thread
From: Benoît Canet @ 2014-01-16 17:18 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, jcody, armbru, stefanha
Le Thursday 12 Dec 2013 à 16:33:53 (+0100), Benoît Canet a écrit :
> v5:
> block empty node names [Kevin]
> factorize setting of node-name option [Kevin]
> NULL terminate node_name on removal [Kevin]
> make query-named-block-nodes return BlockDeviceInfo structure [Eric]
> Change some doc in query-named-block-nodes [Eric]
> Document the choice of the QMP API for node name [Eric]
> Use the same authorization as snapshot on block resize [Kevin]
> Rebase the series [Kevin]
>
> Benoît Canet (7):
> block: Add bs->node_name to hold the name of a bs node of the bs
> graph.
> block: Allow the user to define "node-name" option.
> qmp: Add a command to list the named BlockDriverState nodes.
> qmp: Allow to change password on named block driver states.
> block: Create authorizations mechanism for external snapshot and
> resize.
> qmp: Allow block_resize to manipulate bs graph nodes.
> qmp: Allow to take external snapshots on bs graphs node.
>
> block.c | 210 +++++++++++++++++++++++++++++++++++++++++-----
> block/blkverify.c | 2 +-
> block/qapi.c | 109 ++++++++++++------------
> blockdev.c | 93 ++++++++++++++++----
> hmp.c | 8 +-
> include/block/block.h | 23 +++--
> include/block/block_int.h | 21 ++++-
> include/block/qapi.h | 1 +
> qapi-schema.json | 48 +++++++++--
> qmp-commands.hx | 78 ++++++++++++++++-
> 10 files changed, 471 insertions(+), 122 deletions(-)
Ping
Best regards
Benoît
>
> --
> 1.8.3.2
>
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH V5 0/7] Giving names to BlockDriverState graph nodes
2013-12-12 15:33 [Qemu-devel] [PATCH V5 0/7] Giving names to BlockDriverState graph nodes Benoît Canet
` (7 preceding siblings ...)
2014-01-16 17:18 ` [Qemu-devel] [PATCH V5 0/7] Giving names to BlockDriverState graph nodes Benoît Canet
@ 2014-01-21 4:04 ` Fam Zheng
2014-01-21 14:33 ` Kevin Wolf
9 siblings, 0 replies; 28+ messages in thread
From: Fam Zheng @ 2014-01-21 4:04 UTC (permalink / raw)
To: Benoît Canet; +Cc: kwolf, jcody, qemu-devel, armbru, stefanha
On Thu, 12/12 16:33, Benoît Canet wrote:
> v5:
> block empty node names [Kevin]
> factorize setting of node-name option [Kevin]
> NULL terminate node_name on removal [Kevin]
> make query-named-block-nodes return BlockDeviceInfo structure [Eric]
> Change some doc in query-named-block-nodes [Eric]
> Document the choice of the QMP API for node name [Eric]
> Use the same authorization as snapshot on block resize [Kevin]
> Rebase the series [Kevin]
Looks mostly good to me. But the external snapshot permission protection
interface looks a bit obsecuer to me. Let's wait for other reviewers' comments.
Thanks,
Fam
>
> Benoît Canet (7):
> block: Add bs->node_name to hold the name of a bs node of the bs
> graph.
> block: Allow the user to define "node-name" option.
> qmp: Add a command to list the named BlockDriverState nodes.
> qmp: Allow to change password on named block driver states.
> block: Create authorizations mechanism for external snapshot and
> resize.
> qmp: Allow block_resize to manipulate bs graph nodes.
> qmp: Allow to take external snapshots on bs graphs node.
>
> block.c | 210 +++++++++++++++++++++++++++++++++++++++++-----
> block/blkverify.c | 2 +-
> block/qapi.c | 109 ++++++++++++------------
> blockdev.c | 93 ++++++++++++++++----
> hmp.c | 8 +-
> include/block/block.h | 23 +++--
> include/block/block_int.h | 21 ++++-
> include/block/qapi.h | 1 +
> qapi-schema.json | 48 +++++++++--
> qmp-commands.hx | 78 ++++++++++++++++-
> 10 files changed, 471 insertions(+), 122 deletions(-)
>
> --
> 1.8.3.2
>
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH V5 0/7] Giving names to BlockDriverState graph nodes
2013-12-12 15:33 [Qemu-devel] [PATCH V5 0/7] Giving names to BlockDriverState graph nodes Benoît Canet
` (8 preceding siblings ...)
2014-01-21 4:04 ` Fam Zheng
@ 2014-01-21 14:33 ` Kevin Wolf
2014-01-21 15:36 ` Benoît Canet
9 siblings, 1 reply; 28+ messages in thread
From: Kevin Wolf @ 2014-01-21 14:33 UTC (permalink / raw)
To: Benoît Canet; +Cc: famz, jcody, qemu-devel, armbru, stefanha, mreitz
Am 12.12.2013 um 16:33 hat Benoît Canet geschrieben:
> v5:
> block empty node names [Kevin]
> factorize setting of node-name option [Kevin]
> NULL terminate node_name on removal [Kevin]
> make query-named-block-nodes return BlockDeviceInfo structure [Eric]
> Change some doc in query-named-block-nodes [Eric]
> Document the choice of the QMP API for node name [Eric]
> Use the same authorization as snapshot on block resize [Kevin]
> Rebase the series [Kevin]
>
> Benoît Canet (7):
> block: Add bs->node_name to hold the name of a bs node of the bs
> graph.
> block: Allow the user to define "node-name" option.
> qmp: Add a command to list the named BlockDriverState nodes.
> qmp: Allow to change password on named block driver states.
> block: Create authorizations mechanism for external snapshot and
> resize.
> qmp: Allow block_resize to manipulate bs graph nodes.
> qmp: Allow to take external snapshots on bs graphs node.
One more general question: Now that we have node names, should
bdrv_file_open() use node names rather than device names for references?
(Well, no, come to think of it, it's not really a question, but more of
a request ;-))
Kevin
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH V5 0/7] Giving names to BlockDriverState graph nodes
2014-01-21 14:33 ` Kevin Wolf
@ 2014-01-21 15:36 ` Benoît Canet
2014-01-21 15:44 ` Kevin Wolf
0 siblings, 1 reply; 28+ messages in thread
From: Benoît Canet @ 2014-01-21 15:36 UTC (permalink / raw)
To: Kevin Wolf; +Cc: famz, jcody, qemu-devel, armbru, stefanha, mreitz
Le Tuesday 21 Jan 2014 à 15:33:09 (+0100), Kevin Wolf a écrit :
> Am 12.12.2013 um 16:33 hat Benoît Canet geschrieben:
> > v5:
> > block empty node names [Kevin]
> > factorize setting of node-name option [Kevin]
> > NULL terminate node_name on removal [Kevin]
> > make query-named-block-nodes return BlockDeviceInfo structure [Eric]
> > Change some doc in query-named-block-nodes [Eric]
> > Document the choice of the QMP API for node name [Eric]
> > Use the same authorization as snapshot on block resize [Kevin]
> > Rebase the series [Kevin]
> >
> > Benoît Canet (7):
> > block: Add bs->node_name to hold the name of a bs node of the bs
> > graph.
> > block: Allow the user to define "node-name" option.
> > qmp: Add a command to list the named BlockDriverState nodes.
> > qmp: Allow to change password on named block driver states.
> > block: Create authorizations mechanism for external snapshot and
> > resize.
> > qmp: Allow block_resize to manipulate bs graph nodes.
> > qmp: Allow to take external snapshots on bs graphs node.
>
> One more general question: Now that we have node names, should
> bdrv_file_open() use node names rather than device names for references?
> (Well, no, come to think of it, it's not really a question, but more of
> a request ;-))
:)
I don't understand what you mean: I don't see any reference to device_name in
bdrv_file_open only one in bdrv_open which is used in a printf.
Best regards
Benoît
>
> Kevin
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH V5 0/7] Giving names to BlockDriverState graph nodes
2014-01-21 15:36 ` Benoît Canet
@ 2014-01-21 15:44 ` Kevin Wolf
2014-01-22 21:44 ` Benoît Canet
2014-01-22 22:03 ` Benoît Canet
0 siblings, 2 replies; 28+ messages in thread
From: Kevin Wolf @ 2014-01-21 15:44 UTC (permalink / raw)
To: Benoît Canet; +Cc: famz, jcody, qemu-devel, armbru, stefanha, mreitz
Am 21.01.2014 um 16:36 hat Benoît Canet geschrieben:
> Le Tuesday 21 Jan 2014 à 15:33:09 (+0100), Kevin Wolf a écrit :
> > Am 12.12.2013 um 16:33 hat Benoît Canet geschrieben:
> > > v5:
> > > block empty node names [Kevin]
> > > factorize setting of node-name option [Kevin]
> > > NULL terminate node_name on removal [Kevin]
> > > make query-named-block-nodes return BlockDeviceInfo structure [Eric]
> > > Change some doc in query-named-block-nodes [Eric]
> > > Document the choice of the QMP API for node name [Eric]
> > > Use the same authorization as snapshot on block resize [Kevin]
> > > Rebase the series [Kevin]
> > >
> > > Benoît Canet (7):
> > > block: Add bs->node_name to hold the name of a bs node of the bs
> > > graph.
> > > block: Allow the user to define "node-name" option.
> > > qmp: Add a command to list the named BlockDriverState nodes.
> > > qmp: Allow to change password on named block driver states.
> > > block: Create authorizations mechanism for external snapshot and
> > > resize.
> > > qmp: Allow block_resize to manipulate bs graph nodes.
> > > qmp: Allow to take external snapshots on bs graphs node.
> >
> > One more general question: Now that we have node names, should
> > bdrv_file_open() use node names rather than device names for references?
> > (Well, no, come to think of it, it's not really a question, but more of
> > a request ;-))
> :)
> I don't understand what you mean: I don't see any reference to device_name in
> bdrv_file_open only one in bdrv_open which is used in a printf.
I'm talking about the function parameter 'const char *reference', which
references an existing block device rather than opening a new one. It
doesn't directly use device_name, but it calls bdrv_find().
Kevin
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH V5 0/7] Giving names to BlockDriverState graph nodes
2014-01-21 15:44 ` Kevin Wolf
@ 2014-01-22 21:44 ` Benoît Canet
2014-01-23 10:14 ` Kevin Wolf
2014-01-22 22:03 ` Benoît Canet
1 sibling, 1 reply; 28+ messages in thread
From: Benoît Canet @ 2014-01-22 21:44 UTC (permalink / raw)
To: Kevin Wolf
Cc: Benoît Canet, famz, jcody, qemu-devel, armbru, stefanha,
mreitz
Le Tuesday 21 Jan 2014 à 16:44:49 (+0100), Kevin Wolf a écrit :
> Am 21.01.2014 um 16:36 hat Benoît Canet geschrieben:
> > Le Tuesday 21 Jan 2014 à 15:33:09 (+0100), Kevin Wolf a écrit :
> > > Am 12.12.2013 um 16:33 hat Benoît Canet geschrieben:
> > > > v5:
> > > > block empty node names [Kevin]
> > > > factorize setting of node-name option [Kevin]
> > > > NULL terminate node_name on removal [Kevin]
> > > > make query-named-block-nodes return BlockDeviceInfo structure [Eric]
> > > > Change some doc in query-named-block-nodes [Eric]
> > > > Document the choice of the QMP API for node name [Eric]
> > > > Use the same authorization as snapshot on block resize [Kevin]
> > > > Rebase the series [Kevin]
> > > >
> > > > Benoît Canet (7):
> > > > block: Add bs->node_name to hold the name of a bs node of the bs
> > > > graph.
> > > > block: Allow the user to define "node-name" option.
> > > > qmp: Add a command to list the named BlockDriverState nodes.
> > > > qmp: Allow to change password on named block driver states.
> > > > block: Create authorizations mechanism for external snapshot and
> > > > resize.
> > > > qmp: Allow block_resize to manipulate bs graph nodes.
> > > > qmp: Allow to take external snapshots on bs graphs node.
> > >
> > > One more general question: Now that we have node names, should
> > > bdrv_file_open() use node names rather than device names for references?
> > > (Well, no, come to think of it, it's not really a question, but more of
> > > a request ;-))
> > :)
> > I don't understand what you mean: I don't see any reference to device_name in
> > bdrv_file_open only one in bdrv_open which is used in a printf.
>
> I'm talking about the function parameter 'const char *reference', which
> references an existing block device rather than opening a new one. It
> doesn't directly use device_name, but it calls bdrv_find().
I see:
int bdrv_file_open(BlockDriverState **pbs, const char *filename,
QDict *options, int flags, Error **errp)
so there is a const char *filename but not const char *reference.
also:
benoit@localhost:~/qemu (19f7b13...)$ git grep "const char *reference"
benoit@localhost:~/qemu (19f7b13...)$
Are you talking about something lying in another git branch ?
Best regards
Benoît
>
> Kevin
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH V5 0/7] Giving names to BlockDriverState graph nodes
2014-01-22 21:44 ` Benoît Canet
@ 2014-01-23 10:14 ` Kevin Wolf
0 siblings, 0 replies; 28+ messages in thread
From: Kevin Wolf @ 2014-01-23 10:14 UTC (permalink / raw)
To: Benoît Canet; +Cc: famz, jcody, qemu-devel, armbru, stefanha, mreitz
Am 22.01.2014 um 22:44 hat Benoît Canet geschrieben:
> Le Tuesday 21 Jan 2014 à 16:44:49 (+0100), Kevin Wolf a écrit :
> > Am 21.01.2014 um 16:36 hat Benoît Canet geschrieben:
> > > Le Tuesday 21 Jan 2014 à 15:33:09 (+0100), Kevin Wolf a écrit :
> > > > Am 12.12.2013 um 16:33 hat Benoît Canet geschrieben:
> > > > > v5:
> > > > > block empty node names [Kevin]
> > > > > factorize setting of node-name option [Kevin]
> > > > > NULL terminate node_name on removal [Kevin]
> > > > > make query-named-block-nodes return BlockDeviceInfo structure [Eric]
> > > > > Change some doc in query-named-block-nodes [Eric]
> > > > > Document the choice of the QMP API for node name [Eric]
> > > > > Use the same authorization as snapshot on block resize [Kevin]
> > > > > Rebase the series [Kevin]
> > > > >
> > > > > Benoît Canet (7):
> > > > > block: Add bs->node_name to hold the name of a bs node of the bs
> > > > > graph.
> > > > > block: Allow the user to define "node-name" option.
> > > > > qmp: Add a command to list the named BlockDriverState nodes.
> > > > > qmp: Allow to change password on named block driver states.
> > > > > block: Create authorizations mechanism for external snapshot and
> > > > > resize.
> > > > > qmp: Allow block_resize to manipulate bs graph nodes.
> > > > > qmp: Allow to take external snapshots on bs graphs node.
> > > >
> > > > One more general question: Now that we have node names, should
> > > > bdrv_file_open() use node names rather than device names for references?
> > > > (Well, no, come to think of it, it's not really a question, but more of
> > > > a request ;-))
> > > :)
> > > I don't understand what you mean: I don't see any reference to device_name in
> > > bdrv_file_open only one in bdrv_open which is used in a printf.
> >
> > I'm talking about the function parameter 'const char *reference', which
> > references an existing block device rather than opening a new one. It
> > doesn't directly use device_name, but it calls bdrv_find().
>
> I see:
>
> int bdrv_file_open(BlockDriverState **pbs, const char *filename,
> QDict *options, int flags, Error **errp)
>
> so there is a const char *filename but not const char *reference.
>
> also:
>
> benoit@localhost:~/qemu (19f7b13...)$ git grep "const char *reference"
> benoit@localhost:~/qemu (19f7b13...)$
>
> Are you talking about something lying in another git branch ?
Ah yes, I forgot that Anthony hasn't pulled for two weeks now. :-/
The patches are in git://repo.or.cz/qemu/kevin.git block.
Kevin
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH V5 0/7] Giving names to BlockDriverState graph nodes
2014-01-21 15:44 ` Kevin Wolf
2014-01-22 21:44 ` Benoît Canet
@ 2014-01-22 22:03 ` Benoît Canet
1 sibling, 0 replies; 28+ messages in thread
From: Benoît Canet @ 2014-01-22 22:03 UTC (permalink / raw)
To: Kevin Wolf
Cc: Benoît Canet, famz, jcody, qemu-devel, armbru, stefanha,
mreitz
Le Tuesday 21 Jan 2014 à 16:44:49 (+0100), Kevin Wolf a écrit :
> Am 21.01.2014 um 16:36 hat Benoît Canet geschrieben:
> > Le Tuesday 21 Jan 2014 à 15:33:09 (+0100), Kevin Wolf a écrit :
> > > Am 12.12.2013 um 16:33 hat Benoît Canet geschrieben:
> > > > v5:
> > > > block empty node names [Kevin]
> > > > factorize setting of node-name option [Kevin]
> > > > NULL terminate node_name on removal [Kevin]
> > > > make query-named-block-nodes return BlockDeviceInfo structure [Eric]
> > > > Change some doc in query-named-block-nodes [Eric]
> > > > Document the choice of the QMP API for node name [Eric]
> > > > Use the same authorization as snapshot on block resize [Kevin]
> > > > Rebase the series [Kevin]
> > > >
> > > > Benoît Canet (7):
> > > > block: Add bs->node_name to hold the name of a bs node of the bs
> > > > graph.
> > > > block: Allow the user to define "node-name" option.
> > > > qmp: Add a command to list the named BlockDriverState nodes.
> > > > qmp: Allow to change password on named block driver states.
> > > > block: Create authorizations mechanism for external snapshot and
> > > > resize.
> > > > qmp: Allow block_resize to manipulate bs graph nodes.
> > > > qmp: Allow to take external snapshots on bs graphs node.
> > >
> > > One more general question: Now that we have node names, should
> > > bdrv_file_open() use node names rather than device names for references?
> > > (Well, no, come to think of it, it's not really a question, but more of
> > > a request ;-))
> > :)
> > I don't understand what you mean: I don't see any reference to device_name in
> > bdrv_file_open only one in bdrv_open which is used in a printf.
>
> I'm talking about the function parameter 'const char *reference', which
> references an existing block device rather than opening a new one. It
> doesn't directly use device_name, but it calls bdrv_find().
>
> Kevin
>
Do you mean this one:
{ 'union': 'BlockdevRef',
'discriminator': {},
'data': { 'definition': 'BlockdevOptions',
'reference': 'str' } }
I though that QMP names where set in stone.
Best regards
Benoît
^ permalink raw reply [flat|nested] 28+ messages in thread