[Qemu-devel] [PATCH V6 0/8] Giving names to graph's BlockDriverState

[Qemu-devel] [PATCH V6 0/8] Giving names to graph's BlockDriverState

[Benoît Canet]

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(-)

-- 
1.8.3.2

[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]

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

[Qemu-devel] [PATCH V6 2/8] block: Allow the user to define "node-name" option both on command line and QMP.

[Benoît Canet]

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

[Qemu-devel] [PATCH V6 3/8] qmp: Add QMP query-named-block-nodes to list the named BlockDriverState nodes.

[Benoît Canet]

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

[Qemu-devel] [PATCH V6 4/8] qmp: Allow to change password on named block driver states.

[Benoît Canet]

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

[Qemu-devel] [PATCH V6 5/8] block: Create authorizations mechanism for external snapshot and resize.

[Benoît Canet]

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

[Qemu-devel] [PATCH V6 6/8] qmp: Allow block_resize to manipulate bs graph nodes.

[Benoît Canet]

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

[Qemu-devel] [PATCH V6 7/8] qmp: Allow to take external snapshots on bs graphs node.

[Benoît Canet]

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

[Qemu-devel] [PATCH V6 8/8] block: Use graph node name as reference in bdrv_file_open().

[Benoît Canet]

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

Re: [Qemu-devel] [PATCH V6 8/8] block: Use graph node name as reference in bdrv_file_open().

[Kevin Wolf]

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
> 

Re: [Qemu-devel] [PATCH V6 0/8] Giving names to graph's BlockDriverState

[Kevin Wolf]

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

Re: [Qemu-devel] [PATCH V6 8/8] block: Use graph node name as reference in bdrv_file_open().

[Max Reitz]

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

Re: [Qemu-devel] [PATCH V6 8/8] block: Use graph node name as reference in bdrv_file_open().

[Kevin Wolf]

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

Re: [Qemu-devel] [PATCH V6 2/8] block: Allow the user to define "node-name" option both on command line and QMP.

[Benoît Canet]

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
> 

Re: [Qemu-devel] [PATCH V6 8/8] block: Use graph node name as reference in bdrv_file_open().

[Max Reitz]

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

Re: [Qemu-devel] [PATCH V6 2/8] block: Allow the user to define "node-name" option both on command line and QMP.

[Kevin Wolf]

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
> > 

Re: [Qemu-devel] [PATCH V6 8/8] block: Use graph node name as reference in bdrv_file_open().

[Benoît Canet]

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

Re: [Qemu-devel] [PATCH V6 8/8] block: Use graph node name as reference in bdrv_file_open().

[Max Reitz]

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

Re: [Qemu-devel] [PATCH V6 8/8] block: Use graph node name as reference in bdrv_file_open().

[Benoît Canet]

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

Re: [Qemu-devel] [PATCH V6 8/8] block: Use graph node name as reference in bdrv_file_open().

[Max Reitz]

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

Re: [Qemu-devel] [PATCH V6 8/8] block: Use graph node name as reference in bdrv_file_open().

[Benoît Canet]

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

Re: [Qemu-devel] [PATCH V6 8/8] block: Use graph node name as reference in bdrv_file_open().

[Kevin Wolf]

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

Re: [Qemu-devel] [PATCH V6 5/8] block: Create authorizations mechanism for external snapshot and resize.

[Jeff Cody]

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
> 
> 

Re: [Qemu-devel] [PATCH V6 5/8] block: Create authorizations mechanism for external snapshot and resize.

[Kevin Wolf]

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) {

Re: [Qemu-devel] [PATCH V6 8/8] block: Use graph node name as reference in bdrv_file_open().

[Eric Blake]

[-- 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 --]

Re: [Qemu-devel] [PATCH V6 5/8] block: Create authorizations mechanism for external snapshot and resize.

[Jeff Cody]

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) {
> 

Re: [Qemu-devel] [PATCH V6 5/8] block: Create authorizations mechanism for external snapshot and resize.

[Benoît Canet]

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) {
> 
> 

Re: [Qemu-devel] [PATCH V6 5/8] block: Create authorizations mechanism for external snapshot and resize.

[Benoît Canet]

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) {
> 
> 

Re: [Qemu-devel] [PATCH V6 5/8] block: Create authorizations mechanism for external snapshot and resize.

[Benoît Canet]

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) {
>