[Qemu-devel] [PATCH V4 0/7] Giving names to BlockDriverState graph nodes

[Qemu-devel] [PATCH V4 0/7] Giving names to BlockDriverState graph nodes

[Benoît Canet]

This partial series start to add some node-name manipulation from QMP.
In particular it will allow to take snapshots of quorum files.
I propose reviewing it and merging if it's ok so quorum can be enabled and
merged and I could start enable other block filter feature while writing next
features. (Quorum file repair, crypto, multi bs block throttling)

v4:
    s/prepare/prepare for/ [Eric]
    s/followings/following/ [Eric]
    fix option memory leak [Eric]
    new command to get named bs name list [Eric]
    * struct -> *struct [Eric/Fam]
    Shorter comparison [Eric]
    1.8 -> 2.0 [Eric/Fam]
    More commments to explain authorization method [Fam]
    Add a don't care result to snapshot authorization method [Fam]
    Add #optional [Eric]


Best regards

Benoît

Benoît Canet (7):
  block: Add bs->node_name to hold the name of a bs node of the bs
    graph.
  block: Allow the user to define "node-name" option.
  qmp: Add a command to list the named BlockDriverState nodes.
  qmp: Allow to change password on names block driver states.
  qmp: Allow block_resize to manipulate bs graph nodes.
  block: Create authorizations mechanism for external snapshots.
  qmp: Allow to take external snapshots on bs graphs node.

 block.c                   | 222 ++++++++++++++++++++++++++++++++++++++++------
 block/blkverify.c         |   4 +-
 block/iscsi.c             |   2 +-
 block/vmdk.c              |   2 +-
 block/vvfat.c             |   4 +-
 blockdev.c                |  84 ++++++++++++++----
 hmp.c                     |   8 +-
 hw/block/xen_disk.c       |   2 +-
 include/block/block.h     |  25 ++++--
 include/block/block_int.h |  21 ++++-
 qapi-schema.json          |  43 +++++++--
 qemu-img.c                |   6 +-
 qemu-io.c                 |   2 +-
 qemu-nbd.c                |   2 +-
 qmp-commands.hx           |  36 +++++++-
 15 files changed, 384 insertions(+), 79 deletions(-)

-- 
1.8.3.2

[Qemu-devel] [PATCH V4 1/7] block: Add bs->node_name to hold the name of a bs node of the bs graph.

[Benoît Canet]

Add the minimum of code to prepare for the following patches.

Signed-off-by: Benoit Canet <benoit@irqsave.net>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block.c                   | 72 ++++++++++++++++++++++++++++++++++-------------
 block/blkverify.c         |  2 +-
 block/iscsi.c             |  2 +-
 block/vmdk.c              |  2 +-
 block/vvfat.c             |  4 +--
 blockdev.c                |  8 +++---
 hw/block/xen_disk.c       |  2 +-
 include/block/block.h     |  3 +-
 include/block/block_int.h |  9 +++++-
 qemu-img.c                |  6 ++--
 qemu-io.c                 |  2 +-
 qemu-nbd.c                |  2 +-
 12 files changed, 78 insertions(+), 36 deletions(-)

diff --git a/block.c b/block.c
index 3d78581..4f6b36a 100644
--- a/block.c
+++ b/block.c
@@ -89,6 +89,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);
 
@@ -318,15 +321,21 @@ void bdrv_register(BlockDriver *bdrv)
 }
 
 /* create a new block device (by default it is empty) */
-BlockDriverState *bdrv_new(const char *device_name)
+BlockDriverState *bdrv_new(const char *device_name, const char *node_name)
 {
     BlockDriverState *bs;
 
+    assert(node_name);
+
     bs = g_malloc0(sizeof(BlockDriverState));
     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);
+    }
+    pstrcpy(bs->node_name, sizeof(bs->node_name), node_name);
+    if (node_name[0] != '\0') {
+        QTAILQ_INSERT_TAIL(&graph_bdrv_states, bs, node_list);
     }
     bdrv_iostatus_disable(bs);
     notifier_list_init(&bs->close_notifiers);
@@ -871,7 +880,7 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename,
         options = qdict_new();
     }
 
-    bs = bdrv_new("");
+    bs = bdrv_new("", "");
     bs->options = options;
     options = qdict_clone_shallow(options);
 
@@ -993,7 +1002,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
                                        sizeof(backing_filename));
     }
 
-    bs->backing_hd = bdrv_new("");
+    bs->backing_hd = bdrv_new("", "");
 
     if (bs->backing_format[0] != '\0') {
         back_drv = bdrv_find_format(bs->backing_format);
@@ -1059,7 +1068,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
            instead of opening 'filename' directly */
 
         /* Get the required size from the image */
-        bs1 = bdrv_new("");
+        bs1 = bdrv_new("", "");
         QINCREF(options);
         ret = bdrv_open(bs1, filename, options, BDRV_O_NO_BACKING,
                         drv, &local_err);
@@ -1500,7 +1509,7 @@ void bdrv_close_all(void)
 {
     BlockDriverState *bs;
 
-    QTAILQ_FOREACH(bs, &bdrv_states, list) {
+    QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
         bdrv_close(bs);
     }
 }
@@ -1529,7 +1538,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;
         }
@@ -1559,7 +1568,7 @@ void bdrv_drain_all(void)
         /* FIXME: We do not have timer support here, so this is effectively
          * a busy wait.
          */
-        QTAILQ_FOREACH(bs, &bdrv_states, list) {
+        QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
             if (bdrv_start_throttled_reqs(bs)) {
                 busy = true;
             }
@@ -1570,14 +1579,18 @@ 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);
+    }
 }
 
 static void bdrv_rebind(BlockDriverState *bs)
