* [Qemu-devel] [PATCH V6 1/8] block: Add bs->node_name to hold the name of a bs node of the bs graph.
2014-01-23 20:31 [Qemu-devel] [PATCH V6 0/8] Giving names to graph's BlockDriverState Benoît Canet
@ 2014-01-23 20:31 ` Benoît Canet
2014-01-23 20:31 ` [Qemu-devel] [PATCH V6 2/8] block: Allow the user to define "node-name" option both on command line and QMP Benoît Canet
` (7 subsequent siblings)
8 siblings, 0 replies; 29+ messages in thread
From: Benoît Canet @ 2014-01-23 20:31 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, Benoît Canet, armbru, mreitz, stefanha
From: Benoît Canet <benoit@irqsave.net>
Add the minimum of code to prepare for the following patches.
Signed-off-by: Benoit Canet <benoit@irqsave.net>
Reviewed-by: Fam Zheng <famz@redhat.com>
---
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 70722c2..60b70bc 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);
@@ -1599,7 +1602,7 @@ void bdrv_close_all(void)
{
BlockDriverState *bs;
- QTAILQ_FOREACH(bs, &bdrv_states, list) {
+ QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
bdrv_close(bs);
}
}
@@ -1628,7 +1631,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;
}
@@ -1655,7 +1658,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);
}
@@ -1664,14 +1667,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)
@@ -1725,7 +1733,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;
}
/*
@@ -2050,7 +2063,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) {
@@ -3208,11 +3221,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;
}
@@ -3220,19 +3234,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);
}
}
@@ -3252,7 +3281,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;
@@ -4376,7 +4405,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);
}
}
@@ -4385,7 +4414,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 a47f3d4..501b555 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -378,6 +378,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 2772f2f..f3f518c 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] 29+ messages in thread* [Qemu-devel] [PATCH V6 2/8] block: Allow the user to define "node-name" option both on command line and QMP.
2014-01-23 20:31 [Qemu-devel] [PATCH V6 0/8] Giving names to graph's BlockDriverState Benoît Canet
2014-01-23 20:31 ` [Qemu-devel] [PATCH V6 1/8] block: Add bs->node_name to hold the name of a bs node of the bs graph Benoît Canet
@ 2014-01-23 20:31 ` Benoît Canet
2014-01-24 14:51 ` Benoît Canet
2014-01-23 20:31 ` [Qemu-devel] [PATCH V6 3/8] qmp: Add QMP query-named-block-nodes to list the named BlockDriverState nodes Benoît Canet
` (6 subsequent siblings)
8 siblings, 1 reply; 29+ messages in thread
From: Benoît Canet @ 2014-01-23 20:31 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, Benoît Canet, armbru, mreitz, stefanha
From: Benoît Canet <benoit@irqsave.net>
Signed-off-by: Benoit Canet <benoit@irqsave.net>
---
block.c | 36 ++++++++++++++++++++++++++++++++++++
qapi-schema.json | 2 ++
2 files changed, 38 insertions(+)
diff --git a/block.c b/block.c
index 60b70bc..d9d02d2 100644
--- a/block.c
+++ b/block.c
@@ -728,6 +728,33 @@ static int bdrv_open_flags(BlockDriverState *bs, int flags)
return open_flags;
}
+static int bdrv_assign_node_name(BlockDriverState *bs,
+ const char *node_name,
+ Error **errp)
+{
+ 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);
+
+ return 0;
+}
+
/*
* Common part for opening disk images and files
*
@@ -738,6 +765,7 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
{
int ret, open_flags;
const char *filename;
+ const char *node_name = NULL;
Error *local_err = NULL;
assert(drv != NULL);
@@ -752,6 +780,14 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
trace_bdrv_open_common(bs, filename ?: "", flags, drv->format_name);
+ node_name = qdict_get_try_str(options, "node-name");
+ qdict_del(options, "node-name");
+
+ ret = bdrv_assign_node_name(bs, node_name, 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. */
diff --git a/qapi-schema.json b/qapi-schema.json
index 35f7b34..04167da 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -4090,6 +4090,7 @@
# @id: #optional id by which the new block device can be referred to.
# This is a required option on the top level of blockdev-add, and
# currently not allowed on any other level.
+# @node-name: #optional the name of a block driver state node (Since 2.0)
# @discard: #optional discard-related options (default: ignore)
# @cache: #optional cache-related options
# @aio: #optional AIO backend (default: threads)
@@ -4105,6 +4106,7 @@
{ 'type': 'BlockdevOptionsBase',
'data': { 'driver': 'str',
'*id': 'str',
+ '*node-name': 'str',
'*discard': 'BlockdevDiscardOptions',
'*cache': 'BlockdevCacheOptions',
'*aio': 'BlockdevAioOptions',
--
1.8.3.2
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH V6 2/8] block: Allow the user to define "node-name" option both on command line and QMP.
2014-01-23 20:31 ` [Qemu-devel] [PATCH V6 2/8] block: Allow the user to define "node-name" option both on command line and QMP Benoît Canet
@ 2014-01-24 14:51 ` Benoît Canet
2014-01-24 15:10 ` Kevin Wolf
0 siblings, 1 reply; 29+ messages in thread
From: Benoît Canet @ 2014-01-24 14:51 UTC (permalink / raw)
To: Benoît Canet; +Cc: kwolf, famz, armbru, qemu-devel, stefanha, mreitz
Le Thursday 23 Jan 2014 à 21:31:33 (+0100), Benoît Canet a écrit :
> From: Benoît Canet <benoit@irqsave.net>
>
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
> block.c | 36 ++++++++++++++++++++++++++++++++++++
> qapi-schema.json | 2 ++
> 2 files changed, 38 insertions(+)
>
> diff --git a/block.c b/block.c
> index 60b70bc..d9d02d2 100644
> --- a/block.c
> +++ b/block.c
> @@ -728,6 +728,33 @@ static int bdrv_open_flags(BlockDriverState *bs, int flags)
> return open_flags;
> }
>
> +static int bdrv_assign_node_name(BlockDriverState *bs,
> + const char *node_name,
> + Error **errp)
> +{
> + 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);
> +
> + return 0;
> +}
> +
> /*
> * Common part for opening disk images and files
> *
> @@ -738,6 +765,7 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
> {
> int ret, open_flags;
> const char *filename;
> + const char *node_name = NULL;
> Error *local_err = NULL;
>
> assert(drv != NULL);
> @@ -752,6 +780,14 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
>
> trace_bdrv_open_common(bs, filename ?: "", flags, drv->format_name);
>
> + node_name = qdict_get_try_str(options, "node-name");
> + qdict_del(options, "node-name");
Kevin: I wonder if I delete the option too early hence zeroing node-name ?
> +
> + ret = bdrv_assign_node_name(bs, node_name, 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. */
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 35f7b34..04167da 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -4090,6 +4090,7 @@
> # @id: #optional id by which the new block device can be referred to.
> # This is a required option on the top level of blockdev-add, and
> # currently not allowed on any other level.
> +# @node-name: #optional the name of a block driver state node (Since 2.0)
> # @discard: #optional discard-related options (default: ignore)
> # @cache: #optional cache-related options
> # @aio: #optional AIO backend (default: threads)
> @@ -4105,6 +4106,7 @@
> { 'type': 'BlockdevOptionsBase',
> 'data': { 'driver': 'str',
> '*id': 'str',
> + '*node-name': 'str',
> '*discard': 'BlockdevDiscardOptions',
> '*cache': 'BlockdevCacheOptions',
> '*aio': 'BlockdevAioOptions',
> --
> 1.8.3.2
>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH V6 2/8] block: Allow the user to define "node-name" option both on command line and QMP.
2014-01-24 14:51 ` Benoît Canet
@ 2014-01-24 15:10 ` Kevin Wolf
0 siblings, 0 replies; 29+ messages in thread
From: Kevin Wolf @ 2014-01-24 15:10 UTC (permalink / raw)
To: Benoît Canet; +Cc: armbru, famz, qemu-devel, stefanha, mreitz
Am 24.01.2014 um 15:51 hat Benoît Canet geschrieben:
> Le Thursday 23 Jan 2014 à 21:31:33 (+0100), Benoît Canet a écrit :
> > From: Benoît Canet <benoit@irqsave.net>
> >
> > Signed-off-by: Benoit Canet <benoit@irqsave.net>
> > ---
> > block.c | 36 ++++++++++++++++++++++++++++++++++++
> > qapi-schema.json | 2 ++
> > 2 files changed, 38 insertions(+)
> >
> > diff --git a/block.c b/block.c
> > index 60b70bc..d9d02d2 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -728,6 +728,33 @@ static int bdrv_open_flags(BlockDriverState *bs, int flags)
> > return open_flags;
> > }
> >
> > +static int bdrv_assign_node_name(BlockDriverState *bs,
> > + const char *node_name,
> > + Error **errp)
> > +{
> > + 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);
> > +
> > + return 0;
> > +}
> > +
> > /*
> > * Common part for opening disk images and files
> > *
> > @@ -738,6 +765,7 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
> > {
> > int ret, open_flags;
> > const char *filename;
> > + const char *node_name = NULL;
> > Error *local_err = NULL;
> >
> > assert(drv != NULL);
> > @@ -752,6 +780,14 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
> >
> > trace_bdrv_open_common(bs, filename ?: "", flags, drv->format_name);
> >
> > + node_name = qdict_get_try_str(options, "node-name");
>
> > + qdict_del(options, "node-name");
>
> Kevin: I wonder if I delete the option too early hence zeroing node-name ?
For the record, we concluded on IRC that I would squash the following
patch in:
diff --git a/block.c b/block.c
index 9dc1216..f043669 100644
--- a/block.c
+++ b/block.c
@@ -788,12 +788,11 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
trace_bdrv_open_common(bs, filename ?: "", flags, drv->format_name);
node_name = qdict_get_try_str(options, "node-name");
- qdict_del(options, "node-name");
-
ret = bdrv_assign_node_name(bs, node_name, errp);
if (ret < 0) {
return ret;
}
+ qdict_del(options, "node-name");
/* 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)
Kevin
> > +
> > + ret = bdrv_assign_node_name(bs, node_name, 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. */
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index 35f7b34..04167da 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -4090,6 +4090,7 @@
> > # @id: #optional id by which the new block device can be referred to.
> > # This is a required option on the top level of blockdev-add, and
> > # currently not allowed on any other level.
> > +# @node-name: #optional the name of a block driver state node (Since 2.0)
> > # @discard: #optional discard-related options (default: ignore)
> > # @cache: #optional cache-related options
> > # @aio: #optional AIO backend (default: threads)
> > @@ -4105,6 +4106,7 @@
> > { 'type': 'BlockdevOptionsBase',
> > 'data': { 'driver': 'str',
> > '*id': 'str',
> > + '*node-name': 'str',
> > '*discard': 'BlockdevDiscardOptions',
> > '*cache': 'BlockdevCacheOptions',
> > '*aio': 'BlockdevAioOptions',
> > --
> > 1.8.3.2
> >
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH V6 3/8] qmp: Add QMP query-named-block-nodes to list the named BlockDriverState nodes.
2014-01-23 20:31 [Qemu-devel] [PATCH V6 0/8] Giving names to graph's BlockDriverState Benoît Canet
2014-01-23 20:31 ` [Qemu-devel] [PATCH V6 1/8] block: Add bs->node_name to hold the name of a bs node of the bs graph Benoît Canet
2014-01-23 20:31 ` [Qemu-devel] [PATCH V6 2/8] block: Allow the user to define "node-name" option both on command line and QMP Benoît Canet
@ 2014-01-23 20:31 ` Benoît Canet
2014-01-23 20:31 ` [Qemu-devel] [PATCH V6 4/8] qmp: Allow to change password on named block driver states Benoît Canet
` (5 subsequent siblings)
8 siblings, 0 replies; 29+ messages in thread
From: Benoît Canet @ 2014-01-23 20:31 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, Benoît Canet, armbru, mreitz, stefanha
From: Benoît Canet <benoit@irqsave.net>
Signed-off-by: Benoit Canet <benoit@irqsave.net>
Reviewed-by: Fam Zheng <famz@redhat.com>
---
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 d9d02d2..9e45755 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"
@@ -3285,6 +3286,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 921ca67..130fc14 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1959,6 +1959,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 501b555..13654ed 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -379,6 +379,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 04167da..978632a 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',
@@ -2009,6 +2010,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 02cc815..11b44c5 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -3346,3 +3346,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 all the named block driver nodes
+
+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] 29+ messages in thread* [Qemu-devel] [PATCH V6 4/8] qmp: Allow to change password on named block driver states.
2014-01-23 20:31 [Qemu-devel] [PATCH V6 0/8] Giving names to graph's BlockDriverState Benoît Canet
` (2 preceding siblings ...)
2014-01-23 20:31 ` [Qemu-devel] [PATCH V6 3/8] qmp: Add QMP query-named-block-nodes to list the named BlockDriverState nodes Benoît Canet
@ 2014-01-23 20:31 ` Benoît Canet
2014-01-23 20:31 ` [Qemu-devel] [PATCH V6 5/8] block: Create authorizations mechanism for external snapshot and resize Benoît Canet
` (4 subsequent siblings)
8 siblings, 0 replies; 29+ messages in thread
From: Benoît Canet @ 2014-01-23 20:31 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, Benoît Canet, armbru, mreitz, stefanha
From: Benoît Canet <benoit@irqsave.net>
Signed-off-by: Benoit Canet <benoit@irqsave.net>
Reviewed-by: Fam Zheng <famz@redhat.com>
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 9e45755..e1bc732 100644
--- a/block.c
+++ b/block.c
@@ -3303,6 +3303,38 @@ BlockDeviceInfoList *bdrv_named_nodes_list(void)
return list;
}
+BlockDriverState *bdrv_lookup_bs(const char *device,
+ const char *node_name,
+ Error **errp)
+{
+ BlockDriverState *bs = NULL;
+
+ if ((!device && !node_name) || (device && node_name)) {
+ error_setg(errp, "Use either device or node-name but not both");
+ return NULL;
+ }
+
+ if (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 130fc14..3c27911 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1488,14 +1488,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 : NULL,
+ has_node_name ? node_name : NULL,
+ &local_err);
+ if (error_is_set(&local_err)) {
+ error_propagate(errp, local_err);
return;
}
diff --git a/hmp.c b/hmp.c
index 468f97d..5804a5a 100644
--- a/hmp.c
+++ b/hmp.c
@@ -871,7 +871,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 13654ed..b4a77e6 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -380,6 +380,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(const char *device,
+ 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 978632a..d7a674b 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 11b44c5..5492eb0 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1503,7 +1503,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,
},
@@ -1516,6 +1516,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] 29+ messages in thread* [Qemu-devel] [PATCH V6 5/8] block: Create authorizations mechanism for external snapshot and resize.
2014-01-23 20:31 [Qemu-devel] [PATCH V6 0/8] Giving names to graph's BlockDriverState Benoît Canet
` (3 preceding siblings ...)
2014-01-23 20:31 ` [Qemu-devel] [PATCH V6 4/8] qmp: Allow to change password on named block driver states Benoît Canet
@ 2014-01-23 20:31 ` Benoît Canet
2014-02-04 0:15 ` Jeff Cody
2014-01-23 20:31 ` [Qemu-devel] [PATCH V6 6/8] qmp: Allow block_resize to manipulate bs graph nodes Benoît Canet
` (3 subsequent siblings)
8 siblings, 1 reply; 29+ messages in thread
From: Benoît Canet @ 2014-01-23 20:31 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, Benoît Canet, armbru, mreitz, stefanha
From: Benoît Canet <benoit@irqsave.net>
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 e1bc732..3e0994b 100644
--- a/block.c
+++ b/block.c
@@ -5088,21 +5088,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 checks 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 a2e8f5f..cfcbcf4 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -404,7 +404,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 3c27911..8f95c7f 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1250,7 +1250,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 b4a77e6..59d9f12 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -287,16 +287,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 f3f518c..611a955 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 implementation 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] 29+ messages in thread* Re: [Qemu-devel] [PATCH V6 5/8] block: Create authorizations mechanism for external snapshot and resize.
2014-01-23 20:31 ` [Qemu-devel] [PATCH V6 5/8] block: Create authorizations mechanism for external snapshot and resize Benoît Canet
@ 2014-02-04 0:15 ` Jeff Cody
2014-02-04 10:25 ` Kevin Wolf
0 siblings, 1 reply; 29+ messages in thread
From: Jeff Cody @ 2014-02-04 0:15 UTC (permalink / raw)
To: Benoît Canet
Cc: kwolf, famz, Benoît Canet, qemu-devel, armbru, mreitz,
stefanha
On Thu, Jan 23, 2014 at 09:31:36PM +0100, Benoît Canet wrote:
> From: Benoît Canet <benoit@irqsave.net>
>
> 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 e1bc732..3e0994b 100644
> --- a/block.c
> +++ b/block.c
> @@ -5088,21 +5088,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;
> + }
This seems to break external snapshots; after this patch, I can no
longer perform live ext snapshots (on qcow2, raw, etc..), unless I am
doing something incorrectly.
Instead of checking for bs == candidate, was it intended to check to
see if !strcmp(bs->filename, candidiate->filename) was true?
Or maybe it is how bdrv_is_first_non_filter() is called (see below):
<snip>
> diff --git a/blockdev.c b/blockdev.c
> index 3c27911..8f95c7f 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1250,7 +1250,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)) {
Should this be: !bdrv_is_first_non_filter(state->old_bs->file)?
> error_set(errp, QERR_FEATURE_DISABLED, "snapshot");
> return;
> }
> diff --git a/include/block/block.h b/include/block/block.h
> index b4a77e6..59d9f12 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -287,16 +287,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 f3f518c..611a955 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 implementation 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 [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH V6 5/8] block: Create authorizations mechanism for external snapshot and resize.
2014-02-04 0:15 ` Jeff Cody
@ 2014-02-04 10:25 ` Kevin Wolf
2014-02-04 13:03 ` Jeff Cody
` (3 more replies)
0 siblings, 4 replies; 29+ messages in thread
From: Kevin Wolf @ 2014-02-04 10:25 UTC (permalink / raw)
To: Jeff Cody
Cc: Benoît Canet, famz, Benoît Canet, qemu-devel, armbru,
mreitz, stefanha
Am 04.02.2014 um 01:15 hat Jeff Cody geschrieben:
> On Thu, Jan 23, 2014 at 09:31:36PM +0100, Benoît Canet wrote:
> > From: Benoît Canet <benoit@irqsave.net>
> >
> > 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 e1bc732..3e0994b 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -5088,21 +5088,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;
> > + }
>
> This seems to break external snapshots; after this patch, I can no
> longer perform live ext snapshots (on qcow2, raw, etc..), unless I am
> doing something incorrectly.
>
> Instead of checking for bs == candidate, was it intended to check to
> see if !strcmp(bs->filename, candidiate->filename) was true?
If believe the problem is in bdrv_is_first_non_filter(): It starts with
bs->file, whereas the first non-filter is obviously the top-level BDS
itself. I suspect the following patch fixes it (it makes the simple
snapshotting case work again, but I'm not sure if it forbids everything
that should be forbidden).
In any case, this shows that...
- ...our testing is still lacking (no qemu-iotests case for live
snapshots? Seriously? Expect it to be broken then.)
- ...not all patch authors do a good share of manual testing
- ...I have relaxed my reviewing too much. I wasn't convinced that this
patch is right, because the whole logic confused me, but I couldn't
point to a bug. I shouldn't have merged it when in doubt.
Jeff, would you like to submit a qemu-iotests case for snapshotting?
Benoît, can you check whether the patch below is correct?
Kevin
diff --git a/block.c b/block.c
index ac0ccac..1299484 100644
--- a/block.c
+++ b/block.c
@@ -5412,11 +5412,7 @@ bool bdrv_is_first_non_filter(BlockDriverState *candidate)
QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
bool perm;
- if (!bs->file) {
- continue;
- }
-
- perm = bdrv_recurse_is_first_non_filter(bs->file, candidate);
+ perm = bdrv_recurse_is_first_non_filter(bs, candidate);
/* candidate is the first non filter */
if (perm) {
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH V6 5/8] block: Create authorizations mechanism for external snapshot and resize.
2014-02-04 10:25 ` Kevin Wolf
@ 2014-02-04 13:03 ` Jeff Cody
2014-02-10 19:39 ` Benoît Canet
` (2 subsequent siblings)
3 siblings, 0 replies; 29+ messages in thread
From: Jeff Cody @ 2014-02-04 13:03 UTC (permalink / raw)
To: Kevin Wolf
Cc: Benoît Canet, famz, Benoît Canet, qemu-devel, armbru,
mreitz, stefanha
On Tue, Feb 04, 2014 at 11:25:52AM +0100, Kevin Wolf wrote:
> Am 04.02.2014 um 01:15 hat Jeff Cody geschrieben:
> > On Thu, Jan 23, 2014 at 09:31:36PM +0100, Benoît Canet wrote:
> > > From: Benoît Canet <benoit@irqsave.net>
> > >
> > > 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 e1bc732..3e0994b 100644
> > > --- a/block.c
> > > +++ b/block.c
> > > @@ -5088,21 +5088,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;
> > > + }
> >
> > This seems to break external snapshots; after this patch, I can no
> > longer perform live ext snapshots (on qcow2, raw, etc..), unless I am
> > doing something incorrectly.
> >
> > Instead of checking for bs == candidate, was it intended to check to
> > see if !strcmp(bs->filename, candidiate->filename) was true?
>
> If believe the problem is in bdrv_is_first_non_filter(): It starts with
> bs->file, whereas the first non-filter is obviously the top-level BDS
> itself. I suspect the following patch fixes it (it makes the simple
> snapshotting case work again, but I'm not sure if it forbids everything
> that should be forbidden).
>
> In any case, this shows that...
>
> - ...our testing is still lacking (no qemu-iotests case for live
> snapshots? Seriously? Expect it to be broken then.)
>
> - ...not all patch authors do a good share of manual testing
>
> - ...I have relaxed my reviewing too much. I wasn't convinced that this
> patch is right, because the whole logic confused me, but I couldn't
> point to a bug. I shouldn't have merged it when in doubt.
>
> Jeff, would you like to submit a qemu-iotests case for snapshotting?
Yes, I'll do that.
>
> Benoît, can you check whether the patch below is correct?
>
> Kevin
>
>
> diff --git a/block.c b/block.c
> index ac0ccac..1299484 100644
> --- a/block.c
> +++ b/block.c
> @@ -5412,11 +5412,7 @@ bool bdrv_is_first_non_filter(BlockDriverState *candidate)
> QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> bool perm;
>
> - if (!bs->file) {
> - continue;
> - }
> -
> - perm = bdrv_recurse_is_first_non_filter(bs->file, candidate);
> + perm = bdrv_recurse_is_first_non_filter(bs, candidate);
>
> /* candidate is the first non filter */
> if (perm) {
>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH V6 5/8] block: Create authorizations mechanism for external snapshot and resize.
2014-02-04 10:25 ` Kevin Wolf
2014-02-04 13:03 ` Jeff Cody
@ 2014-02-10 19:39 ` Benoît Canet
2014-02-10 19:45 ` Benoît Canet
2014-02-10 20:18 ` Benoît Canet
3 siblings, 0 replies; 29+ messages in thread
From: Benoît Canet @ 2014-02-10 19:39 UTC (permalink / raw)
To: Kevin Wolf
Cc: Benoît Canet, famz, armbru, Jeff Cody, qemu-devel, mreitz,
stefanha
Le Tuesday 04 Feb 2014 à 11:25:52 (+0100), Kevin Wolf a écrit :
> Am 04.02.2014 um 01:15 hat Jeff Cody geschrieben:
> > On Thu, Jan 23, 2014 at 09:31:36PM +0100, Benoît Canet wrote:
> > > From: Benoît Canet <benoit@irqsave.net>
> > >
> > > 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 e1bc732..3e0994b 100644
> > > --- a/block.c
> > > +++ b/block.c
> > > @@ -5088,21 +5088,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;
> > > + }
> >
> > This seems to break external snapshots; after this patch, I can no
> > longer perform live ext snapshots (on qcow2, raw, etc..), unless I am
> > doing something incorrectly.
> >
> > Instead of checking for bs == candidate, was it intended to check to
> > see if !strcmp(bs->filename, candidiate->filename) was true?
>
> If believe the problem is in bdrv_is_first_non_filter(): It starts with
> bs->file, whereas the first non-filter is obviously the top-level BDS
> itself. I suspect the following patch fixes it (it makes the simple
> snapshotting case work again, but I'm not sure if it forbids everything
> that should be forbidden).
>
> In any case, this shows that...
>
> - ...our testing is still lacking (no qemu-iotests case for live
> snapshots? Seriously? Expect it to be broken then.)
>
> - ...not all patch authors do a good share of manual testing
>
> - ...I have relaxed my reviewing too much. I wasn't convinced that this
> patch is right, because the whole logic confused me, but I couldn't
> point to a bug. I shouldn't have merged it when in doubt.
>
> Jeff, would you like to submit a qemu-iotests case for snapshotting?
>
> Benoît, can you check whether the patch below is correct?
I just read this mail thread I will do the test.
Best regards
Benoît
>
> Kevin
>
>
> diff --git a/block.c b/block.c
> index ac0ccac..1299484 100644
> --- a/block.c
> +++ b/block.c
> @@ -5412,11 +5412,7 @@ bool bdrv_is_first_non_filter(BlockDriverState *candidate)
> QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> bool perm;
>
> - if (!bs->file) {
> - continue;
> - }
> -
> - perm = bdrv_recurse_is_first_non_filter(bs->file, candidate);
> + perm = bdrv_recurse_is_first_non_filter(bs, candidate);
>
> /* candidate is the first non filter */
> if (perm) {
>
>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH V6 5/8] block: Create authorizations mechanism for external snapshot and resize.
2014-02-04 10:25 ` Kevin Wolf
2014-02-04 13:03 ` Jeff Cody
2014-02-10 19:39 ` Benoît Canet
@ 2014-02-10 19:45 ` Benoît Canet
2014-02-10 20:18 ` Benoît Canet
3 siblings, 0 replies; 29+ messages in thread
From: Benoît Canet @ 2014-02-10 19:45 UTC (permalink / raw)
To: Kevin Wolf
Cc: Benoît Canet, famz, armbru, Jeff Cody, qemu-devel, mreitz,
stefanha
Le Tuesday 04 Feb 2014 à 11:25:52 (+0100), Kevin Wolf a écrit :
> Am 04.02.2014 um 01:15 hat Jeff Cody geschrieben:
> > On Thu, Jan 23, 2014 at 09:31:36PM +0100, Benoît Canet wrote:
> > > From: Benoît Canet <benoit@irqsave.net>
> > >
> > > 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 e1bc732..3e0994b 100644
> > > --- a/block.c
> > > +++ b/block.c
> > > @@ -5088,21 +5088,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;
> > > + }
> >
> > This seems to break external snapshots; after this patch, I can no
> > longer perform live ext snapshots (on qcow2, raw, etc..), unless I am
> > doing something incorrectly.
> >
> > Instead of checking for bs == candidate, was it intended to check to
> > see if !strcmp(bs->filename, candidiate->filename) was true?
>
> If believe the problem is in bdrv_is_first_non_filter(): It starts with
> bs->file, whereas the first non-filter is obviously the top-level BDS
> itself. I suspect the following patch fixes it (it makes the simple
> snapshotting case work again, but I'm not sure if it forbids everything
> that should be forbidden).
>
> In any case, this shows that...
>
> - ...our testing is still lacking (no qemu-iotests case for live
> snapshots? Seriously? Expect it to be broken then.)
>
> - ...not all patch authors do a good share of manual testing
>
> - ...I have relaxed my reviewing too much. I wasn't convinced that this
> patch is right, because the whole logic confused me, but I couldn't
> point to a bug. I shouldn't have merged it when in doubt.
>
> Jeff, would you like to submit a qemu-iotests case for snapshotting?
>
> Benoît, can you check whether the patch below is correct?
>
> Kevin
This patch is not correct I will dig the issue.
Best regards
Benoît
feel free to ping me on irc when you fell I miss an email.
>
>
> diff --git a/block.c b/block.c
> index ac0ccac..1299484 100644
> --- a/block.c
> +++ b/block.c
> @@ -5412,11 +5412,7 @@ bool bdrv_is_first_non_filter(BlockDriverState *candidate)
> QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> bool perm;
>
> - if (!bs->file) {
> - continue;
> - }
> -
> - perm = bdrv_recurse_is_first_non_filter(bs->file, candidate);
> + perm = bdrv_recurse_is_first_non_filter(bs, candidate);
>
> /* candidate is the first non filter */
> if (perm) {
>
>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH V6 5/8] block: Create authorizations mechanism for external snapshot and resize.
2014-02-04 10:25 ` Kevin Wolf
` (2 preceding siblings ...)
2014-02-10 19:45 ` Benoît Canet
@ 2014-02-10 20:18 ` Benoît Canet
3 siblings, 0 replies; 29+ messages in thread
From: Benoît Canet @ 2014-02-10 20:18 UTC (permalink / raw)
To: Kevin Wolf
Cc: Benoît Canet, famz, armbru, Jeff Cody, qemu-devel, mreitz,
stefanha
Le Tuesday 04 Feb 2014 à 11:25:52 (+0100), Kevin Wolf a écrit :
> Am 04.02.2014 um 01:15 hat Jeff Cody geschrieben:
> > On Thu, Jan 23, 2014 at 09:31:36PM +0100, Benoît Canet wrote:
> > > From: Benoît Canet <benoit@irqsave.net>
> > >
> > > 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 e1bc732..3e0994b 100644
> > > --- a/block.c
> > > +++ b/block.c
> > > @@ -5088,21 +5088,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;
> > > + }
> >
> > This seems to break external snapshots; after this patch, I can no
> > longer perform live ext snapshots (on qcow2, raw, etc..), unless I am
> > doing something incorrectly.
> >
> > Instead of checking for bs == candidate, was it intended to check to
> > see if !strcmp(bs->filename, candidiate->filename) was true?
>
> If believe the problem is in bdrv_is_first_non_filter(): It starts with
> bs->file, whereas the first non-filter is obviously the top-level BDS
> itself. I suspect the following patch fixes it (it makes the simple
> snapshotting case work again, but I'm not sure if it forbids everything
> that should be forbidden).
>
> In any case, this shows that...
>
> - ...our testing is still lacking (no qemu-iotests case for live
> snapshots? Seriously? Expect it to be broken then.)
>
> - ...not all patch authors do a good share of manual testing
>
> - ...I have relaxed my reviewing too much. I wasn't convinced that this
> patch is right, because the whole logic confused me, but I couldn't
> point to a bug. I shouldn't have merged it when in doubt.
>
> Jeff, would you like to submit a qemu-iotests case for snapshotting?
>
> Benoît, can you check whether the patch below is correct?
>
> Kevin
I have a patch that I will post.
I was confused by the fact that when a quorum driver is running it's not on the
top of the tree but on bs->file.
I tested regular snapshot and quorum single file snapshots and it works with the
new patch.
Best regards
Benoît
>
>
> diff --git a/block.c b/block.c
> index ac0ccac..1299484 100644
> --- a/block.c
> +++ b/block.c
> @@ -5412,11 +5412,7 @@ bool bdrv_is_first_non_filter(BlockDriverState *candidate)
> QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> bool perm;
>
> - if (!bs->file) {
> - continue;
> - }
> -
> - perm = bdrv_recurse_is_first_non_filter(bs->file, candidate);
> + perm = bdrv_recurse_is_first_non_filter(bs, candidate);
>
> /* candidate is the first non filter */
> if (perm) {
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH V6 6/8] qmp: Allow block_resize to manipulate bs graph nodes.
2014-01-23 20:31 [Qemu-devel] [PATCH V6 0/8] Giving names to graph's BlockDriverState Benoît Canet
` (4 preceding siblings ...)
2014-01-23 20:31 ` [Qemu-devel] [PATCH V6 5/8] block: Create authorizations mechanism for external snapshot and resize Benoît Canet
@ 2014-01-23 20:31 ` Benoît Canet
2014-01-23 20:31 ` [Qemu-devel] [PATCH V6 7/8] qmp: Allow to take external snapshots on bs graphs node Benoît Canet
` (2 subsequent siblings)
8 siblings, 0 replies; 29+ messages in thread
From: Benoît Canet @ 2014-01-23 20:31 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, Benoît Canet, armbru, mreitz, stefanha
From: Benoît Canet <benoit@irqsave.net>
Signed-off-by: Benoit Canet <benoit@irqsave.net>
Reviewed-by: Fam Zheng <famz@redhat.com>
---
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 8f95c7f..e8e110c 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1690,14 +1690,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 : NULL,
+ has_node_name ? node_name : NULL,
+ &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 5804a5a..66c8d7e 100644
--- a/hmp.c
+++ b/hmp.c
@@ -893,7 +893,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 d7a674b..ffa9430 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 5492eb0..a45f26c 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -931,7 +931,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,
},
@@ -944,6 +944,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] 29+ messages in thread* [Qemu-devel] [PATCH V6 7/8] qmp: Allow to take external snapshots on bs graphs node.
2014-01-23 20:31 [Qemu-devel] [PATCH V6 0/8] Giving names to graph's BlockDriverState Benoît Canet
` (5 preceding siblings ...)
2014-01-23 20:31 ` [Qemu-devel] [PATCH V6 6/8] qmp: Allow block_resize to manipulate bs graph nodes Benoît Canet
@ 2014-01-23 20:31 ` Benoît Canet
2014-01-23 20:31 ` [Qemu-devel] [PATCH V6 8/8] block: Use graph node name as reference in bdrv_file_open() Benoît Canet
2014-01-24 13:27 ` [Qemu-devel] [PATCH V6 0/8] Giving names to graph's BlockDriverState Kevin Wolf
8 siblings, 0 replies; 29+ messages in thread
From: Benoît Canet @ 2014-01-23 20:31 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, Benoît Canet, armbru, mreitz, stefanha
From: Benoît Canet <benoit@irqsave.net>
Signed-off-by: Benoit Canet <benoit@irqsave.net>
Reviewed-by: Fam Zheng <famz@redhat.com>
---
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 e8e110c..78e5bb9 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -954,14 +954,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,
@@ -1199,8 +1207,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;
@@ -1211,7 +1225,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;
@@ -1227,9 +1248,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 : NULL,
+ has_node_name ? node_name : NULL,
+ &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;
}
@@ -1269,15 +1302,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 66c8d7e..1af0809 100644
--- a/hmp.c
+++ b/hmp.c
@@ -972,7 +972,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 ffa9430..f90fcde 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 a45f26c..4f28250 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1089,7 +1089,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")
@@ -1104,6 +1106,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",
@@ -1117,7 +1124,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,
},
@@ -1134,7 +1141,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] 29+ messages in thread* [Qemu-devel] [PATCH V6 8/8] block: Use graph node name as reference in bdrv_file_open().
2014-01-23 20:31 [Qemu-devel] [PATCH V6 0/8] Giving names to graph's BlockDriverState Benoît Canet
` (6 preceding siblings ...)
2014-01-23 20:31 ` [Qemu-devel] [PATCH V6 7/8] qmp: Allow to take external snapshots on bs graphs node Benoît Canet
@ 2014-01-23 20:31 ` Benoît Canet
2014-01-24 13:26 ` Kevin Wolf
2014-01-24 13:27 ` [Qemu-devel] [PATCH V6 0/8] Giving names to graph's BlockDriverState Kevin Wolf
8 siblings, 1 reply; 29+ messages in thread
From: Benoît Canet @ 2014-01-23 20:31 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, Benoît Canet, famz, Benoit Canet, armbru, mreitz,
stefanha
Signed-off-by: Benoit Canet <benoit@irqsave.net>
---
block.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/block.c b/block.c
index 3e0994b..7726636 100644
--- a/block.c
+++ b/block.c
@@ -908,15 +908,15 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename,
if (reference) {
if (filename || qdict_size(options)) {
- error_setg(errp, "Cannot reference an existing block device with "
+ error_setg(errp, "Cannot reference an existing graph node with "
"additional options or a new filename");
return -EINVAL;
}
QDECREF(options);
- bs = bdrv_find(reference);
+ bs = bdrv_find_node(reference);
if (!bs) {
- error_setg(errp, "Cannot find block device '%s'", reference);
+ error_setg(errp, "Cannot find graph node '%s'", reference);
return -ENODEV;
}
bdrv_ref(bs);
--
1.8.3.2
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH V6 8/8] block: Use graph node name as reference in bdrv_file_open().
2014-01-23 20:31 ` [Qemu-devel] [PATCH V6 8/8] block: Use graph node name as reference in bdrv_file_open() Benoît Canet
@ 2014-01-24 13:26 ` Kevin Wolf
2014-01-24 13:37 ` Max Reitz
0 siblings, 1 reply; 29+ messages in thread
From: Kevin Wolf @ 2014-01-24 13:26 UTC (permalink / raw)
To: Benoît Canet
Cc: famz, Benoit Canet, armbru, qemu-devel, stefanha, mreitz
Am 23.01.2014 um 21:31 hat Benoît Canet geschrieben:
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
> block.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
I'm not going to merge this one yet. It breaks qemu-iotests case 071,
which would have to be adapted.
However, first of all I'd like to hear the opinions of at least Eric and
Max on what BlockRef should really refer to. I think node names make
most sense, but perhaps it's a bit inconvenient and the command line
should default to node-name = id when id is set, but node-name isn't?
Kevin
> diff --git a/block.c b/block.c
> index 3e0994b..7726636 100644
> --- a/block.c
> +++ b/block.c
> @@ -908,15 +908,15 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename,
>
> if (reference) {
> if (filename || qdict_size(options)) {
> - error_setg(errp, "Cannot reference an existing block device with "
> + error_setg(errp, "Cannot reference an existing graph node with "
> "additional options or a new filename");
> return -EINVAL;
> }
> QDECREF(options);
>
> - bs = bdrv_find(reference);
> + bs = bdrv_find_node(reference);
> if (!bs) {
> - error_setg(errp, "Cannot find block device '%s'", reference);
> + error_setg(errp, "Cannot find graph node '%s'", reference);
> return -ENODEV;
> }
> bdrv_ref(bs);
> --
> 1.8.3.2
>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH V6 8/8] block: Use graph node name as reference in bdrv_file_open().
2014-01-24 13:26 ` Kevin Wolf
@ 2014-01-24 13:37 ` Max Reitz
2014-01-24 14:48 ` Kevin Wolf
0 siblings, 1 reply; 29+ messages in thread
From: Max Reitz @ 2014-01-24 13:37 UTC (permalink / raw)
To: Kevin Wolf, Benoît Canet
Cc: famz, Benoit Canet, qemu-devel, armbru, stefanha
On 24.01.2014 14:26, Kevin Wolf wrote:
> Am 23.01.2014 um 21:31 hat Benoît Canet geschrieben:
>> Signed-off-by: Benoit Canet <benoit@irqsave.net>
>> ---
>> block.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
> I'm not going to merge this one yet. It breaks qemu-iotests case 071,
> which would have to be adapted.
>
> However, first of all I'd like to hear the opinions of at least Eric and
> Max on what BlockRef should really refer to. I think node names make
> most sense, but perhaps it's a bit inconvenient and the command line
> should default to node-name = id when id is set, but node-name isn't?
The QAPI schema is pretty clear about this: “references the ID of an
existing block device.” However, if the ID cannot be found, I think we
should interpret it as a reference to the node name.
Therefore, I'd first try bdrv_find() and if that returns NULL, try again
with bdrv_find_node().
Max
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH V6 8/8] block: Use graph node name as reference in bdrv_file_open().
2014-01-24 13:37 ` Max Reitz
@ 2014-01-24 14:48 ` Kevin Wolf
2014-01-24 14:54 ` Max Reitz
0 siblings, 1 reply; 29+ messages in thread
From: Kevin Wolf @ 2014-01-24 14:48 UTC (permalink / raw)
To: Max Reitz
Cc: Benoît Canet, famz, Benoit Canet, qemu-devel, armbru,
stefanha
Am 24.01.2014 um 14:37 hat Max Reitz geschrieben:
> On 24.01.2014 14:26, Kevin Wolf wrote:
> >Am 23.01.2014 um 21:31 hat Benoît Canet geschrieben:
> >>Signed-off-by: Benoit Canet <benoit@irqsave.net>
> >>---
> >> block.c | 6 +++---
> >> 1 file changed, 3 insertions(+), 3 deletions(-)
> >I'm not going to merge this one yet. It breaks qemu-iotests case 071,
> >which would have to be adapted.
> >
> >However, first of all I'd like to hear the opinions of at least Eric and
> >Max on what BlockRef should really refer to. I think node names make
> >most sense, but perhaps it's a bit inconvenient and the command line
> >should default to node-name = id when id is set, but node-name isn't?
>
> The QAPI schema is pretty clear about this: “references the ID of an
> existing block device.”
Sure, that's because I wrote that text before we had a node name.
However, in 1.7 references didn't work yet, so we still have all freedom
to change the interface as we like.
> However, if the ID cannot be found, I think
> we should interpret it as a reference to the node name.
>
> Therefore, I'd first try bdrv_find() and if that returns NULL, try
> again with bdrv_find_node().
I think I would prefer to avoid such ambiguities. Otherwise a management
tool that wants to use the node name needs to check first if it's not
already used as a device name somewhere else and would therefore operate
on the wrong device.
On the other hand, a management tool using the same names for devices
and nodes just gets what it deserves.
Perhaps we should use a common namespace for both, i.e. you get an error
if you try to assign a node name that is already a device name and vice
versa?
Kevin
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH V6 8/8] block: Use graph node name as reference in bdrv_file_open().
2014-01-24 14:48 ` Kevin Wolf
@ 2014-01-24 14:54 ` Max Reitz
2014-01-27 14:36 ` Benoît Canet
0 siblings, 1 reply; 29+ messages in thread
From: Max Reitz @ 2014-01-24 14:54 UTC (permalink / raw)
To: Kevin Wolf
Cc: Benoît Canet, famz, Benoit Canet, qemu-devel, armbru,
stefanha
On 24.01.2014 15:48, Kevin Wolf wrote:
> Am 24.01.2014 um 14:37 hat Max Reitz geschrieben:
>> On 24.01.2014 14:26, Kevin Wolf wrote:
>>> Am 23.01.2014 um 21:31 hat Benoît Canet geschrieben:
>>>> Signed-off-by: Benoit Canet <benoit@irqsave.net>
>>>> ---
>>>> block.c | 6 +++---
>>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>> I'm not going to merge this one yet. It breaks qemu-iotests case 071,
>>> which would have to be adapted.
>>>
>>> However, first of all I'd like to hear the opinions of at least Eric and
>>> Max on what BlockRef should really refer to. I think node names make
>>> most sense, but perhaps it's a bit inconvenient and the command line
>>> should default to node-name = id when id is set, but node-name isn't?
>> The QAPI schema is pretty clear about this: “references the ID of an
>> existing block device.”
> Sure, that's because I wrote that text before we had a node name.
>
> However, in 1.7 references didn't work yet, so we still have all freedom
> to change the interface as we like.
Yes, that's right.
>> However, if the ID cannot be found, I think
>> we should interpret it as a reference to the node name.
>>
>> Therefore, I'd first try bdrv_find() and if that returns NULL, try
>> again with bdrv_find_node().
> I think I would prefer to avoid such ambiguities. Otherwise a management
> tool that wants to use the node name needs to check first if it's not
> already used as a device name somewhere else and would therefore operate
> on the wrong device.
>
> On the other hand, a management tool using the same names for devices
> and nodes just gets what it deserves.
>
> Perhaps we should use a common namespace for both, i.e. you get an error
> if you try to assign a node name that is already a device name and vice
> versa?
This is what I would go for. However, then I don't really know why we
should separate the ID and the node name in the first place (although
that's probably because I haven't followed the discussion around node
names).
Max
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH V6 8/8] block: Use graph node name as reference in bdrv_file_open().
2014-01-24 14:54 ` Max Reitz
@ 2014-01-27 14:36 ` Benoît Canet
2014-01-27 19:11 ` Max Reitz
0 siblings, 1 reply; 29+ messages in thread
From: Benoît Canet @ 2014-01-27 14:36 UTC (permalink / raw)
To: Max Reitz
Cc: Kevin Wolf, Benoît Canet, famz, qemu-devel, armbru, stefanha
Le Friday 24 Jan 2014 à 15:54:39 (+0100), Max Reitz a écrit :
> On 24.01.2014 15:48, Kevin Wolf wrote:
> >Am 24.01.2014 um 14:37 hat Max Reitz geschrieben:
> >>On 24.01.2014 14:26, Kevin Wolf wrote:
> >>>Am 23.01.2014 um 21:31 hat Benoît Canet geschrieben:
> >>>>Signed-off-by: Benoit Canet <benoit@irqsave.net>
> >>>>---
> >>>> block.c | 6 +++---
> >>>> 1 file changed, 3 insertions(+), 3 deletions(-)
> >>>I'm not going to merge this one yet. It breaks qemu-iotests case 071,
> >>>which would have to be adapted.
> >>>
> >>>However, first of all I'd like to hear the opinions of at least Eric and
> >>>Max on what BlockRef should really refer to. I think node names make
> >>>most sense, but perhaps it's a bit inconvenient and the command line
> >>>should default to node-name = id when id is set, but node-name isn't?
> >>The QAPI schema is pretty clear about this: “references the ID of an
> >>existing block device.”
> >Sure, that's because I wrote that text before we had a node name.
> >
> >However, in 1.7 references didn't work yet, so we still have all freedom
> >to change the interface as we like.
>
> Yes, that's right.
>
> >>However, if the ID cannot be found, I think
> >>we should interpret it as a reference to the node name.
> >>
> >>Therefore, I'd first try bdrv_find() and if that returns NULL, try
> >>again with bdrv_find_node().
> >I think I would prefer to avoid such ambiguities. Otherwise a management
> >tool that wants to use the node name needs to check first if it's not
> >already used as a device name somewhere else and would therefore operate
> >on the wrong device.
> >
> >On the other hand, a management tool using the same names for devices
> >and nodes just gets what it deserves.
> >
> >Perhaps we should use a common namespace for both, i.e. you get an error
> >if you try to assign a node name that is already a device name and vice
> >versa?
>
> This is what I would go for. However, then I don't really know why
> we should separate the ID and the node name in the first place
> (although that's probably because I haven't followed the discussion
> around node names).
>
> Max
Ping,
I still want to make quorum merge.
What should be done for the references ?
Best regards
Benoît
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH V6 8/8] block: Use graph node name as reference in bdrv_file_open().
2014-01-27 14:36 ` Benoît Canet
@ 2014-01-27 19:11 ` Max Reitz
2014-01-28 0:04 ` Benoît Canet
0 siblings, 1 reply; 29+ messages in thread
From: Max Reitz @ 2014-01-27 19:11 UTC (permalink / raw)
To: Benoît Canet; +Cc: Kevin Wolf, famz, qemu-devel, armbru, stefanha
On 27.01.2014 15:36, Benoît Canet wrote:
> Le Friday 24 Jan 2014 à 15:54:39 (+0100), Max Reitz a écrit :
>> On 24.01.2014 15:48, Kevin Wolf wrote:
>>> Am 24.01.2014 um 14:37 hat Max Reitz geschrieben:
>>>> On 24.01.2014 14:26, Kevin Wolf wrote:
>>>>> Am 23.01.2014 um 21:31 hat Benoît Canet geschrieben:
>>>>>> Signed-off-by: Benoit Canet <benoit@irqsave.net>
>>>>>> ---
>>>>>> block.c | 6 +++---
>>>>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>>> I'm not going to merge this one yet. It breaks qemu-iotests case 071,
>>>>> which would have to be adapted.
>>>>>
>>>>> However, first of all I'd like to hear the opinions of at least Eric and
>>>>> Max on what BlockRef should really refer to. I think node names make
>>>>> most sense, but perhaps it's a bit inconvenient and the command line
>>>>> should default to node-name = id when id is set, but node-name isn't?
>>>> The QAPI schema is pretty clear about this: “references the ID of an
>>>> existing block device.”
>>> Sure, that's because I wrote that text before we had a node name.
>>>
>>> However, in 1.7 references didn't work yet, so we still have all freedom
>>> to change the interface as we like.
>> Yes, that's right.
>>
>>>> However, if the ID cannot be found, I think
>>>> we should interpret it as a reference to the node name.
>>>>
>>>> Therefore, I'd first try bdrv_find() and if that returns NULL, try
>>>> again with bdrv_find_node().
>>> I think I would prefer to avoid such ambiguities. Otherwise a management
>>> tool that wants to use the node name needs to check first if it's not
>>> already used as a device name somewhere else and would therefore operate
>>> on the wrong device.
>>>
>>> On the other hand, a management tool using the same names for devices
>>> and nodes just gets what it deserves.
>>>
>>> Perhaps we should use a common namespace for both, i.e. you get an error
>>> if you try to assign a node name that is already a device name and vice
>>> versa?
>> This is what I would go for. However, then I don't really know why
>> we should separate the ID and the node name in the first place
>> (although that's probably because I haven't followed the discussion
>> around node names).
>>
>> Max
> Ping,
>
> I still want to make quorum merge.
> What should be done for the references ?
>
> Best regards
>
> Benoît
My only problem is that I don't really know what IDs are for, then. ;-)
Currently, I think using the node name is probably (more) correct and it
can't hurt anyone; thus, I'm okay with this patch.
Max
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH V6 8/8] block: Use graph node name as reference in bdrv_file_open().
2014-01-27 19:11 ` Max Reitz
@ 2014-01-28 0:04 ` Benoît Canet
2014-01-31 20:32 ` Max Reitz
0 siblings, 1 reply; 29+ messages in thread
From: Benoît Canet @ 2014-01-28 0:04 UTC (permalink / raw)
To: Max Reitz
Cc: Benoît Canet, Kevin Wolf, famz, qemu-devel, armbru, stefanha
Le Monday 27 Jan 2014 à 20:11:59 (+0100), Max Reitz a écrit :
> On 27.01.2014 15:36, Benoît Canet wrote:
> >Le Friday 24 Jan 2014 à 15:54:39 (+0100), Max Reitz a écrit :
> >>On 24.01.2014 15:48, Kevin Wolf wrote:
> >>>Am 24.01.2014 um 14:37 hat Max Reitz geschrieben:
> >>>>On 24.01.2014 14:26, Kevin Wolf wrote:
> >>>>>Am 23.01.2014 um 21:31 hat Benoît Canet geschrieben:
> >>>>>>Signed-off-by: Benoit Canet <benoit@irqsave.net>
> >>>>>>---
> >>>>>> block.c | 6 +++---
> >>>>>> 1 file changed, 3 insertions(+), 3 deletions(-)
> >>>>>I'm not going to merge this one yet. It breaks qemu-iotests case 071,
> >>>>>which would have to be adapted.
> >>>>>
> >>>>>However, first of all I'd like to hear the opinions of at least Eric and
> >>>>>Max on what BlockRef should really refer to. I think node names make
> >>>>>most sense, but perhaps it's a bit inconvenient and the command line
> >>>>>should default to node-name = id when id is set, but node-name isn't?
> >>>>The QAPI schema is pretty clear about this: “references the ID of an
> >>>>existing block device.”
> >>>Sure, that's because I wrote that text before we had a node name.
> >>>
> >>>However, in 1.7 references didn't work yet, so we still have all freedom
> >>>to change the interface as we like.
> >>Yes, that's right.
> >>
> >>>>However, if the ID cannot be found, I think
> >>>>we should interpret it as a reference to the node name.
> >>>>
> >>>>Therefore, I'd first try bdrv_find() and if that returns NULL, try
> >>>>again with bdrv_find_node().
> >>>I think I would prefer to avoid such ambiguities. Otherwise a management
> >>>tool that wants to use the node name needs to check first if it's not
> >>>already used as a device name somewhere else and would therefore operate
> >>>on the wrong device.
> >>>
> >>>On the other hand, a management tool using the same names for devices
> >>>and nodes just gets what it deserves.
> >>>
> >>>Perhaps we should use a common namespace for both, i.e. you get an error
> >>>if you try to assign a node name that is already a device name and vice
> >>>versa?
> >>This is what I would go for. However, then I don't really know why
> >>we should separate the ID and the node name in the first place
> >>(although that's probably because I haven't followed the discussion
> >>around node names).
> >>
> >>Max
> >Ping,
> >
> >I still want to make quorum merge.
> >What should be done for the references ?
> >
> >Best regards
> >
> >Benoît
>
> My only problem is that I don't really know what IDs are for, then. ;-)
>
>From the understanding I have ID are for block backend top level bds and
node-name naming all the bds burried in the graph.
So my personal opinion would be to relax the constraint on bdrv_lookup_bs
and use it for references.
Kevin && Max: what do you think of this scheme ?
Best regards
Benoît
> Currently, I think using the node name is probably (more) correct
> and it can't hurt anyone; thus, I'm okay with this patch.
>
> Max
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH V6 8/8] block: Use graph node name as reference in bdrv_file_open().
2014-01-28 0:04 ` Benoît Canet
@ 2014-01-31 20:32 ` Max Reitz
2014-01-31 21:37 ` Benoît Canet
0 siblings, 1 reply; 29+ messages in thread
From: Max Reitz @ 2014-01-31 20:32 UTC (permalink / raw)
To: Benoît Canet; +Cc: Kevin Wolf, famz, qemu-devel, armbru, stefanha
On 28.01.2014 01:04, Benoît Canet wrote:
> Le Monday 27 Jan 2014 à 20:11:59 (+0100), Max Reitz a écrit :
>> On 27.01.2014 15:36, Benoît Canet wrote:
>>> Le Friday 24 Jan 2014 à 15:54:39 (+0100), Max Reitz a écrit :
>>>> On 24.01.2014 15:48, Kevin Wolf wrote:
>>>>> Am 24.01.2014 um 14:37 hat Max Reitz geschrieben:
>>>>>> On 24.01.2014 14:26, Kevin Wolf wrote:
>>>>>>> Am 23.01.2014 um 21:31 hat Benoît Canet geschrieben:
>>>>>>>> Signed-off-by: Benoit Canet <benoit@irqsave.net>
>>>>>>>> ---
>>>>>>>> block.c | 6 +++---
>>>>>>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>>>>> I'm not going to merge this one yet. It breaks qemu-iotests case 071,
>>>>>>> which would have to be adapted.
>>>>>>>
>>>>>>> However, first of all I'd like to hear the opinions of at least Eric and
>>>>>>> Max on what BlockRef should really refer to. I think node names make
>>>>>>> most sense, but perhaps it's a bit inconvenient and the command line
>>>>>>> should default to node-name = id when id is set, but node-name isn't?
>>>>>> The QAPI schema is pretty clear about this: “references the ID of an
>>>>>> existing block device.”
>>>>> Sure, that's because I wrote that text before we had a node name.
>>>>>
>>>>> However, in 1.7 references didn't work yet, so we still have all freedom
>>>>> to change the interface as we like.
>>>> Yes, that's right.
>>>>
>>>>>> However, if the ID cannot be found, I think
>>>>>> we should interpret it as a reference to the node name.
>>>>>>
>>>>>> Therefore, I'd first try bdrv_find() and if that returns NULL, try
>>>>>> again with bdrv_find_node().
>>>>> I think I would prefer to avoid such ambiguities. Otherwise a management
>>>>> tool that wants to use the node name needs to check first if it's not
>>>>> already used as a device name somewhere else and would therefore operate
>>>>> on the wrong device.
>>>>>
>>>>> On the other hand, a management tool using the same names for devices
>>>>> and nodes just gets what it deserves.
>>>>>
>>>>> Perhaps we should use a common namespace for both, i.e. you get an error
>>>>> if you try to assign a node name that is already a device name and vice
>>>>> versa?
>>>> This is what I would go for. However, then I don't really know why
>>>> we should separate the ID and the node name in the first place
>>>> (although that's probably because I haven't followed the discussion
>>>> around node names).
>>>>
>>>> Max
>>> Ping,
>>>
>>> I still want to make quorum merge.
>>> What should be done for the references ?
>>>
>>> Best regards
>>>
>>> Benoît
>> My only problem is that I don't really know what IDs are for, then. ;-)
>>
> From the understanding I have ID are for block backend top level bds and
> node-name naming all the bds burried in the graph.
>
> So my personal opinion would be to relax the constraint on bdrv_lookup_bs
> and use it for references.
>
> Kevin && Max: what do you think of this scheme ?
I agree. For example, we could change the constraint to report an error
only if both ID and node name are actually valid (and point to different
devices), that is, bdrv_find() and bdrv_find_node() return different
non-NULL values.
Max
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH V6 8/8] block: Use graph node name as reference in bdrv_file_open().
2014-01-31 20:32 ` Max Reitz
@ 2014-01-31 21:37 ` Benoît Canet
2014-02-03 9:43 ` Kevin Wolf
0 siblings, 1 reply; 29+ messages in thread
From: Benoît Canet @ 2014-01-31 21:37 UTC (permalink / raw)
To: Max Reitz
Cc: Benoît Canet, Kevin Wolf, famz, qemu-devel, armbru, stefanha
Le Friday 31 Jan 2014 à 21:32:34 (+0100), Max Reitz a écrit :
> On 28.01.2014 01:04, Benoît Canet wrote:
> >Le Monday 27 Jan 2014 à 20:11:59 (+0100), Max Reitz a écrit :
> >>On 27.01.2014 15:36, Benoît Canet wrote:
> >>>Le Friday 24 Jan 2014 à 15:54:39 (+0100), Max Reitz a écrit :
> >>>>On 24.01.2014 15:48, Kevin Wolf wrote:
> >>>>>Am 24.01.2014 um 14:37 hat Max Reitz geschrieben:
> >>>>>>On 24.01.2014 14:26, Kevin Wolf wrote:
> >>>>>>>Am 23.01.2014 um 21:31 hat Benoît Canet geschrieben:
> >>>>>>>>Signed-off-by: Benoit Canet <benoit@irqsave.net>
> >>>>>>>>---
> >>>>>>>> block.c | 6 +++---
> >>>>>>>> 1 file changed, 3 insertions(+), 3 deletions(-)
> >>>>>>>I'm not going to merge this one yet. It breaks qemu-iotests case 071,
> >>>>>>>which would have to be adapted.
> >>>>>>>
> >>>>>>>However, first of all I'd like to hear the opinions of at least Eric and
> >>>>>>>Max on what BlockRef should really refer to. I think node names make
> >>>>>>>most sense, but perhaps it's a bit inconvenient and the command line
> >>>>>>>should default to node-name = id when id is set, but node-name isn't?
> >>>>>>The QAPI schema is pretty clear about this: “references the ID of an
> >>>>>>existing block device.”
> >>>>>Sure, that's because I wrote that text before we had a node name.
> >>>>>
> >>>>>However, in 1.7 references didn't work yet, so we still have all freedom
> >>>>>to change the interface as we like.
> >>>>Yes, that's right.
> >>>>
> >>>>>>However, if the ID cannot be found, I think
> >>>>>>we should interpret it as a reference to the node name.
> >>>>>>
> >>>>>>Therefore, I'd first try bdrv_find() and if that returns NULL, try
> >>>>>>again with bdrv_find_node().
> >>>>>I think I would prefer to avoid such ambiguities. Otherwise a management
> >>>>>tool that wants to use the node name needs to check first if it's not
> >>>>>already used as a device name somewhere else and would therefore operate
> >>>>>on the wrong device.
> >>>>>
> >>>>>On the other hand, a management tool using the same names for devices
> >>>>>and nodes just gets what it deserves.
> >>>>>
> >>>>>Perhaps we should use a common namespace for both, i.e. you get an error
> >>>>>if you try to assign a node name that is already a device name and vice
> >>>>>versa?
> >>>>This is what I would go for. However, then I don't really know why
> >>>>we should separate the ID and the node name in the first place
> >>>>(although that's probably because I haven't followed the discussion
> >>>>around node names).
> >>>>
> >>>>Max
> >>>Ping,
> >>>
> >>>I still want to make quorum merge.
> >>>What should be done for the references ?
> >>>
> >>>Best regards
> >>>
> >>>Benoît
> >>My only problem is that I don't really know what IDs are for, then. ;-)
> >>
> > From the understanding I have ID are for block backend top level bds and
> >node-name naming all the bds burried in the graph.
> >
> >So my personal opinion would be to relax the constraint on bdrv_lookup_bs
> >and use it for references.
> >
> >Kevin && Max: what do you think of this scheme ?
>
> I agree. For example, we could change the constraint to report an
> error only if both ID and node name are actually valid (and point to
> different devices), that is, bdrv_find() and bdrv_find_node() return
> different non-NULL values.
Ok I will write patch doing this on top of quorum patches.
Best regards
Benoît
>
> Max
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH V6 8/8] block: Use graph node name as reference in bdrv_file_open().
2014-01-31 21:37 ` Benoît Canet
@ 2014-02-03 9:43 ` Kevin Wolf
2014-02-04 13:02 ` Eric Blake
0 siblings, 1 reply; 29+ messages in thread
From: Kevin Wolf @ 2014-02-03 9:43 UTC (permalink / raw)
To: Benoît Canet; +Cc: famz, armbru, qemu-devel, stefanha, Max Reitz
Am 31.01.2014 um 22:37 hat Benoît Canet geschrieben:
> Le Friday 31 Jan 2014 à 21:32:34 (+0100), Max Reitz a écrit :
> > On 28.01.2014 01:04, Benoît Canet wrote:
> > >Le Monday 27 Jan 2014 à 20:11:59 (+0100), Max Reitz a écrit :
> > >>On 27.01.2014 15:36, Benoît Canet wrote:
> > >>>Le Friday 24 Jan 2014 à 15:54:39 (+0100), Max Reitz a écrit :
> > >>>>On 24.01.2014 15:48, Kevin Wolf wrote:
> > >>>>>Am 24.01.2014 um 14:37 hat Max Reitz geschrieben:
> > >>>>>>On 24.01.2014 14:26, Kevin Wolf wrote:
> > >>>>>>>Am 23.01.2014 um 21:31 hat Benoît Canet geschrieben:
> > >>>>>>>>Signed-off-by: Benoit Canet <benoit@irqsave.net>
> > >>>>>>>>---
> > >>>>>>>> block.c | 6 +++---
> > >>>>>>>> 1 file changed, 3 insertions(+), 3 deletions(-)
> > >>>>>>>I'm not going to merge this one yet. It breaks qemu-iotests case 071,
> > >>>>>>>which would have to be adapted.
> > >>>>>>>
> > >>>>>>>However, first of all I'd like to hear the opinions of at least Eric and
> > >>>>>>>Max on what BlockRef should really refer to. I think node names make
> > >>>>>>>most sense, but perhaps it's a bit inconvenient and the command line
> > >>>>>>>should default to node-name = id when id is set, but node-name isn't?
> > >>>>>>The QAPI schema is pretty clear about this: “references the ID of an
> > >>>>>>existing block device.”
> > >>>>>Sure, that's because I wrote that text before we had a node name.
> > >>>>>
> > >>>>>However, in 1.7 references didn't work yet, so we still have all freedom
> > >>>>>to change the interface as we like.
> > >>>>Yes, that's right.
> > >>>>
> > >>>>>>However, if the ID cannot be found, I think
> > >>>>>>we should interpret it as a reference to the node name.
> > >>>>>>
> > >>>>>>Therefore, I'd first try bdrv_find() and if that returns NULL, try
> > >>>>>>again with bdrv_find_node().
> > >>>>>I think I would prefer to avoid such ambiguities. Otherwise a management
> > >>>>>tool that wants to use the node name needs to check first if it's not
> > >>>>>already used as a device name somewhere else and would therefore operate
> > >>>>>on the wrong device.
> > >>>>>
> > >>>>>On the other hand, a management tool using the same names for devices
> > >>>>>and nodes just gets what it deserves.
> > >>>>>
> > >>>>>Perhaps we should use a common namespace for both, i.e. you get an error
> > >>>>>if you try to assign a node name that is already a device name and vice
> > >>>>>versa?
> > >>>>This is what I would go for. However, then I don't really know why
> > >>>>we should separate the ID and the node name in the first place
> > >>>>(although that's probably because I haven't followed the discussion
> > >>>>around node names).
> > >>>>
> > >>>>Max
> > >>>Ping,
> > >>>
> > >>>I still want to make quorum merge.
> > >>>What should be done for the references ?
> > >>>
> > >>>Best regards
> > >>>
> > >>>Benoît
> > >>My only problem is that I don't really know what IDs are for, then. ;-)
> > >>
> > > From the understanding I have ID are for block backend top level bds and
> > >node-name naming all the bds burried in the graph.
> > >
> > >So my personal opinion would be to relax the constraint on bdrv_lookup_bs
> > >and use it for references.
> > >
> > >Kevin && Max: what do you think of this scheme ?
> >
> > I agree. For example, we could change the constraint to report an
> > error only if both ID and node name are actually valid (and point to
> > different devices), that is, bdrv_find() and bdrv_find_node() return
> > different non-NULL values.
>
> Ok I will write patch doing this on top of quorum patches.
Yes, I think allowing bdrv_lookup_bs() to find both node names and
device names makes sense.
I would still use a common namespace and forbid using the same name for
a device and a node.
Kevin
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH V6 8/8] block: Use graph node name as reference in bdrv_file_open().
2014-02-03 9:43 ` Kevin Wolf
@ 2014-02-04 13:02 ` Eric Blake
0 siblings, 0 replies; 29+ messages in thread
From: Eric Blake @ 2014-02-04 13:02 UTC (permalink / raw)
To: Kevin Wolf, Benoît Canet
Cc: armbru, famz, qemu-devel, stefanha, Max Reitz
[-- Attachment #1: Type: text/plain, Size: 610 bytes --]
On 02/03/2014 02:43 AM, Kevin Wolf wrote:
>
> Yes, I think allowing bdrv_lookup_bs() to find both node names and
> device names makes sense.
>
> I would still use a common namespace and forbid using the same name for
> a device and a node.
That makes sense for libvirt as well (libvirt has every intention of
using a different prefix when referring to a device than when referring
to a node, so a shared namespace won't hurt libvirt, and makes the most
sense for other operations).
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH V6 0/8] Giving names to graph's BlockDriverState
2014-01-23 20:31 [Qemu-devel] [PATCH V6 0/8] Giving names to graph's BlockDriverState Benoît Canet
` (7 preceding siblings ...)
2014-01-23 20:31 ` [Qemu-devel] [PATCH V6 8/8] block: Use graph node name as reference in bdrv_file_open() Benoît Canet
@ 2014-01-24 13:27 ` Kevin Wolf
8 siblings, 0 replies; 29+ messages in thread
From: Kevin Wolf @ 2014-01-24 13:27 UTC (permalink / raw)
To: Benoît Canet; +Cc: armbru, famz, qemu-devel, stefanha, mreitz
Am 23.01.2014 um 21:31 hat Benoît Canet geschrieben:
> The following series introduce a new file.node-name property in order to be
> able to give a name to each BlockDriverState of the graph.
>
> since v5:
> rename to bdrv_assign_node_name [Fam]
> add node-name to BlockdevOptionsBase [Kevin]
> add query-named-block-nodes name in commit message [Fam]
> make bdrv_lookup_bs interface more generic [Kevin]
> s/check/checks/ [Fam]
> s/implentation/implementation/ [Fam]
> use node names for references [Kevin]
>
>
>
> Benoît Canet (8):
> 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 both on command
> line and QMP.
> qmp: Add QMP query-named-block-nodes 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: Use graph node name as reference in bdrv_file_open().
>
> block.c | 214 ++++++++++++++++++++++++++++++++++++++++------
> 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 | 50 +++++++++--
> qmp-commands.hx | 78 ++++++++++++++++-
> 10 files changed, 474 insertions(+), 125 deletions(-)
Thanks, applied patches 1-7 to the block branch.
Kevin
^ permalink raw reply [flat|nested] 29+ messages in thread