@@ -1631,7 +1644,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 */
+    pstrcpy(bs_dest->node_name, sizeof(bs_dest->node_name),
+            bs_src->node_name);
+    bs_dest->node_list   = bs_src->node_list;
 }
 
 /*
@@ -1956,7 +1974,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) {
@@ -3098,11 +3116,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;
         }
@@ -3110,19 +3129,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);
     }
 }
@@ -3142,7 +3176,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;
@@ -4251,7 +4285,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);
     }
 }
@@ -4260,7 +4294,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);
     }
 }
@@ -4771,7 +4805,7 @@ void bdrv_img_create(const char *filename, const char *fmt,
             back_flags =
                 flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);
 
-            bs = bdrv_new("");
+            bs = bdrv_new("", "");
 
             ret = bdrv_open(bs, backing_file->value.s, NULL, back_flags,
                             backing_drv, &local_err);
diff --git a/block/blkverify.c b/block/blkverify.c
index 3c63528..a7c9da2 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -155,7 +155,7 @@ static int blkverify_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail;
     }
 
-    s->test_file = bdrv_new("");
+    s->test_file = bdrv_new("", "");
     ret = bdrv_open(s->test_file, filename, NULL, flags, NULL, &local_err);
     if (ret < 0) {
         error_propagate(errp, local_err);
diff --git a/block/iscsi.c b/block/iscsi.c
index b7b5238..1304a3c 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1519,7 +1519,7 @@ static int iscsi_create(const char *filename, QEMUOptionParameter *options,
     IscsiLun *iscsilun = NULL;
     QDict *bs_options;
 
-    bs = bdrv_new("");
+    bs = bdrv_new("", "");
 
     /* Read out options */
     while (options && options->name) {
diff --git a/block/vmdk.c b/block/vmdk.c
index 88d09e3..d873de1 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1696,7 +1696,7 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options,
         return -ENOTSUP;
     }
     if (backing_file) {
-        BlockDriverState *bs = bdrv_new("");
+        BlockDriverState *bs = bdrv_new("", "");
         ret = bdrv_open(bs, backing_file, NULL, BDRV_O_NO_BACKING, NULL, errp);
         if (ret != 0) {
             bdrv_unref(bs);
diff --git a/block/vvfat.c b/block/vvfat.c
index 3ddaa0b..f973c08 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -2935,7 +2935,7 @@ static int enable_write_target(BDRVVVFATState *s)
         goto err;
     }
 
-    s->qcow = bdrv_new("");
+    s->qcow = bdrv_new("", "");
 
     ret = bdrv_open(s->qcow, s->qcow_filename, NULL,
             BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH, bdrv_qcow,
@@ -2951,7 +2951,7 @@ static int enable_write_target(BDRVVVFATState *s)
     unlink(s->qcow_filename);
 #endif
 
-    s->bs->backing_hd = bdrv_new("");
+    s->bs->backing_hd = bdrv_new("", "");
     s->bs->backing_hd->drv = &vvfat_write_target;
     s->bs->backing_hd->opaque = g_malloc(sizeof(void*));
     *(void**)s->bs->backing_hd->opaque = s;
diff --git a/blockdev.c b/blockdev.c
index 44755e1..a474bb5 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -468,7 +468,7 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
     /* init */
     dinfo = g_malloc0(sizeof(*dinfo));
     dinfo->id = g_strdup(qemu_opts_id(opts));
-    dinfo->bdrv = bdrv_new(dinfo->id);
+    dinfo->bdrv = bdrv_new(dinfo->id, "");
     dinfo->bdrv->open_flags = snapshot ? BDRV_O_SNAPSHOT : 0;
     dinfo->bdrv->read_only = ro;
     dinfo->type = type;
@@ -1256,7 +1256,7 @@ static void external_snapshot_prepare(BlkTransactionState *common,
     }
 
     /* We will manually add the backing_hd field to the bs later */
-    state->new_bs = bdrv_new("");
+    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,
@@ -1923,7 +1923,7 @@ void qmp_drive_backup(const char *device, const char *target,
         return;
     }
 
-    target_bs = bdrv_new("");
+    target_bs = bdrv_new("", "");
     ret = bdrv_open(target_bs, target, NULL, flags, drv, &local_err);
     if (ret < 0) {
         bdrv_unref(target_bs);
@@ -2062,7 +2062,7 @@ void qmp_drive_mirror(const char *device, const char *target,
     /* Mirroring takes care of copy-on-write using the source's backing
      * file.
      */
-    target_bs = bdrv_new("");
+    target_bs = bdrv_new("", "");
     ret = bdrv_open(target_bs, target, NULL, flags | BDRV_O_NO_BACKING, drv,
                     &local_err);
     if (ret < 0) {
diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
index 098f6c6..e8a45d1 100644
--- a/hw/block/xen_disk.c
+++ b/hw/block/xen_disk.c
@@ -808,7 +808,7 @@ static int blk_connect(struct XenDevice *xendev)
     if (!blkdev->dinfo) {
         /* setup via xenbus -> create new block driver instance */
         xen_be_printf(&blkdev->xendev, 2, "create new bdrv (xenbus setup)\n");
-        blkdev->bs = bdrv_new(blkdev->dev);
+        blkdev->bs = bdrv_new(blkdev->dev, "");
         if (blkdev->bs) {
             Error *local_err = NULL;
             BlockDriver *drv = bdrv_find_whitelisted_format(blkdev->fileproto,
diff --git a/include/block/block.h b/include/block/block.h
index 5beccbf..6426ca6 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -177,7 +177,7 @@ int bdrv_create(BlockDriver *drv, const char* filename,
     QEMUOptionParameter *options, Error **errp);
 int bdrv_create_file(const char* filename, QEMUOptionParameter *options,
                      Error **errp);
-BlockDriverState *bdrv_new(const char *device_name);
+BlockDriverState *bdrv_new(const char *device_name, const char *node_name);
 void bdrv_make_anon(BlockDriverState *bs);
 void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old);
 void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top);
@@ -370,6 +370,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 773899b..9e789d2 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -320,11 +320,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;
 
diff --git a/qemu-img.c b/qemu-img.c
index dc0c2f0..c026fbb 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -273,7 +273,7 @@ static BlockDriverState *bdrv_new_open(const char *filename,
     Error *local_err = NULL;
     int ret;
 
-    bs = bdrv_new("image");
+    bs = bdrv_new("image", "");
 
     if (fmt) {
         drv = bdrv_find_format(fmt);
@@ -2237,7 +2237,7 @@ static int img_rebase(int argc, char **argv)
     } else {
         char backing_name[1024];
 
-        bs_old_backing = bdrv_new("old_backing");
+        bs_old_backing = bdrv_new("old_backing", "");
         bdrv_get_backing_filename(bs, backing_name, sizeof(backing_name));
         ret = bdrv_open(bs_old_backing, backing_name, NULL, BDRV_O_FLAGS,
                         old_backing_drv, &local_err);
@@ -2248,7 +2248,7 @@ static int img_rebase(int argc, char **argv)
             goto out;
         }
         if (out_baseimg[0]) {
-            bs_new_backing = bdrv_new("new_backing");
+            bs_new_backing = bdrv_new("new_backing", "");
             ret = bdrv_open(bs_new_backing, out_baseimg, NULL, BDRV_O_FLAGS,
                         new_backing_drv, &local_err);
             if (ret) {
diff --git a/qemu-io.c b/qemu-io.c
index 3b3340a..fb04c74 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -63,7 +63,7 @@ static int openfile(char *name, int flags, int growable, QDict *opts)
             return 1;
         }
     } else {
-        qemuio_bs = bdrv_new("hda");
+        qemuio_bs = bdrv_new("hda", "");
 
         if (bdrv_open(qemuio_bs, name, opts, flags, NULL, &local_err) < 0) {
             fprintf(stderr, "%s: can't open device %s: %s\n", progname, name,
diff --git a/qemu-nbd.c b/qemu-nbd.c
index c26c98e..245fecf 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -572,7 +572,7 @@ int main(int argc, char **argv)
         drv = NULL;
     }
 
-    bs = bdrv_new("hda");
+    bs = bdrv_new("hda", "");
     srcpath = argv[optind];
     ret = bdrv_open(bs, srcpath, NULL, flags, drv, &local_err);
     if (ret < 0) {
-- 
1.8.3.2

[Qemu-devel] [PATCH V4 2/7] block: Allow the user to define "node-name" option.

[Benoît Canet]

Signed-off-by: Benoit Canet <benoit@irqsave.net>
---
 block.c | 44 +++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 39 insertions(+), 5 deletions(-)

diff --git a/block.c b/block.c
index 4f6b36a..61f5ba0 100644
--- a/block.c
+++ b/block.c
@@ -873,6 +873,7 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename,
     const char *drvname;
     bool allow_protocol_prefix = false;
     Error *local_err = NULL;
+    const char *node_name = NULL;
     int ret;
 
     /* NULL means an empty set of options */
@@ -880,7 +881,15 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename,
         options = qdict_new();
     }
 
-    bs = bdrv_new("", "");
+    node_name = qdict_get_try_str(options, "node-name");
+    if (node_name && bdrv_find_node(node_name)) {
+        error_setg(errp, "Duplicate node name");
+        QDECREF(options);
+        return -EINVAL;
+    }
+    bs = bdrv_new("", node_name ? node_name : "");
+    qdict_del(options, "node-name");
+
     bs->options = options;
     options = qdict_clone_shallow(options);
 
@@ -980,6 +989,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
     int back_flags, ret;
     BlockDriver *back_drv = NULL;
     Error *local_err = NULL;
+    const char *node_name = NULL;
 
     if (bs->backing_hd != NULL) {
         QDECREF(options);
@@ -1002,7 +1012,14 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
                                        sizeof(backing_filename));
     }
 
-    bs->backing_hd = bdrv_new("", "");
+    node_name = qdict_get_try_str(options, "node-name");
+    if (node_name && bdrv_find_node(node_name)) {
+        error_setg(errp, "Duplicate node name");
+        QDECREF(options);
+        return -EINVAL;
+    }
+    bs->backing_hd = bdrv_new("", node_name ? node_name : "");
+    qdict_del(options, "node-name");
 
     if (bs->backing_format[0] != '\0') {
         back_drv = bdrv_find_format(bs->backing_format);
@@ -1046,6 +1063,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
     BlockDriverState *file = NULL;
     QDict *file_options = NULL;
     const char *drvname;
+    const char *node_name = NULL;
     Error *local_err = NULL;
 
     /* NULL means an empty set of options */
@@ -1053,6 +1071,22 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
         options = qdict_new();
     }
 
+    node_name = qdict_get_try_str(options, "node-name");
+    if (node_name && bdrv_find_node(node_name)) {
+        error_setg(errp, "Duplicate node name");
+        QDECREF(options);
+        return -EINVAL;
+    }
+
+    if (node_name) {
+        pstrcpy(bs->node_name, sizeof(bs->node_name), node_name);
+        if (node_name[0] != '\0') {
+            QTAILQ_INSERT_TAIL(&graph_bdrv_states, bs, node_list);
+        }
+    }
+
+    qdict_del(options, "node-name");
+
     bs->options = options;
     options = qdict_clone_shallow(options);
 
@@ -1646,9 +1680,9 @@ static void bdrv_move_feature_fields(BlockDriverState *bs_dest,
             bs_src->device_name);
     bs_dest->device_list = bs_src->device_list;
 
-    /* keep the same entry in graph_bdrv_states */
-    pstrcpy(bs_dest->node_name, sizeof(bs_dest->node_name),
-            bs_src->node_name);
+    /* 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;
 }
 
-- 
1.8.3.2

[Qemu-devel] [PATCH V4 3/7] qmp: Add a command to list the named BlockDriverState nodes.

[Benoît Canet]

Signed-off-by: Benoit Canet <benoit@irqsave.net>
---
 block.c               | 16 ++++++++++++++++
 blockdev.c            |  5 +++++
 include/block/block.h |  1 +
 qapi-schema.json      | 11 +++++++++++
 qmp-commands.hx       | 19 +++++++++++++++++++
 5 files changed, 52 insertions(+)

diff --git a/block.c b/block.c
index 61f5ba0..372aa3b 100644
--- a/block.c
+++ b/block.c
@@ -3178,6 +3178,22 @@ BlockDriverState *bdrv_find_node(const char *node_name)
     return NULL;
 }
 
+strList *bdrv_named_nodes_list(void)
+{
+    strList *str_list, *entry;
+    BlockDriverState *bs;
+
+    str_list = NULL;
+    QTAILQ_FOREACH(bs, &graph_bdrv_states, node_list) {
+        entry = g_malloc0(sizeof(*entry));
+        entry->value = g_strdup(bs->node_name);
+        entry->next = str_list;
+        str_list = entry;
+    }
+
+    return str_list;
+}
+
 BlockDriverState *bdrv_next(BlockDriverState *bs)
 {
     if (!bs) {
diff --git a/blockdev.c b/blockdev.c
index a474bb5..1eb4639 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1940,6 +1940,11 @@ void qmp_drive_backup(const char *device, const char *target,
     }
 }
 
+strList *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 6426ca6..fe642d9 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -371,6 +371,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);
+strList *bdrv_named_nodes_list(void);
 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 8630eb5..3a4a4c8 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2008,6 +2008,17 @@
 { 'command': 'drive-backup', 'data': 'DriveBackup' }
 
 ##
+# @query-named-block-nodes
+#
+# Get the named block driver list
+#
+# Returns: the list of named nodes names
+#
+# Since 2.0
+##
+{ 'command': 'query-named-block-nodes', 'returns': [ 'str' ] }
+
+##
 # @drive-mirror
 #
 # Start mirroring a block device's writes to a new destination.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index fba15cd..107795b 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -3295,3 +3295,22 @@ 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 string containing the name of the named block driver states
+
+Example:
+
+-> { "execute": "query-named-block-nodes" }
+<- { "return": [ "quorum1.qcow2", "quorum2.qcow2", "quorum3.qcow2" ] }
+
+EQMP
-- 
1.8.3.2

[Qemu-devel] [PATCH V4 4/7] qmp: Allow to change password on names block driver states.

[Benoît Canet]

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 372aa3b..e53f24f 100644
--- a/block.c
+++ b/block.c
@@ -3194,6 +3194,38 @@ strList *bdrv_named_nodes_list(void)
     return str_list;
 }
 
+BlockDriverState *bdrv_lookup_bs(bool has_device, const char *device,
+                                 bool has_node_name, const char *node_name,
+                                 Error **errp)
+{
+    BlockDriverState *bs = NULL;
+
+    if (has_device == has_node_name) {
+        error_setg(errp, "Use either device or node-name but not both");
+        return NULL;
+    }
+
+    if (has_device) {
+        bs = bdrv_find(device);
+
+        if (!bs) {
+            error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+            return NULL;
+        }
+
+        return bs;
+    }
+
+    bs = bdrv_find_node(node_name);
+
+    if (!bs) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, node_name);
+        return NULL;
+    }
+
+    return bs;
+}
+
 BlockDriverState *bdrv_next(BlockDriverState *bs)
 {
     if (!bs) {
diff --git a/blockdev.c b/blockdev.c
index 1eb4639..5abf303 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1474,14 +1474,19 @@ void qmp_eject(const char *device, bool has_force, bool force, Error **errp)
     eject_device(bs, force, errp);
 }
 
-void qmp_block_passwd(const char *device, const char *password, Error **errp)
+void qmp_block_passwd(bool has_device, const char *device,
+                      bool has_node_name, const char *node_name,
+                      const char *password, Error **errp)
 {
+    Error *local_err = NULL;
     BlockDriverState *bs;
     int err;
 
-    bs = bdrv_find(device);
-    if (!bs) {
-        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+    bs = bdrv_lookup_bs(has_device, device,
+                        has_node_name, node_name,
+                        &local_err);
+    if (error_is_set(&local_err)) {
+        error_propagate(errp, local_err);
         return;
     }
 
diff --git a/hmp.c b/hmp.c
index 32ee285..3820fbe 100644
--- a/hmp.c
+++ b/hmp.c
@@ -870,7 +870,7 @@ void hmp_block_passwd(Monitor *mon, const QDict *qdict)
     const char *password = qdict_get_str(qdict, "password");
     Error *errp = NULL;
 
-    qmp_block_passwd(device, password, &errp);
+    qmp_block_passwd(true, device, false, NULL, password, &errp);
     hmp_handle_error(mon, &errp);
 }
 
diff --git a/include/block/block.h b/include/block/block.h
index fe642d9..49dab83 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -372,6 +372,9 @@ const char *bdrv_get_format_name(BlockDriverState *bs);
 BlockDriverState *bdrv_find(const char *name);
 BlockDriverState *bdrv_find_node(const char *node_name);
 strList *bdrv_named_nodes_list(void);
+BlockDriverState *bdrv_lookup_bs(bool has_device, const char *device,
+                                 bool has_node_name, const char *node_name,
+                                 Error **errp);
 BlockDriverState *bdrv_next(BlockDriverState *bs);
 void bdrv_iterate(void (*it)(void *opaque, BlockDriverState *bs),
                   void *opaque);
diff --git a/qapi-schema.json b/qapi-schema.json
index 3a4a4c8..ddd0dbd 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1675,7 +1675,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
 #
@@ -1689,7 +1693,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 107795b..19a7eb2 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1452,7 +1452,7 @@ EQMP
 
     {
         .name       = "block_passwd",
-        .args_type  = "device:B,password:s",
+        .args_type  = "device:s?,node-name:s?,password:s",
         .mhandler.cmd_new = qmp_marshal_input_block_passwd,
     },
 
@@ -1465,6 +1465,7 @@ Set the password of encrypted block devices.
 Arguments:
 
 - "device": device name (json-string)
+- "node-name": name in the block driver state graph (json-string)
 - "password": password (json-string)
 
 Example:
-- 
1.8.3.2

[Qemu-devel] [PATCH V4 5/7] qmp: Allow block_resize to manipulate bs graph nodes.

[Benoît Canet]

Signed-off-by: Benoit Canet <benoit@irqsave.net>
---
 blockdev.c       | 13 +++++++++----
 hmp.c            |  2 +-
 qapi-schema.json | 10 ++++++++--
 qmp-commands.hx  |  3 ++-
 4 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 5abf303..5bc9ddc 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1676,14 +1676,19 @@ int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
     return 0;
 }
 
-void qmp_block_resize(const char *device, int64_t size, Error **errp)
+void qmp_block_resize(bool has_device, const char *device,
+                      bool has_node_name, const char *node_name,
+                      int64_t size, Error **errp)
 {
+    Error *local_err = NULL;
     BlockDriverState *bs;
     int ret;
 
-    bs = bdrv_find(device);
-    if (!bs) {
-        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+    bs = bdrv_lookup_bs(has_device, device,
+                        has_node_name, node_name,
+                        &local_err);
+    if (error_is_set(&local_err)) {
+        error_propagate(errp, local_err);
         return;
     }
 
diff --git a/hmp.c b/hmp.c
index 3820fbe..906ddb7 100644
--- a/hmp.c
+++ b/hmp.c
@@ -892,7 +892,7 @@ void hmp_block_resize(Monitor *mon, const QDict *qdict)
     int64_t size = qdict_get_int(qdict, "size");
     Error *errp = NULL;
 
-    qmp_block_resize(device, size, &errp);
+    qmp_block_resize(true, device, false, NULL, size, &errp);
     hmp_handle_error(mon, &errp);
 }
 
diff --git a/qapi-schema.json b/qapi-schema.json
index ddd0dbd..9d41d9f 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1721,7 +1721,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
 #
@@ -1730,7 +1734,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 19a7eb2..3cec99f 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -880,7 +880,7 @@ EQMP
 
     {
         .name       = "block_resize",
-        .args_type  = "device:B,size:o",
+        .args_type  = "device:s?,node-name:s?,size:o",
         .mhandler.cmd_new = qmp_marshal_input_block_resize,
     },
 
@@ -893,6 +893,7 @@ Resize a block image while a guest is running.
 Arguments:
 
 - "device": the device's ID, must be unique (json-string)
+- "node-name": the node name in the block driver state graph (json-string)
 - "size": new size
 
 Example:
-- 
1.8.3.2

[Qemu-devel] [PATCH V4 6/7] block: Create authorizations mechanism for external snapshots.

[Benoît Canet]

Signed-off-by: Benoit Canet <benoit@irqsave.net>
---
 block.c                   | 68 +++++++++++++++++++++++++++++++++++++++++------
 block/blkverify.c         |  2 +-
 include/block/block.h     | 18 ++++++++++---
 include/block/block_int.h | 12 ++++++---
 4 files changed, 84 insertions(+), 16 deletions(-)

diff --git a/block.c b/block.c
index e53f24f..cb7ac88 100644
--- a/block.c
+++ b/block.c
@@ -4962,21 +4962,73 @@ int bdrv_amend_options(BlockDriverState *bs, QEMUOptionParameter *options)
     return bs->drv->bdrv_amend_options(bs, options);
 }
 
-ExtSnapshotPerm bdrv_check_ext_snapshot(BlockDriverState *bs)
+/* will be used to recurse on single child block filter until first format
+ * (single child block filter will store their child in bs->file)
+ */
+ExtSnapshotPerm bdrv_generic_check_ext_snapshot(BlockDriverState *bs,
+                                                BlockDriverState *candidate)
 {
-    if (bs->drv->bdrv_check_ext_snapshot) {
-        return bs->drv->bdrv_check_ext_snapshot(bs);
+    if (!bs->drv) {
+        return EXT_SNAPSHOT_DO_NOT_CARE;
+    }
+
+    /* This double negation allow to implement the behavior without modifying
+     * all the BlockDrivers.
+     */
+    if (!bs->drv->authorizations[BS_CANT_SNAPSHOT]) {
+        if (bs == candidate) {
+            return EXT_SNAPSHOT_ALLOWED;
+        } else {
+            return EXT_SNAPSHOT_FORBIDDEN;
+        }
     }
 
-    if (bs->file && bs->file->drv && bs->file->drv->bdrv_check_ext_snapshot) {
-        return bs->file->drv->bdrv_check_ext_snapshot(bs);
+    if (!bs->drv->authorizations[BS_FILTER_PASS_DOWN]) {
+        return EXT_SNAPSHOT_DO_NOT_CARE;
+    }
+
+    if (!bs->file) {
+        return EXT_SNAPSHOT_DO_NOT_CARE;
+    }
+
+    return bdrv_recurse_check_ext_snapshot(bs->file, candidate);
+}
+
+ExtSnapshotPerm bdrv_recurse_check_ext_snapshot(BlockDriverState *bs,
+                                                BlockDriverState *candidate)
+{
+    if (bs->drv && bs->drv->bdrv_recurse_check_ext_snapshot) {
+        return bs->drv->bdrv_recurse_check_ext_snapshot(bs, candidate);
     }
 
-    /* external snapshots are allowed by default */
-    return EXT_SNAPSHOT_ALLOWED;
+    return bdrv_generic_check_ext_snapshot(bs, candidate);
 }
 
-ExtSnapshotPerm bdrv_check_ext_snapshot_forbidden(BlockDriverState *bs)
+/* This function check if the candidate bs has snapshots authorized by going
+ * down the forest of bs, skipping filters and stopping on the the first bses
+ * authorizing snapshots. If one of these bses is the the candidate and has
+ * !authorizations[BS_CANT_SNAPSHOT] the snapshot is authorized.
+ */
+ExtSnapshotPerm bdrv_check_ext_snapshot(BlockDriverState *candidate)
 {
+    BlockDriverState *bs;
+
+    /* walk down the bs forest recursively */
+    QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
+        ExtSnapshotPerm perm;
+
+        if (!bs->file) {
+            continue;
+        }
+
+        perm = bdrv_recurse_check_ext_snapshot(bs->file, candidate);
+
+        /* allowed in the right subtree -> stop here */
+        if (perm == EXT_SNAPSHOT_ALLOWED) {
+            return EXT_SNAPSHOT_ALLOWED;
+        }
+    }
+
+    /* external snapshots are forbidden by default */
     return EXT_SNAPSHOT_FORBIDDEN;
 }
diff --git a/block/blkverify.c b/block/blkverify.c
index a7c9da2..105d4ac 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -417,7 +417,7 @@ static BlockDriver bdrv_blkverify = {
     .bdrv_aio_writev        = blkverify_aio_writev,
     .bdrv_aio_flush         = blkverify_aio_flush,
 
-    .bdrv_check_ext_snapshot = bdrv_check_ext_snapshot_forbidden,
+    .authorizations         = { true, false },
 };
 
 static void bdrv_blkverify_init(void)
diff --git a/include/block/block.h b/include/block/block.h
index 49dab83..f98f902 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -280,16 +280,26 @@ int bdrv_amend_options(BlockDriverState *bs_new, QEMUOptionParameter *options);
 /* external snapshots */
 
 typedef enum {
-    EXT_SNAPSHOT_ALLOWED,
     EXT_SNAPSHOT_FORBIDDEN,
+    EXT_SNAPSHOT_DO_NOT_CARE,
+    EXT_SNAPSHOT_ALLOWED,
 } ExtSnapshotPerm;
 
+typedef enum {
+    BS_CANT_SNAPSHOT,
+    BS_FILTER_PASS_DOWN,
+    BS_AUTHORIZATION_COUNT,
+} BsAuthorization;
+
 /* return EXT_SNAPSHOT_ALLOWED if external snapshot is allowed
  * return EXT_SNAPSHOT_FORBIDDEN if external snapshot is forbidden
+ * return EXT_SNAPSHOT_FORBIDDEN if the candidate bs is not in this bs tree
  */
-ExtSnapshotPerm bdrv_check_ext_snapshot(BlockDriverState *bs);
-/* helper used to forbid external snapshots like in blkverify */
-ExtSnapshotPerm bdrv_check_ext_snapshot_forbidden(BlockDriverState *bs);
+ExtSnapshotPerm bdrv_generic_check_ext_snapshot(BlockDriverState *bs,
+                                                BlockDriverState *candidate);
+ExtSnapshotPerm bdrv_recurse_check_ext_snapshot(BlockDriverState *bs,
+                                                BlockDriverState *candidate);
+ExtSnapshotPerm bdrv_check_ext_snapshot(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 9e789d2..edd85a9 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_CANT_SNAPSHOT.
+     * It's purpose is to recurse on the filter children while calling
+     * bdrv_recurse_check_ext_snapshot on them.
+     * For a sample implentation look in the future Quorum block filter.
      */
-    ExtSnapshotPerm (*bdrv_check_ext_snapshot)(BlockDriverState *bs);
+    ExtSnapshotPerm (*bdrv_recurse_check_ext_snapshot)(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 V4 7/7] qmp: Allow to take external snapshots on bs graphs node.

[Benoît Canet]

Signed-off-by: Benoit Canet <benoit@irqsave.net>
---
 blockdev.c       | 47 ++++++++++++++++++++++++++++++++++++++++-------
 hmp.c            |  4 +++-
 qapi-schema.json | 13 ++++++++++---
 qmp-commands.hx  | 11 ++++++++++-
 4 files changed, 63 insertions(+), 12 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 5bc9ddc..975394c 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -940,14 +940,22 @@ static void blockdev_do_action(int kind, void *data, Error **errp)
     qmp_transaction(&list, errp);
 }
 
-void qmp_blockdev_snapshot_sync(const char *device, const char *snapshot_file,
+void qmp_blockdev_snapshot_sync(bool has_device, const char *device,
+                                bool has_node_name, const char *node_name,
+                                const char *snapshot_file,
+                                bool has_snapshot_node_name,
+                                const char *snapshot_node_name,
                                 bool has_format, const char *format,
-                                bool has_mode, enum NewImageMode mode,
-                                Error **errp)
+                                bool has_mode, NewImageMode mode, Error **errp)
 {
     BlockdevSnapshot snapshot = {
+        .has_device = has_device,
         .device = (char *) device,
+        .has_node_name = has_node_name,
+        .node_name = (char *) node_name,
         .snapshot_file = (char *) snapshot_file,
+        .has_snapshot_node_name = has_snapshot_node_name,
+        .snapshot_node_name = (char *) snapshot_node_name,
         .has_format = has_format,
         .format = (char *) format,
         .has_mode = has_mode,
@@ -1186,7 +1194,12 @@ static void external_snapshot_prepare(BlkTransactionState *common,
     BlockDriver *drv;
     int flags, ret;
     Error *local_err = NULL;
+    bool has_device = false;
     const char *device;
+    bool has_node_name = false;
+    const char *node_name;
+    bool has_snapshot_node_name = false;
+    const char *snapshot_node_name;
     const char *new_image_file;
     const char *format = "qcow2";
     enum NewImageMode mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
@@ -1197,7 +1210,14 @@ static void external_snapshot_prepare(BlkTransactionState *common,
     /* get parameters */
     g_assert(action->kind == TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC);
 
+    has_device = action->blockdev_snapshot_sync->has_device;
     device = action->blockdev_snapshot_sync->device;
+    has_node_name = action->blockdev_snapshot_sync->has_node_name;
+    node_name = action->blockdev_snapshot_sync->node_name;
+    has_snapshot_node_name =
+        action->blockdev_snapshot_sync->has_snapshot_node_name;
+    snapshot_node_name = action->blockdev_snapshot_sync->snapshot_node_name;
+
     new_image_file = action->blockdev_snapshot_sync->snapshot_file;
     if (action->blockdev_snapshot_sync->has_format) {
         format = action->blockdev_snapshot_sync->format;
@@ -1213,9 +1233,21 @@ static void external_snapshot_prepare(BlkTransactionState *common,
         return;
     }
 
-    state->old_bs = bdrv_find(device);
-    if (!state->old_bs) {
-        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+    state->old_bs = bdrv_lookup_bs(has_device, device,
+                                   has_node_name, node_name,
+                                   &local_err);
+    if (error_is_set(&local_err)) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    if (has_node_name && !has_snapshot_node_name) {
+        error_setg(errp, "New snapshot node name missing");
+        return;
+    }
+
+    if (has_snapshot_node_name && bdrv_find_node(snapshot_node_name)) {
+        error_setg(errp, "New snapshot node name already existing");
         return;
     }
 
@@ -1256,7 +1288,8 @@ static void external_snapshot_prepare(BlkTransactionState *common,
     }
 
     /* We will manually add the backing_hd field to the bs later */
-    state->new_bs = bdrv_new("", "");
+    state->new_bs = bdrv_new("",
+                             has_snapshot_node_name ? snapshot_node_name : "");
     /* TODO Inherit bs->options or only take explicit options with an
      * extended QMP command? */
     ret = bdrv_open(state->new_bs, new_image_file, NULL,
diff --git a/hmp.c b/hmp.c
index 906ddb7..47dcf0c 100644
--- a/hmp.c
+++ b/hmp.c
@@ -971,7 +971,9 @@ void hmp_snapshot_blkdev(Monitor *mon, const QDict *qdict)
     }
 
     mode = reuse ? NEW_IMAGE_MODE_EXISTING : NEW_IMAGE_MODE_ABSOLUTE_PATHS;
-    qmp_blockdev_snapshot_sync(device, filename, !!format, format,
+    qmp_blockdev_snapshot_sync(true, device, false, NULL,
+                               filename, false, NULL,
+                               !!format, format,
                                true, mode, &errp);
     hmp_handle_error(mon, &errp);
 }
diff --git a/qapi-schema.json b/qapi-schema.json
index 9d41d9f..25ea53d 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1758,18 +1758,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 3cec99f..7b1e5e6 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1038,7 +1038,9 @@ actions array:
     - "data": a dictionary.  The contents depend on the value
       of "type".  When "type" is "blockdev-snapshot-sync":
       - "device": device name to snapshot (json-string)
+      - "node-name": graph node name to snapshot (json-string)
       - "snapshot-file": name of new image file (json-string)
+      - "snapshot-node-name": graph node name of the new snapshot (json-string)
       - "format": format of new image (json-string, optional)
       - "mode": whether and how QEMU should create the snapshot file
         (NewImageMode, optional, default "absolute-paths")
@@ -1053,6 +1055,11 @@ Example:
          { 'type': 'blockdev-snapshot-sync', 'data' : { "device": "ide-hd0",
                                          "snapshot-file": "/some/place/my-image",
                                          "format": "qcow2" } },
+         { 'type': 'blockdev-snapshot-sync', 'data' : { "node-name": "myfile",
+                                         "snapshot-file": "/some/place/my-image2",
+                                         "snapshot-node-name": "node3432",
+                                         "mode": "existing",
+                                         "format": "qcow2" } },
          { 'type': 'blockdev-snapshot-sync', 'data' : { "device": "ide-hd1",
                                          "snapshot-file": "/some/place/my-image2",
                                          "mode": "existing",
@@ -1066,7 +1073,7 @@ EQMP
 
     {
         .name       = "blockdev-snapshot-sync",
-        .args_type  = "device:B,snapshot-file:s,format:s?,mode:s?",
+        .args_type  = "device:s?,node-name:s?,snapshot-file:s,snapshot-node-name:s?,format:s?,mode:s?",
         .mhandler.cmd_new = qmp_marshal_input_blockdev_snapshot_sync,
     },
 
@@ -1083,7 +1090,9 @@ snapshot image, default is qcow2.
 Arguments:
 
 - "device": device name to snapshot (json-string)
+- "node-name": graph node name to snapshot (json-string)
 - "snapshot-file": name of new image file (json-string)
+- "snapshot-node-name": graph node name of the new snapshot (json-string)
 - "mode": whether and how QEMU should create the snapshot file
   (NewImageMode, optional, default "absolute-paths")
 - "format": format of new image (json-string, optional)
-- 
1.8.3.2

Re: [Qemu-devel] [PATCH V4 4/7] qmp: Allow to change password on names block driver states.

[Luiz Capitulino]

On Thu,  5 Dec 2013 18:15:00 +0100
Benoît Canet <benoit@irqsave.net> wrote:

> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1675,7 +1675,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
>  #
> @@ -1689,7 +1693,8 @@
>  #
>  # Since: 0.14.0
>  ##
> -{ 'command': 'block_passwd', 'data': {'device': 'str', 'password': 'str'} }
> +{ 'command': 'block_passwd', 'data': {'*device': 'str',
> +                                      '*node-name': 'str', 'password': 'str'} }

What about:

{ 'command': 'block_passwd', 'data': {'device': 'str',
                                      '*device-is-node': 'bool', 'password': 'str'} }

Re: [Qemu-devel] [PATCH V4 4/7] qmp: Allow to change password on names block driver states.

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1132 bytes --]

On 12/06/2013 07:27 AM, Luiz Capitulino wrote:
> On Thu,  5 Dec 2013 18:15:00 +0100
> Benoît Canet <benoit@irqsave.net> wrote:

>> -{ 'command': 'block_passwd', 'data': {'device': 'str', 'password': 'str'} }
>> +{ 'command': 'block_passwd', 'data': {'*device': 'str',
>> +                                      '*node-name': 'str', 'password': 'str'} }
> 
> What about:
> 
> { 'command': 'block_passwd', 'data': {'device': 'str',
>                                       '*device-is-node': 'bool', 'password': 'str'} }

That would also work; the naming is a bit more awkward, but then you
don't have the issue of mutually-exclusive optional arguments where
exactly one of the two arguments is required.

I'm leaning slightly towards the approach that Benoît took, if only for
the naming aspect (that is, I also thought of the idea of a bool flag,
but didn't suggest it because I didn't like the implications on the
naming).  But I can live with either approach, if anyone else has a
strong opinion.

-- 
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: 621 bytes --]

Re: [Qemu-devel] [PATCH V4 2/7] block: Allow the user to define "node-name" option.

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 382 bytes --]

On 12/05/2013 10:14 AM, Benoît Canet wrote:
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
>  block.c | 44 +++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 39 insertions(+), 5 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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: 621 bytes --]

Re: [Qemu-devel] [PATCH V4 3/7] qmp: Add a command to list the named BlockDriverState nodes.

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1310 bytes --]

On 12/05/2013 10:14 AM, Benoît Canet wrote:
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
>  block.c               | 16 ++++++++++++++++
>  blockdev.c            |  5 +++++
>  include/block/block.h |  1 +
>  qapi-schema.json      | 11 +++++++++++
>  qmp-commands.hx       | 19 +++++++++++++++++++
>  5 files changed, 52 insertions(+)

>  ##
> +# @query-named-block-nodes
> +#
> +# Get the named block driver list
> +#
> +# Returns: the list of named nodes names

Sounds funny.  Maybe better with just:

Returns: the list of names

> +#
> +# Since 2.0
> +##
> +{ 'command': 'query-named-block-nodes', 'returns': [ 'str' ] }

Also, is list of names sufficient, or are we better off returning a list
of structs with the details already included?  With list of names, the
management app has to follow up with another QMP command per name if it
wants to know details about each node, instead of getting it all in one
command.

> +SQMP
> +@query-named-block-nodes
> +------------------------
> +
> +Return a list of string containing the name of the named block driver states

Return a list of strings containing the name of each named block driver node

-- 
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: 621 bytes --]

Re: [Qemu-devel] [PATCH V4 4/7] qmp: Allow to change password on names block driver states.

[Luiz Capitulino]

On Fri, 06 Dec 2013 08:24:33 -0700
Eric Blake <eblake@redhat.com> wrote:

> On 12/06/2013 07:27 AM, Luiz Capitulino wrote:
> > On Thu,  5 Dec 2013 18:15:00 +0100
> > Benoît Canet <benoit@irqsave.net> wrote:
> 
> >> -{ 'command': 'block_passwd', 'data': {'device': 'str', 'password': 'str'} }
> >> +{ 'command': 'block_passwd', 'data': {'*device': 'str',
> >> +                                      '*node-name': 'str', 'password': 'str'} }
> > 
> > What about:
> > 
> > { 'command': 'block_passwd', 'data': {'device': 'str',
> >                                       '*device-is-node': 'bool', 'password': 'str'} }
> 
> That would also work; the naming is a bit more awkward, but then you
> don't have the issue of mutually-exclusive optional arguments where
> exactly one of the two arguments is required.

Yes, and I dislike that very much.

Btw, can anyone remind me why we can't have new commands instead?

> I'm leaning slightly towards the approach that Benoît took, if only for
> the naming aspect (that is, I also thought of the idea of a bool flag,
> but didn't suggest it because I didn't like the implications on the
> naming).  But I can live with either approach, if anyone else has a
> strong opinion.

Well, we can pick up any descriptive name 'treat-device-as-a-node',
'device-is-a-graph-node'...

Re: [Qemu-devel] [PATCH V4 4/7] qmp: Allow to change password on names block driver states.

[Benoît Canet]

Le Friday 06 Dec 2013 à 11:52:15 (-0500), Luiz Capitulino a écrit :
> On Fri, 06 Dec 2013 08:24:33 -0700
> Eric Blake <eblake@redhat.com> wrote:
> 
> > On 12/06/2013 07:27 AM, Luiz Capitulino wrote:
> > > On Thu,  5 Dec 2013 18:15:00 +0100
> > > Benoît Canet <benoit@irqsave.net> wrote:
> > 
> > >> -{ 'command': 'block_passwd', 'data': {'device': 'str', 'password': 'str'} }
> > >> +{ 'command': 'block_passwd', 'data': {'*device': 'str',
> > >> +                                      '*node-name': 'str', 'password': 'str'} }
> > > 
> > > What about:
> > > 
> > > { 'command': 'block_passwd', 'data': {'device': 'str',
> > >                                       '*device-is-node': 'bool', 'password': 'str'} }
> > 
> > That would also work; the naming is a bit more awkward, but then you
> > don't have the issue of mutually-exclusive optional arguments where
> > exactly one of the two arguments is required.
> 
> Yes, and I dislike that very much.

Luiz, I will rewrite these patch using your boolean.

Best regards

Benoît

> 
> Btw, can anyone remind me why we can't have new commands instead?

That would mean doubling the whole QMP block command set once the work is done.

> 
> > I'm leaning slightly towards the approach that Benoît took, if only for
> > the naming aspect (that is, I also thought of the idea of a bool flag,
> > but didn't suggest it because I didn't like the implications on the
> > naming).  But I can live with either approach, if anyone else has a
> > strong opinion.
> 
> Well, we can pick up any descriptive name 'treat-device-as-a-node',
> 'device-is-a-graph-node'...
> 

Re: [Qemu-devel] [PATCH V4 0/7] Giving names to BlockDriverState graph nodes

[Kevin Wolf]

Am 05.12.2013 um 18:14 hat Benoît Canet geschrieben:
> This partial series start to add some node-name manipulation from QMP.
> In particular it will allow to take snapshots of quorum files.
> I propose reviewing it and merging if it's ok so quorum can be enabled and
> merged and I could start enable other block filter feature while writing next
> features. (Quorum file repair, crypto, multi bs block throttling)
> 
> v4:
>     s/prepare/prepare for/ [Eric]
>     s/followings/following/ [Eric]
>     fix option memory leak [Eric]
>     new command to get named bs name list [Eric]
>     * struct -> *struct [Eric/Fam]
>     Shorter comparison [Eric]
>     1.8 -> 2.0 [Eric/Fam]
>     More commments to explain authorization method [Fam]
>     Add a don't care result to snapshot authorization method [Fam]
>     Add #optional [Eric]

This series doesn't apply any more. I think I'm going to have a look
anyway, but providing a meaningful review is easier when you can apply
the patches for it.

Kevin

Re: [Qemu-devel] [PATCH V4 3/7] qmp: Add a command to list the named BlockDriverState nodes.

[Benoît Canet]

Le Friday 06 Dec 2013 à 08:59:38 (-0700), Eric Blake a écrit :
> On 12/05/2013 10:14 AM, Benoît Canet wrote:
> > Signed-off-by: Benoit Canet <benoit@irqsave.net>
> > ---
> >  block.c               | 16 ++++++++++++++++
> >  blockdev.c            |  5 +++++
> >  include/block/block.h |  1 +
> >  qapi-schema.json      | 11 +++++++++++
> >  qmp-commands.hx       | 19 +++++++++++++++++++
> >  5 files changed, 52 insertions(+)
> 
> >  ##
> > +# @query-named-block-nodes
> > +#
> > +# Get the named block driver list
> > +#
> > +# Returns: the list of named nodes names
> 
> Sounds funny.  Maybe better with just:
> 
> Returns: the list of names
> 
> > +#
> > +# Since 2.0
> > +##
> > +{ 'command': 'query-named-block-nodes', 'returns': [ 'str' ] }
> 
> Also, is list of names sufficient, or are we better off returning a list
> of structs with the details already included?  With list of names, the
> management app has to follow up with another QMP command per name if it
> wants to know details about each node, instead of getting it all in one
> command.

Would a list of BlockDeviceInfo be fine ?

Best regards

Benoît

> 
> > +SQMP
> > +@query-named-block-nodes
> > +------------------------
> > +
> > +Return a list of string containing the name of the named block driver states
> 
> Return a list of strings containing the name of each named block driver node
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 

Re: [Qemu-devel] [PATCH V4 0/7] Giving names to BlockDriverState graph nodes

[Benoît Canet]

Le Monday 09 Dec 2013 à 16:32:14 (+0100), Kevin Wolf a écrit :
> Am 05.12.2013 um 18:14 hat Benoît Canet geschrieben:
> > This partial series start to add some node-name manipulation from QMP.
> > In particular it will allow to take snapshots of quorum files.
> > I propose reviewing it and merging if it's ok so quorum can be enabled and
> > merged and I could start enable other block filter feature while writing next
> > features. (Quorum file repair, crypto, multi bs block throttling)
> > 
> > v4:
> >     s/prepare/prepare for/ [Eric]
> >     s/followings/following/ [Eric]
> >     fix option memory leak [Eric]
> >     new command to get named bs name list [Eric]
> >     * struct -> *struct [Eric/Fam]
> >     Shorter comparison [Eric]
> >     1.8 -> 2.0 [Eric/Fam]
> >     More commments to explain authorization method [Fam]
> >     Add a don't care result to snapshot authorization method [Fam]
> >     Add #optional [Eric]
> 
> This series doesn't apply any more. I think I'm going to have a look
> anyway, but providing a meaningful review is easier when you can apply
> the patches for it.

I am rebasing it on kevin/block and will respin it soon.

Thanks

Best regards

Benoît

> 
> Kevin
> 

Re: [Qemu-devel] [PATCH V4 1/7] block: Add bs->node_name to hold the name of a bs node of the bs graph.

[Kevin Wolf]

Am 05.12.2013 um 18:14 hat Benoît Canet geschrieben:
> Add the minimum of code to prepare for the following patches.
> 
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Should non-empty node names be checked so that no duplicates can be
created?

Kevin

Re: [Qemu-devel] [PATCH V4 1/7] block: Add bs->node_name to hold the name of a bs node of the bs graph.

[Kevin Wolf]

Am 05.12.2013 um 18:14 hat Benoît Canet geschrieben:
> Add the minimum of code to prepare for the following patches.
> 
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  block.c                   | 72 ++++++++++++++++++++++++++++++++++-------------
>  block/blkverify.c         |  2 +-
>  block/iscsi.c             |  2 +-
>  block/vmdk.c              |  2 +-
>  block/vvfat.c             |  4 +--
>  blockdev.c                |  8 +++---
>  hw/block/xen_disk.c       |  2 +-
>  include/block/block.h     |  3 +-
>  include/block/block_int.h |  9 +++++-
>  qemu-img.c                |  6 ++--
>  qemu-io.c                 |  2 +-
>  qemu-nbd.c                |  2 +-
>  12 files changed, 78 insertions(+), 36 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 3d78581..4f6b36a 100644
> --- a/block.c
> +++ b/block.c
> @@ -89,6 +89,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);
>  
> @@ -318,15 +321,21 @@ void bdrv_register(BlockDriver *bdrv)
>  }
>  
>  /* create a new block device (by default it is empty) */
> -BlockDriverState *bdrv_new(const char *device_name)
> +BlockDriverState *bdrv_new(const char *device_name, const char *node_name)
>  {
>      BlockDriverState *bs;
>  
> +    assert(node_name);
> +
>      bs = g_malloc0(sizeof(BlockDriverState));
>      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);
> +    }
> +    pstrcpy(bs->node_name, sizeof(bs->node_name), node_name);
> +    if (node_name[0] != '\0') {
> +        QTAILQ_INSERT_TAIL(&graph_bdrv_states, bs, node_list);
>      }
>      bdrv_iostatus_disable(bs);
>      notifier_list_init(&bs->close_notifiers);
> @@ -871,7 +880,7 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename,
>          options = qdict_new();
>      }
>  
> -    bs = bdrv_new("");
> +    bs = bdrv_new("", "");
>      bs->options = options;
>      options = qdict_clone_shallow(options);
>  
> @@ -993,7 +1002,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
>                                         sizeof(backing_filename));
>      }
>  
> -    bs->backing_hd = bdrv_new("");
> +    bs->backing_hd = bdrv_new("", "");
>  
>      if (bs->backing_format[0] != '\0') {
>          back_drv = bdrv_find_format(bs->backing_format);
> @@ -1059,7 +1068,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
>             instead of opening 'filename' directly */
>  
>          /* Get the required size from the image */
> -        bs1 = bdrv_new("");
> +        bs1 = bdrv_new("", "");
>          QINCREF(options);
>          ret = bdrv_open(bs1, filename, options, BDRV_O_NO_BACKING,
>                          drv, &local_err);
> @@ -1500,7 +1509,7 @@ void bdrv_close_all(void)
>  {
>      BlockDriverState *bs;
>  
> -    QTAILQ_FOREACH(bs, &bdrv_states, list) {
> +    QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
>          bdrv_close(bs);
>      }
>  }
> @@ -1529,7 +1538,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;
>          }
> @@ -1559,7 +1568,7 @@ void bdrv_drain_all(void)
>          /* FIXME: We do not have timer support here, so this is effectively
>           * a busy wait.
>           */
> -        QTAILQ_FOREACH(bs, &bdrv_states, list) {
> +        QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
>              if (bdrv_start_throttled_reqs(bs)) {
>                  busy = true;
>              }
> @@ -1570,14 +1579,18 @@ 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);
> +    }
>  }

Should we also set bs->node_name[0] = '\0'?

Kevin

Re: [Qemu-devel] [PATCH V4 2/7] block: Allow the user to define "node-name" option.

[Kevin Wolf]

Am 05.12.2013 um 18:14 hat Benoît Canet geschrieben:
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
>  block.c | 44 +++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 39 insertions(+), 5 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 4f6b36a..61f5ba0 100644
> --- a/block.c
> +++ b/block.c
> @@ -873,6 +873,7 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename,
>      const char *drvname;
>      bool allow_protocol_prefix = false;
>      Error *local_err = NULL;
> +    const char *node_name = NULL;
>      int ret;
>  
>      /* NULL means an empty set of options */
> @@ -880,7 +881,15 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename,
>          options = qdict_new();
>      }
>  
> -    bs = bdrv_new("", "");
> +    node_name = qdict_get_try_str(options, "node-name");
> +    if (node_name && bdrv_find_node(node_name)) {
> +        error_setg(errp, "Duplicate node name");
> +        QDECREF(options);
> +        return -EINVAL;
> +    }
> +    bs = bdrv_new("", node_name ? node_name : "");
> +    qdict_del(options, "node-name");
> +
>      bs->options = options;
>      options = qdict_clone_shallow(options);
>  
> @@ -980,6 +989,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
>      int back_flags, ret;
>      BlockDriver *back_drv = NULL;
>      Error *local_err = NULL;
> +    const char *node_name = NULL;
>  
>      if (bs->backing_hd != NULL) {
>          QDECREF(options);
> @@ -1002,7 +1012,14 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
>                                         sizeof(backing_filename));
>      }
>  
> -    bs->backing_hd = bdrv_new("", "");
> +    node_name = qdict_get_try_str(options, "node-name");
> +    if (node_name && bdrv_find_node(node_name)) {
> +        error_setg(errp, "Duplicate node name");
> +        QDECREF(options);
> +        return -EINVAL;
> +    }
> +    bs->backing_hd = bdrv_new("", node_name ? node_name : "");
> +    qdict_del(options, "node-name");
>  
>      if (bs->backing_format[0] != '\0') {
>          back_drv = bdrv_find_format(bs->backing_format);
> @@ -1046,6 +1063,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
>      BlockDriverState *file = NULL;
>      QDict *file_options = NULL;
>      const char *drvname;
> +    const char *node_name = NULL;
>      Error *local_err = NULL;
>  
>      /* NULL means an empty set of options */
> @@ -1053,6 +1071,22 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
>          options = qdict_new();
>      }
>  
> +    node_name = qdict_get_try_str(options, "node-name");
> +    if (node_name && bdrv_find_node(node_name)) {
> +        error_setg(errp, "Duplicate node name");
> +        QDECREF(options);
> +        return -EINVAL;
> +    }
> +
> +    if (node_name) {
> +        pstrcpy(bs->node_name, sizeof(bs->node_name), node_name);
> +        if (node_name[0] != '\0') {
> +            QTAILQ_INSERT_TAIL(&graph_bdrv_states, bs, node_list);
> +        }
> +    }
> +
> +    qdict_del(options, "node-name");

We duplicate some code all over the place. In general you seem to be
trying to let the caller of bdrv_new() already figure out what the node
name is by parsing the options QDict; here however, you do it after
bdrv_new().

Can't we consolidate this and only ever set the node name in
bdrv_open_common(), so that the option is parsed only once, there is
only once place adding BDSes to the list, and there is only one place
checking for duplicates?

Kevin

Re: [Qemu-devel] [PATCH V4 4/7] qmp: Allow to change password on names block driver states.

[Kevin Wolf]

Am 06.12.2013 um 17:52 hat Luiz Capitulino geschrieben:
> On Fri, 06 Dec 2013 08:24:33 -0700
> Eric Blake <eblake@redhat.com> wrote:
> 
> > On 12/06/2013 07:27 AM, Luiz Capitulino wrote:
> > > On Thu,  5 Dec 2013 18:15:00 +0100
> > > Benoît Canet <benoit@irqsave.net> wrote:
> > 
> > >> -{ 'command': 'block_passwd', 'data': {'device': 'str', 'password': 'str'} }
> > >> +{ 'command': 'block_passwd', 'data': {'*device': 'str',
> > >> +                                      '*node-name': 'str', 'password': 'str'} }
> > > 
> > > What about:
> > > 
> > > { 'command': 'block_passwd', 'data': {'device': 'str',
> > >                                       '*device-is-node': 'bool', 'password': 'str'} }
> > 
> > That would also work; the naming is a bit more awkward, but then you
> > don't have the issue of mutually-exclusive optional arguments where
> > exactly one of the two arguments is required.
> 
> Yes, and I dislike that very much.
> 
> Btw, can anyone remind me why we can't have new commands instead?

Because having to maintain two commands for the same functionality is
bad.

> > I'm leaning slightly towards the approach that Benoît took, if only for
> > the naming aspect (that is, I also thought of the idea of a bool flag,
> > but didn't suggest it because I didn't like the implications on the
> > naming).  But I can live with either approach, if anyone else has a
> > strong opinion.
> 
> Well, we can pick up any descriptive name 'treat-device-as-a-node',
> 'device-is-a-graph-node'...

All devices are represented by nodes, so that doesn't make sense.
If anything, 'interpret-device-name-as-node-name', which at the same
time makes it pretty clear that we're abusing a field for something it
wasn't meant for.

Kevin

Re: [Qemu-devel] [PATCH V4 5/7] qmp: Allow block_resize to manipulate bs graph nodes.

[Kevin Wolf]

Am 05.12.2013 um 18:15 hat Benoît Canet geschrieben:
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
>  blockdev.c       | 13 +++++++++----
>  hmp.c            |  2 +-
>  qapi-schema.json | 10 ++++++++--
>  qmp-commands.hx  |  3 ++-
>  4 files changed, 20 insertions(+), 8 deletions(-)

Why would you do this on non-top-level nodes?

Kevin

Re: [Qemu-devel] [PATCH V4 4/7] qmp: Allow to change password on names block driver states.

[Luiz Capitulino]

On Mon, 9 Dec 2013 17:23:09 +0100
Kevin Wolf <kwolf@redhat.com> wrote:

> > > I'm leaning slightly towards the approach that Benoît took, if only for
> > > the naming aspect (that is, I also thought of the idea of a bool flag,
> > > but didn't suggest it because I didn't like the implications on the
> > > naming).  But I can live with either approach, if anyone else has a
> > > strong opinion.
> > 
> > Well, we can pick up any descriptive name 'treat-device-as-a-node',
> > 'device-is-a-graph-node'...
> 
> All devices are represented by nodes, so that doesn't make sense.
> If anything, 'interpret-device-name-as-node-name', which at the same
> time makes it pretty clear that we're abusing a field for something it
> wasn't meant for.

Having two optionals where they can't be specified at the same time
and can't be left off at the same time is a clear abuse as well.

The truth is, both proposals are bad. This makes me think that maybe
we should introduce a block API 2.0 and deprecate the current one
(partly or completely).

Re: [Qemu-devel] [PATCH V4 5/7] qmp: Allow block_resize to manipulate bs graph nodes.

[Benoît Canet]

Le Monday 09 Dec 2013 à 17:24:09 (+0100), Kevin Wolf a écrit :
> Am 05.12.2013 um 18:15 hat Benoît Canet geschrieben:
> > Signed-off-by: Benoit Canet <benoit@irqsave.net>
> > ---
> >  blockdev.c       | 13 +++++++++----
> >  hmp.c            |  2 +-
> >  qapi-schema.json | 10 ++++++++--
> >  qmp-commands.hx  |  3 ++-
> >  4 files changed, 20 insertions(+), 8 deletions(-)
> 
> Why would you do this on non-top-level nodes?

What is the meaning of resizing a block filter (quorum/throttle) ?
Would not we let the user resize the qcow2 file below the filters directly ?

Best regards

Benoît

> 
> Kevin

Re: [Qemu-devel] [PATCH V4 4/7] qmp: Allow to change password on names block driver states.

[Benoît Canet]

Le Monday 09 Dec 2013 à 11:41:09 (-0500), Luiz Capitulino a écrit :
> On Mon, 9 Dec 2013 17:23:09 +0100
> Kevin Wolf <kwolf@redhat.com> wrote:
> 
> > > > I'm leaning slightly towards the approach that Benoît took, if only for
> > > > the naming aspect (that is, I also thought of the idea of a bool flag,
> > > > but didn't suggest it because I didn't like the implications on the
> > > > naming).  But I can live with either approach, if anyone else has a
> > > > strong opinion.
> > > 
> > > Well, we can pick up any descriptive name 'treat-device-as-a-node',
> > > 'device-is-a-graph-node'...
> > 
> > All devices are represented by nodes, so that doesn't make sense.
> > If anything, 'interpret-device-name-as-node-name', which at the same
> > time makes it pretty clear that we're abusing a field for something it
> > wasn't meant for.
> 
> Having two optionals where they can't be specified at the same time
> and can't be left off at the same time is a clear abuse as well.
> 
> The truth is, both proposals are bad. This makes me think that maybe
> we should introduce a block API 2.0 and deprecate the current one
> (partly or completely).
> 

It took me one year to go from the block filters and block backend
requirement to the state where my customer allows me to work on block filters.

Now if we add to this the new requirement of block API 2.0 I think I will soon
have time to concentrate myself on non qemu projects :(

Best regards

Benoît

Re: [Qemu-devel] [PATCH V4 4/7] qmp: Allow to change password on names block driver states.

[Luiz Capitulino]

On Mon, 9 Dec 2013 17:48:50 +0100
Benoît Canet <benoit.canet@irqsave.net> wrote:

> Le Monday 09 Dec 2013 à 11:41:09 (-0500), Luiz Capitulino a écrit :
> > On Mon, 9 Dec 2013 17:23:09 +0100
> > Kevin Wolf <kwolf@redhat.com> wrote:
> > 
> > > > > I'm leaning slightly towards the approach that Benoît took, if only for
> > > > > the naming aspect (that is, I also thought of the idea of a bool flag,
> > > > > but didn't suggest it because I didn't like the implications on the
> > > > > naming).  But I can live with either approach, if anyone else has a
> > > > > strong opinion.
> > > > 
> > > > Well, we can pick up any descriptive name 'treat-device-as-a-node',
> > > > 'device-is-a-graph-node'...
> > > 
> > > All devices are represented by nodes, so that doesn't make sense.
> > > If anything, 'interpret-device-name-as-node-name', which at the same
> > > time makes it pretty clear that we're abusing a field for something it
> > > wasn't meant for.
> > 
> > Having two optionals where they can't be specified at the same time
> > and can't be left off at the same time is a clear abuse as well.
> > 
> > The truth is, both proposals are bad. This makes me think that maybe
> > we should introduce a block API 2.0 and deprecate the current one
> > (partly or completely).
> > 
> 
> It took me one year to go from the block filters and block backend
> requirement to the state where my customer allows me to work on block filters.
> 
> Now if we add to this the new requirement of block API 2.0 I think I will soon
> have time to concentrate myself on non qemu projects :(

I don't think it would be something major as far as code is concerned.
What can take a lot of time and energy is to define the API. The QMP
commands implementation would probably be a wrapper around a single (or
a set of) block functions.

Again, I can live with what I suggested because I find it simpler
than your original proposal: no existing field is changed, only one
field is added, and clients can happily omit it if they don't know
what it's about.

Re: [Qemu-devel] [PATCH V4 4/7] qmp: Allow to change password on names block driver states.

[Benoît Canet]

Le Monday 09 Dec 2013 à 12:03:26 (-0500), Luiz Capitulino a écrit :
> On Mon, 9 Dec 2013 17:48:50 +0100
> Benoît Canet <benoit.canet@irqsave.net> wrote:
> 
> > Le Monday 09 Dec 2013 à 11:41:09 (-0500), Luiz Capitulino a écrit :
> > > On Mon, 9 Dec 2013 17:23:09 +0100
> > > Kevin Wolf <kwolf@redhat.com> wrote:
> > > 
> > > > > > I'm leaning slightly towards the approach that Benoît took, if only for
> > > > > > the naming aspect (that is, I also thought of the idea of a bool flag,
> > > > > > but didn't suggest it because I didn't like the implications on the
> > > > > > naming).  But I can live with either approach, if anyone else has a
> > > > > > strong opinion.
> > > > > 
> > > > > Well, we can pick up any descriptive name 'treat-device-as-a-node',
> > > > > 'device-is-a-graph-node'...
> > > > 
> > > > All devices are represented by nodes, so that doesn't make sense.
> > > > If anything, 'interpret-device-name-as-node-name', which at the same
> > > > time makes it pretty clear that we're abusing a field for something it
> > > > wasn't meant for.
> > > 
> > > Having two optionals where they can't be specified at the same time
> > > and can't be left off at the same time is a clear abuse as well.
> > > 
> > > The truth is, both proposals are bad. This makes me think that maybe
> > > we should introduce a block API 2.0 and deprecate the current one
> > > (partly or completely).
> > > 
> > 
> > It took me one year to go from the block filters and block backend
> > requirement to the state where my customer allows me to work on block filters.
> > 
> > Now if we add to this the new requirement of block API 2.0 I think I will soon
> > have time to concentrate myself on non qemu projects :(
> 
> I don't think it would be something major as far as code is concerned.
> What can take a lot of time and energy is to define the API. The QMP
> commands implementation would probably be a wrapper around a single (or
> a set of) block functions.
> 
> Again, I can live with what I suggested because I find it simpler
> than your original proposal: no existing field is changed, only one
> field is added, and clients can happily omit it if they don't know
> what it's about.
> 

I already have rewritten the patches to support your version of the commands.

I will let you people decide which version qemu will merge.

Best regards

Benoît

Re: [Qemu-devel] [PATCH V4 4/7] qmp: Allow to change password on names block driver states.

[Kevin Wolf]

Am 09.12.2013 um 17:41 hat Luiz Capitulino geschrieben:
> On Mon, 9 Dec 2013 17:23:09 +0100
> Kevin Wolf <kwolf@redhat.com> wrote:
> 
> > > > I'm leaning slightly towards the approach that Benoît took, if only for
> > > > the naming aspect (that is, I also thought of the idea of a bool flag,
> > > > but didn't suggest it because I didn't like the implications on the
> > > > naming).  But I can live with either approach, if anyone else has a
> > > > strong opinion.
> > > 
> > > Well, we can pick up any descriptive name 'treat-device-as-a-node',
> > > 'device-is-a-graph-node'...
> > 
> > All devices are represented by nodes, so that doesn't make sense.
> > If anything, 'interpret-device-name-as-node-name', which at the same
> > time makes it pretty clear that we're abusing a field for something it
> > wasn't meant for.
> 
> Having two optionals where they can't be specified at the same time
> and can't be left off at the same time is a clear abuse as well.

Is it? If you wanted to express this in the schema, we'd need to extend
the QAPI generator, but until now we never have. I don't think this is
the first time that optional fields are not completely independent, but
may be required/forbidden based on values of other fields. Documenting
it should be enough, in my opinion.

> The truth is, both proposals are bad. This makes me think that maybe
> we should introduce a block API 2.0 and deprecate the current one
> (partly or completely).

Nice try, but of course this is equivalent to the "new command"
solution.  Deprecating the old version doesn't get you rid of it, you
still need to support it for compatibility. And then you're back to
square one.

For what it's worth, I think what Benoît implemented is the outcome of
discussions of the Block BOF on KVM Forum that included both block layer
developers and API users (i.e. libvirt), after considering and
dismissing other options (which, of course, included separate commands).

Kevin

Re: [Qemu-devel] [PATCH V4 5/7] qmp: Allow block_resize to manipulate bs graph nodes.

[Kevin Wolf]

Am 09.12.2013 um 17:41 hat Benoît Canet geschrieben:
> Le Monday 09 Dec 2013 à 17:24:09 (+0100), Kevin Wolf a écrit :
> > Am 05.12.2013 um 18:15 hat Benoît Canet geschrieben:
> > > Signed-off-by: Benoit Canet <benoit@irqsave.net>
> > > ---
> > >  blockdev.c       | 13 +++++++++----
> > >  hmp.c            |  2 +-
> > >  qapi-schema.json | 10 ++++++++--
> > >  qmp-commands.hx  |  3 ++-
> > >  4 files changed, 20 insertions(+), 8 deletions(-)
> > 
> > Why would you do this on non-top-level nodes?
> 
> What is the meaning of resizing a block filter (quorum/throttle) ?
> Would not we let the user resize the qcow2 file below the filters directly ?

Hm, right... But you wouldn't do it below the first non-filter. I
imagine we need similar rules as for snapshotting there?

Kevin

Re: [Qemu-devel] [PATCH V4 4/7] qmp: Allow to change password on names block driver states.

[Luiz Capitulino]

On Tue, 10 Dec 2013 10:57:50 +0100
Kevin Wolf <kwolf@redhat.com> wrote:

> Am 09.12.2013 um 17:41 hat Luiz Capitulino geschrieben:
> > On Mon, 9 Dec 2013 17:23:09 +0100
> > Kevin Wolf <kwolf@redhat.com> wrote:
> > 
> > > > > I'm leaning slightly towards the approach that Benoît took, if only for
> > > > > the naming aspect (that is, I also thought of the idea of a bool flag,
> > > > > but didn't suggest it because I didn't like the implications on the
> > > > > naming).  But I can live with either approach, if anyone else has a
> > > > > strong opinion.
> > > > 
> > > > Well, we can pick up any descriptive name 'treat-device-as-a-node',
> > > > 'device-is-a-graph-node'...
> > > 
> > > All devices are represented by nodes, so that doesn't make sense.
> > > If anything, 'interpret-device-name-as-node-name', which at the same
> > > time makes it pretty clear that we're abusing a field for something it
> > > wasn't meant for.
> > 
> > Having two optionals where they can't be specified at the same time
> > and can't be left off at the same time is a clear abuse as well.
> 
> Is it? If you wanted to express this in the schema, we'd need to extend
> the QAPI generator, but until now we never have. I don't think this is
> the first time that optional fields are not completely independent, but
> may be required/forbidden based on values of other fields. Documenting
> it should be enough, in my opinion.

We disagree here, and what makes my objection strong is that I
provided an alternative which I believe is less worse because it
makes less changes to the command.

> > The truth is, both proposals are bad. This makes me think that maybe
> > we should introduce a block API 2.0 and deprecate the current one
> > (partly or completely).
> 
> Nice try, but of course this is equivalent to the "new command"
> solution.  Deprecating the old version doesn't get you rid of it, you
> still need to support it for compatibility. And then you're back to
> square one.

We can't get rid of anything in QMP. Deprecating means that the command
is still available but a better replacement exists and should be used
in new implementations.

> For what it's worth, I think what Benoît implemented is the outcome of
> discussions of the Block BOF on KVM Forum that included both block layer
> developers and API users (i.e. libvirt), after considering and
> dismissing other options (which, of course, included separate commands).

I appreciate those discussions, but patch review and acceptance happens
on upstream lists.

Re: [Qemu-devel] [PATCH V4 4/7] qmp: Allow to change password on names block driver states.

[Kevin Wolf]

Am 10.12.2013 um 15:06 hat Luiz Capitulino geschrieben:
> On Tue, 10 Dec 2013 10:57:50 +0100
> Kevin Wolf <kwolf@redhat.com> wrote:
> 
> > Am 09.12.2013 um 17:41 hat Luiz Capitulino geschrieben:
> > > On Mon, 9 Dec 2013 17:23:09 +0100
> > > Kevin Wolf <kwolf@redhat.com> wrote:
> > > 
> > > > > > I'm leaning slightly towards the approach that Benoît took, if only for
> > > > > > the naming aspect (that is, I also thought of the idea of a bool flag,
> > > > > > but didn't suggest it because I didn't like the implications on the
> > > > > > naming).  But I can live with either approach, if anyone else has a
> > > > > > strong opinion.
> > > > > 
> > > > > Well, we can pick up any descriptive name 'treat-device-as-a-node',
> > > > > 'device-is-a-graph-node'...
> > > > 
> > > > All devices are represented by nodes, so that doesn't make sense.
> > > > If anything, 'interpret-device-name-as-node-name', which at the same
> > > > time makes it pretty clear that we're abusing a field for something it
> > > > wasn't meant for.
> > > 
> > > Having two optionals where they can't be specified at the same time
> > > and can't be left off at the same time is a clear abuse as well.
> > 
> > Is it? If you wanted to express this in the schema, we'd need to extend
> > the QAPI generator, but until now we never have. I don't think this is
> > the first time that optional fields are not completely independent, but
> > may be required/forbidden based on values of other fields. Documenting
> > it should be enough, in my opinion.
> 
> We disagree here, and what makes my objection strong is that I
> provided an alternative which I believe is less worse because it
> makes less changes to the command.

My objection to your approach is strong because Benoît already sent an
alternative which I believe is less worse because with it, arguments
actually mean what their names tell instead of having additional bools
for "oh, and I said A, but I didn't mean it, I really want B".

> > > The truth is, both proposals are bad. This makes me think that maybe
> > > we should introduce a block API 2.0 and deprecate the current one
> > > (partly or completely).
> > 
> > Nice try, but of course this is equivalent to the "new command"
> > solution.  Deprecating the old version doesn't get you rid of it, you
> > still need to support it for compatibility. And then you're back to
> > square one.
> 
> We can't get rid of anything in QMP. Deprecating means that the command
> is still available but a better replacement exists and should be used
> in new implementations.

At least one thing on which we agree.

However, suggesting to deprecate the old version would mean that we
think that referencing by device name is a bad thing that you shouldn't
be doing any more. I don't think it's true.

> > For what it's worth, I think what Benoît implemented is the outcome of
> > discussions of the Block BOF on KVM Forum that included both block layer
> > developers and API users (i.e. libvirt), after considering and
> > dismissing other options (which, of course, included separate commands).
> 
> I appreciate those discussions, but patch review and acceptance happens
> on upstream lists.

Okay. So you're going to block an interface that block layer developers
and block layer users have agreed on because it doesn't match your
aesthetic demands - and you wrote some parts of the infrastructure we're
using, so you have an inherent right to have them met?

I love productive discussions like this.

Kevin

Re: [Qemu-devel] [PATCH V4 4/7] qmp: Allow to change password on names block driver states.

[Luiz Capitulino]

On Tue, 10 Dec 2013 15:25:07 +0100
Kevin Wolf <kwolf@redhat.com> wrote:

> My objection to your approach is strong because Benoît already sent an
> alternative which I believe is less worse because with it, arguments
> actually mean what their names tell instead of having additional bools
> for "oh, and I said A, but I didn't mean it, I really want B".

Current proposal:

{ 'command': 'block_passwd', 'data': {'*device': 'str',
                                      '*node-name': 'str', 'password': 'str'} }

When I look at it, I ask myself:

 - What happens when device=NULL?

 - What happens when node-name=NULL?

 - What happens when device=NULL and node-name=NULL?

 - What happens when device != NULL and node-node != NULL?

 - What happens when device != NULL but node-node=NULL?

 - What happens when device=NULL but node-node != NULL?

My proposal:

{ 'command': 'block_passwd', 'data': {'device': 'str',
                                      '*device-is-node': 'bool', 'password': 'str'} }

 - What happens when device-is-node=NULL?

 - What happens when device-is-node != NULL?

PS: I'm not NACKing anything. My review to this series started with a
    "what about" question. I did voice my objections against this
    proposal, but didn't NACK it. Besides you're a maintainer as much
    as I am, so I could NACK this as much as you could push this
    through your tree ignoring review.

Re: [Qemu-devel] [PATCH V4 3/7] qmp: Add a command to list the named BlockDriverState nodes.

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 637 bytes --]

On 12/09/2013 08:46 AM, Benoît Canet wrote:
>>> +{ 'command': 'query-named-block-nodes', 'returns': [ 'str' ] }
>>
>> Also, is list of names sufficient, or are we better off returning a list
>> of structs with the details already included?  With list of names, the
>> management app has to follow up with another QMP command per name if it
>> wants to know details about each node, instead of getting it all in one
>> command.
> 
> Would a list of BlockDeviceInfo be fine ?

Yes, I think that would be just fine.

-- 
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: 621 bytes --]

Re: [Qemu-devel] [PATCH V4 4/7] qmp: Allow to change password on names block driver states.

[Kevin Wolf]

Am 10.12.2013 um 16:16 hat Luiz Capitulino geschrieben:
> On Tue, 10 Dec 2013 15:25:07 +0100
> Kevin Wolf <kwolf@redhat.com> wrote:
> 
> > My objection to your approach is strong because Benoît already sent an
> > alternative which I believe is less worse because with it, arguments
> > actually mean what their names tell instead of having additional bools
> > for "oh, and I said A, but I didn't mean it, I really want B".
> 
> Current proposal:
> 
> { 'command': 'block_passwd', 'data': {'*device': 'str',
>                                       '*node-name': 'str', 'password': 'str'} }
> 
> When I look at it, I ask myself:
> 
>  - What happens when device=NULL?
> 
>  - What happens when node-name=NULL?
> 
>  - What happens when device=NULL and node-name=NULL?
> 
>  - What happens when device != NULL and node-node != NULL?
> 
>  - What happens when device != NULL but node-node=NULL?
> 
>  - What happens when device=NULL but node-node != NULL?

Looks pretty long, but the latter four are just the combination of the
former two.

I think it's relevant in what context you are looking at it. As a user,
the question I'm really asking is:

    Hm, okay, passing just a password can't be enough, so...
    - ...do I need to specificy device, and if so, with which value?
    - ...do I need to specificy node-name, and if so, with which value?

Looking at the documentation comment would make it clear rather quickly
that I should use only one of them and what the right value is.

For the implementation of the command, I actually need to think about
all the corner cases. However, even there a comment "device and
node-name may never be specified at the same time" makes it pretty clear
what I'm supposed to implement.

For debugging, suppose I read a QMP command in a log file, I read either
"device" or "node-name" and I know exactly what is meant.

> My proposal:
> 
> { 'command': 'block_passwd', 'data': {'device': 'str',
>                                       '*device-is-node': 'bool', 'password': 'str'} }
>
>  - What happens when device-is-node=NULL?
> 
>  - What happens when device-is-node != NULL?

Okay, as a user, I see that need to pass a device name and a password,
fine. Hm, I really wanted to pass a node name instead... Okay, a second
look shows me the optional flag that changes the meaning of "device", so
I use that, fine.

Implementing the functionality in qemu, this can't be hard. I just need
to check the flag when I search the BlockDriverState so that I search by
node-name or by device name, depending on what is requested. Hopefully
nobody will even use the 'device' parameter outside my if block because
it's overloaded with different meanings, so they would introduce a bug.

Debugging a QMP command in a log file, I see the 'device' key and am
happy because now I know the name of the device that caused trouble. Oh,
there was the "I didn't really mean 'device'" flag set? Whoops...


So perhaps in some cases you have to ask yourself more questions with
the separate fields, but the "whoops" cases of misunderstanding the
overloaded field will mean that people might not even think of asking a
question, but rather just do the wrong thing.

> PS: I'm not NACKing anything. My review to this series started with a
>     "what about" question. I did voice my objections against this
>     proposal, but didn't NACK it. Besides you're a maintainer as much
>     as I am, so I could NACK this as much as you could push this
>     through your tree ignoring review.

I hope we agree that edit wars aren't where we're going to go.

Kevin

Re: [Qemu-devel] [PATCH V4 4/7] qmp: Allow to change password on names block driver states.

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2683 bytes --]

On 12/10/2013 08:16 AM, Luiz Capitulino wrote:
> On Tue, 10 Dec 2013 15:25:07 +0100
> Kevin Wolf <kwolf@redhat.com> wrote:
> 
>> My objection to your approach is strong because Benoît already sent an
>> alternative which I believe is less worse because with it, arguments
>> actually mean what their names tell instead of having additional bools
>> for "oh, and I said A, but I didn't mean it, I really want B".
> 
> Current proposal:
> 
> { 'command': 'block_passwd', 'data': {'*device': 'str',
>                                       '*node-name': 'str', 'password': 'str'} }
> 
> When I look at it, I ask myself:
> 
>  - What happens when device=NULL?

Then, per docs, node-name must be non-NULL.

> 
>  - What happens when node-name=NULL?

Then, per docs, device must be non-NULL.

> 
>  - What happens when device=NULL and node-name=NULL?

Error; violates docs.

> 
>  - What happens when device != NULL and node-node != NULL?

Error; violates docs.

> 
>  - What happens when device != NULL but node-node=NULL?

Operate on the device.

> 
>  - What happens when device=NULL but node-node != NULL?

Operate on the node-name.

> 
> My proposal:
> 
> { 'command': 'block_passwd', 'data': {'device': 'str',
>                                       '*device-is-node': 'bool', 'password': 'str'} }
> 
>  - What happens when device-is-node=NULL?

Operate on the device (same as if device-is-node were false)

> 
>  - What happens when device-is-node != NULL?

The state of the bool determines whether we are operating on device or
on node.

> 
> PS: I'm not NACKing anything. My review to this series started with a
>     "what about" question. I did voice my objections against this
>     proposal, but didn't NACK it. Besides you're a maintainer as much
>     as I am, so I could NACK this as much as you could push this
>     through your tree ignoring review.

And I appreciate the critical review - if we capture design decisions
like this in the commit message, then a year from now when someone asks
"why didn't you do it this alternative way" we can say "look at the
commit message which explores the tradeoffs, and why we settled for what
we did" rather than saying "oops, we painted ourselves into a corner
because we didn't think about that".

The more this goes on, the more I like the mutually-exclusive
device[str]/node-name[str] arguments, and the less I like the
device[str]/device-is-node[bool] solution, but I can still live with
either approach from libvirt's point of view.

-- 
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: 621 bytes --]

Re: [Qemu-devel] [PATCH V4 4/7] qmp: Allow to change password on names block driver states.

[Fam Zheng]

On 2013年12月10日 23:16, Luiz Capitulino wrote:
> On Tue, 10 Dec 2013 15:25:07 +0100
> Kevin Wolf <kwolf@redhat.com> wrote:
>
>> My objection to your approach is strong because Benoît already sent an
>> alternative which I believe is less worse because with it, arguments
>> actually mean what their names tell instead of having additional bools
>> for "oh, and I said A, but I didn't mean it, I really want B".
>
> Current proposal:
>
> { 'command': 'block_passwd', 'data': {'*device': 'str',
>                                        '*node-name': 'str', 'password': 'str'} }
>

I vote for this.

> When I look at it, I ask myself:
>
>   - What happens when device=NULL?
>
>   - What happens when node-name=NULL?
>
>   - What happens when device=NULL and node-name=NULL?
>
>   - What happens when device != NULL and node-node != NULL?
>
>   - What happens when device != NULL but node-node=NULL?
>
>   - What happens when device=NULL but node-node != NULL?
>
> My proposal:
>
> { 'command': 'block_passwd', 'data': {'device': 'str',
>                                        '*device-is-node': 'bool', 'password': 'str'} }
>
>   - What happens when device-is-node=NULL?
>
>   - What happens when device-is-node != NULL?
>

I think our starting point is that device names and node names are 
separate name spaces. For this reason we should avoid connecting them by 
reusing one field. From a user's view, "device-is-node", or anything 
meaning this, just doesn't sound interesting, because we also told them 
"device-is-not-node".

Fam

Re: [Qemu-devel] [PATCH V4 4/7] qmp: Allow to change password on names block driver states.

[Luiz Capitulino]

On Wed, 11 Dec 2013 11:52:28 +0800
Fam Zheng <famz@redhat.com> wrote:

> On 2013年12月10日 23:16, Luiz Capitulino wrote:
> > On Tue, 10 Dec 2013 15:25:07 +0100
> > Kevin Wolf <kwolf@redhat.com> wrote:
> >
> >> My objection to your approach is strong because Benoît already sent an
> >> alternative which I believe is less worse because with it, arguments
> >> actually mean what their names tell instead of having additional bools
> >> for "oh, and I said A, but I didn't mean it, I really want B".
> >
> > Current proposal:
> >
> > { 'command': 'block_passwd', 'data': {'*device': 'str',
> >                                        '*node-name': 'str', 'password': 'str'} }
> >
> 
> I vote for this.

Ok. As it's clear that I've failed to demonstrate how I think we're
going to move all those commands into the wrong direction, I think it's
time to withdrawn my suggestion.

Re: [Qemu-devel] [PATCH V4 2/7] block: Allow the user to define "node-name" option.

[Benoît Canet]

Le Monday 09 Dec 2013 à 17:15:26 (+0100), Kevin Wolf a écrit :
> Am 05.12.2013 um 18:14 hat Benoît Canet geschrieben:
> > Signed-off-by: Benoit Canet <benoit@irqsave.net>
> > ---
> >  block.c | 44 +++++++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 39 insertions(+), 5 deletions(-)
> > 
> > diff --git a/block.c b/block.c
> > index 4f6b36a..61f5ba0 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -873,6 +873,7 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename,
> >      const char *drvname;
> >      bool allow_protocol_prefix = false;
> >      Error *local_err = NULL;
> > +    const char *node_name = NULL;
> >      int ret;
> >  
> >      /* NULL means an empty set of options */
> > @@ -880,7 +881,15 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename,
> >          options = qdict_new();
> >      }
> >  
> > -    bs = bdrv_new("", "");
> > +    node_name = qdict_get_try_str(options, "node-name");
> > +    if (node_name && bdrv_find_node(node_name)) {
> > +        error_setg(errp, "Duplicate node name");
> > +        QDECREF(options);
> > +        return -EINVAL;
> > +    }
> > +    bs = bdrv_new("", node_name ? node_name : "");
> > +    qdict_del(options, "node-name");
> > +
> >      bs->options = options;
> >      options = qdict_clone_shallow(options);
> >  
> > @@ -980,6 +989,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
> >      int back_flags, ret;
> >      BlockDriver *back_drv = NULL;
> >      Error *local_err = NULL;
> > +    const char *node_name = NULL;
> >  
> >      if (bs->backing_hd != NULL) {
> >          QDECREF(options);
> > @@ -1002,7 +1012,14 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
> >                                         sizeof(backing_filename));
> >      }
> >  
> > -    bs->backing_hd = bdrv_new("", "");
> > +    node_name = qdict_get_try_str(options, "node-name");
> > +    if (node_name && bdrv_find_node(node_name)) {
> > +        error_setg(errp, "Duplicate node name");
> > +        QDECREF(options);
> > +        return -EINVAL;
> > +    }
> > +    bs->backing_hd = bdrv_new("", node_name ? node_name : "");
> > +    qdict_del(options, "node-name");
> >  
> >      if (bs->backing_format[0] != '\0') {
> >          back_drv = bdrv_find_format(bs->backing_format);
> > @@ -1046,6 +1063,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
> >      BlockDriverState *file = NULL;
> >      QDict *file_options = NULL;
> >      const char *drvname;
> > +    const char *node_name = NULL;
> >      Error *local_err = NULL;
> >  
> >      /* NULL means an empty set of options */
> > @@ -1053,6 +1071,22 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
> >          options = qdict_new();
> >      }
> >  
> > +    node_name = qdict_get_try_str(options, "node-name");
> > +    if (node_name && bdrv_find_node(node_name)) {
> > +        error_setg(errp, "Duplicate node name");
> > +        QDECREF(options);
> > +        return -EINVAL;
> > +    }
> > +
> > +    if (node_name) {
> > +        pstrcpy(bs->node_name, sizeof(bs->node_name), node_name);
> > +        if (node_name[0] != '\0') {
> > +            QTAILQ_INSERT_TAIL(&graph_bdrv_states, bs, node_list);
> > +        }
> > +    }
> > +
> > +    qdict_del(options, "node-name");
> 
> We duplicate some code all over the place. In general you seem to be
> trying to let the caller of bdrv_new() already figure out what the node
> name is by parsing the options QDict; here however, you do it after
> bdrv_new().
> 
> Can't we consolidate this and only ever set the node name in
> bdrv_open_common(), so that the option is parsed only once, there is
> only once place adding BDSes to the list, and there is only one place
> checking for duplicates?

Nice idea it will make the patches much smaller and cleaner.

Best regards

Benoît

> 
> Kevin