[Qemu-devel] [RFC V3 0/7] Giving names to BlockDriverState graph nodes

[Qemu-devel] [RFC V3 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 allow to take snapshots of quorum files.
I post it in a partial state to spread the reviewing load.

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.
  qapi: Add skeletton of command to query a drive bs graph.
  qmp: Allow block_passwd to manipulate bs graph nodes.
  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                   | 201 +++++++++++++++++++++++++++++++++++++++-------
 block/blkverify.c         |   4 +-
 block/iscsi.c             |   2 +-
 block/vmdk.c              |   2 +-
 block/vvfat.c             |   4 +-
 blockdev.c                |  89 +++++++++++++++-----
 hmp.c                     |   8 +-
 hw/block/xen_disk.c       |   2 +-
 include/block/block.h     |  22 +++--
 include/block/block_int.h |  18 ++++-
 qapi-schema.json          |  64 +++++++++++++--
 qemu-img.c                |   6 +-
 qemu-io.c                 |   2 +-
 qemu-nbd.c                |   2 +-
 qmp-commands.hx           |  17 +++-
 15 files changed, 363 insertions(+), 80 deletions(-)

-- 
1.8.3.2

[Qemu-devel] [RFC V3 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 the followings patches.

Signed-off-by: Benoit Canet <benoit@irqsave.net>
---
 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 55819a0..e755e4e 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

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

[Eric Blake]

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

On 12/03/2013 06:25 AM, Benoît Canet wrote:
> Add the minimum of code to prepare the followings patches.

s/prepare/prepare for/
s/followings/following/

> 
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---

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

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

[Benoît Canet]

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

diff --git a/block.c b/block.c
index 4f6b36a..3a0cb30 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,14 @@ 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");
+        return -EINVAL;
+    }
+    bs = bdrv_new("", node_name ? node_name : "");
+    qdict_del(options, "node-name");
+
     bs->options = options;
     options = qdict_clone_shallow(options);
 
@@ -980,6 +988,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 +1011,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 +1062,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 +1070,21 @@ 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");
+        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 +1678,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

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

[Eric Blake]

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

On 12/03/2013 06:26 AM, Benoît Canet wrote:
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
>  block.c | 42 +++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 37 insertions(+), 5 deletions(-)
> 

> @@ -880,7 +881,14 @@ 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");
> +        return -EINVAL;

This leaks options on failure.

> @@ -1053,6 +1070,21 @@ 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");
> +        return -EINVAL;

So does this.

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

[Qemu-devel] [RFC V3 3/7] qapi: Add skeletton of command to query a drive bs graph.

[Benoît Canet]

---
 blockdev.c       |  8 ++++++++
 qapi-schema.json | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index a474bb5..824e718 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1940,6 +1940,14 @@ void qmp_drive_backup(const char *device, const char *target,
     }
 }
 
+BlockGraphNode * qmp_query_drive_graph(const char *device, Error **errp)
+{
+    /* the implementation of this function would recurse through the
+     * BlockDriverState graph to build it's result
+     */
+    return NULL;
+}
+
 #define DEFAULT_MIRROR_BUF_SIZE   (10 << 20)
 
 void qmp_drive_mirror(const char *device, const char *target,
diff --git a/qapi-schema.json b/qapi-schema.json
index 8630eb5..938f8b9 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2008,6 +2008,38 @@
 { 'command': 'drive-backup', 'data': 'DriveBackup' }
 
 ##
+# @BlockGraphNode
+#
+# Information about a node of the block driver state graph
+#
+# @node-name: the name of the node in the graph
+#
+# @drv: the name of the block format used by this node as described in
+#       @BlockDeviceInfo.
+#
+# @children: a list of @BlockGraphNode being the children of this node
+#
+# Since 1.8
+##
+{ 'type': 'BlockGraphNode',
+  'data': { 'node-name': 'str', 'drv': 'str', 'children': ['BlockGraphNode'] } }
+
+##
+# @query-drive-graph
+#
+# Get the block driver states graph for a given drive
+#
+# @device: the name of the device to get the graph from
+#
+# Returns: the root @BlockGraphNode
+#
+# Since 1.8
+##
+{ 'command': 'query-drive-graph',
+  'data': { 'device': 'str' },
+  'returns': 'BlockGraphNode' }
+
+##
 # @drive-mirror
 #
 # Start mirroring a block device's writes to a new destination.
-- 
1.8.3.2

Re: [Qemu-devel] [RFC V3 3/7] qapi: Add skeletton of command to query a drive bs graph.

[Fam Zheng]

On 2013年12月03日 21:26, Benoît Canet wrote:
> ---
>   blockdev.c       |  8 ++++++++
>   qapi-schema.json | 32 ++++++++++++++++++++++++++++++++
>   2 files changed, 40 insertions(+)
>
> diff --git a/blockdev.c b/blockdev.c
> index a474bb5..824e718 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1940,6 +1940,14 @@ void qmp_drive_backup(const char *device, const char *target,
>       }
>   }
>
> +BlockGraphNode * qmp_query_drive_graph(const char *device, Error **errp)
> +{
> +    /* the implementation of this function would recurse through the
> +     * BlockDriverState graph to build it's result

s/it's/its/

>
>   ##
> +# @BlockGraphNode
> +#
> +# Information about a node of the block driver state graph
> +#
> +# @node-name: the name of the node in the graph
> +#
> +# @drv: the name of the block format used by this node as described in
> +#       @BlockDeviceInfo.
> +#
> +# @children: a list of @BlockGraphNode being the children of this node
> +#
> +# Since 1.8

Since 2.0

> +##
> +{ 'type': 'BlockGraphNode',
> +  'data': { 'node-name': 'str', 'drv': 'str', 'children': ['BlockGraphNode'] } }
> +
> +##
> +# @query-drive-graph
> +#
> +# Get the block driver states graph for a given drive
> +#
> +# @device: the name of the device to get the graph from
> +#
> +# Returns: the root @BlockGraphNode
> +#
> +# Since 1.8

Same as above.

Fam

Re: [Qemu-devel] [RFC V3 3/7] qapi: Add skeletton of command to query a drive bs graph.

[Eric Blake]

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

On 12/03/2013 06:26 AM, Benoît Canet wrote:

In addition to Fam's review,

s/skeletton/skeleton/ in subject

> ---
>  blockdev.c       |  8 ++++++++
>  qapi-schema.json | 32 ++++++++++++++++++++++++++++++++
>  2 files changed, 40 insertions(+)
> 
> diff --git a/blockdev.c b/blockdev.c
> index a474bb5..824e718 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1940,6 +1940,14 @@ void qmp_drive_backup(const char *device, const char *target,
>      }
>  }
>  
> +BlockGraphNode * qmp_query_drive_graph(const char *device, Error **errp)

Style: no space after *

> +{
> +    /* the implementation of this function would recurse through the
> +     * BlockDriverState graph to build it's result
> +     */
> +    return NULL;

Shouldn't you set errp when returning failure?

> +++ b/qapi-schema.json
> @@ -2008,6 +2008,38 @@
>  { 'command': 'drive-backup', 'data': 'DriveBackup' }
>  
>  ##
> +# @BlockGraphNode
> +#
> +# Information about a node of the block driver state graph
> +#
> +# @node-name: the name of the node in the graph
> +#
> +# @drv: the name of the block format used by this node as described in
> +#       @BlockDeviceInfo.

It would be nice if BlockDeviceInfo and BlockGraphNode used an enum
rather than an open-coded string for this field.

> +#
> +# @children: a list of @BlockGraphNode being the children of this node

s/being/that are/

> +##
> +# @query-drive-graph
> +#
> +# Get the block driver states graph for a given drive
> +#
> +# @device: the name of the device to get the graph from
> +#
> +# Returns: the root @BlockGraphNode
> +#
> +# Since 1.8
> +##
> +{ 'command': 'query-drive-graph',
> +  'data': { 'device': 'str' },
> +  'returns': 'BlockGraphNode' }

Am I correct that it will be possible to have named nodes that are not
currently associated with any device?  If so, how do we learn about
those nodes?  Would it be better to have a command that returns an array
of structs for all known node roots, with an optional member describing
which device owns that node root?  Something like:

# Represent a root of a block graph
# @root: a named node forming a root of a node graph
# @device: #optional device name that owns this root
{ 'type': 'BlockGraphRoot',
  'data': { 'root': 'BlockGraphNode',
            '*device': 'str' } }

# @query_drive-graphs
# Returns an array of all node graph roots
{ 'command': 'query-drive-graphs',
  'returns': [ 'BlockGraphRoot' ] }

possibly with 'data':{'*device':'str'} to allow filtering to just a
1-element array based on the device name (although I'm not sure if
providing the complexity of filtering is worth it).

-- 
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] [RFC V3 3/7] qapi: Add skeletton of command to query a drive bs graph.

[Benoît Canet]

Le Wednesday 04 Dec 2013 à 16:46:34 (-0700), Eric Blake a écrit :
> On 12/03/2013 06:26 AM, Benoît Canet wrote:
> 
> In addition to Fam's review,
> 
> s/skeletton/skeleton/ in subject
> 
> > ---
> >  blockdev.c       |  8 ++++++++
> >  qapi-schema.json | 32 ++++++++++++++++++++++++++++++++
> >  2 files changed, 40 insertions(+)
> > 
> > diff --git a/blockdev.c b/blockdev.c
> > index a474bb5..824e718 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -1940,6 +1940,14 @@ void qmp_drive_backup(const char *device, const char *target,
> >      }
> >  }
> >  
> > +BlockGraphNode * qmp_query_drive_graph(const char *device, Error **errp)
> 
> Style: no space after *
> 
> > +{
> > +    /* the implementation of this function would recurse through the
> > +     * BlockDriverState graph to build it's result
> > +     */
> > +    return NULL;
> 
> Shouldn't you set errp when returning failure?
> 
> > +++ b/qapi-schema.json
> > @@ -2008,6 +2008,38 @@
> >  { 'command': 'drive-backup', 'data': 'DriveBackup' }
> >  
> >  ##
> > +# @BlockGraphNode
> > +#
> > +# Information about a node of the block driver state graph
> > +#
> > +# @node-name: the name of the node in the graph
> > +#
> > +# @drv: the name of the block format used by this node as described in
> > +#       @BlockDeviceInfo.
> 
> It would be nice if BlockDeviceInfo and BlockGraphNode used an enum
> rather than an open-coded string for this field.
> 
> > +#
> > +# @children: a list of @BlockGraphNode being the children of this node
> 
> s/being/that are/
> 
> > +##
> > +# @query-drive-graph
> > +#
> > +# Get the block driver states graph for a given drive
> > +#
> > +# @device: the name of the device to get the graph from
> > +#
> > +# Returns: the root @BlockGraphNode
> > +#
> > +# Since 1.8
> > +##
> > +{ 'command': 'query-drive-graph',
> > +  'data': { 'device': 'str' },
> > +  'returns': 'BlockGraphNode' }
> 
> Am I correct that it will be possible to have named nodes that are not
> currently associated with any device?  If so, how do we learn about
> those nodes?  Would it be better to have a command that returns an array
> of structs for all known node roots, with an optional member describing
> which device owns that node root?  Something like:

The code have a list of all named nodes but not a list of named nodes roots.
Also it's difficult to get the device name for a named node because the bses don't
have any backward pointers to their parents.
It could be done by recursing into all the blockbackend bs but it's twisted.

In fact I am wondering if we really need something to spit out the named nodes
topology in QMP for the simple reason that the names of the nodes are given by the
management so the management should already know the topology.

Best regards

Benoît

> 
> # Represent a root of a block graph
> # @root: a named node forming a root of a node graph
> # @device: #optional device name that owns this root
> { 'type': 'BlockGraphRoot',
>   'data': { 'root': 'BlockGraphNode',
>             '*device': 'str' } }
> 
> # @query_drive-graphs
> # Returns an array of all node graph roots
> { 'command': 'query-drive-graphs',
>   'returns': [ 'BlockGraphRoot' ] }
> 
> possibly with 'data':{'*device':'str'} to allow filtering to just a
> 1-element array based on the device name (although I'm not sure if
> providing the complexity of filtering is worth it).
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 

Re: [Qemu-devel] [RFC V3 3/7] qapi: Add skeletton of command to query a drive bs graph.

[Eric Blake]

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

On 12/05/2013 07:24 AM, Benoît Canet wrote:

>>
>> Am I correct that it will be possible to have named nodes that are not
>> currently associated with any device?  If so, how do we learn about
>> those nodes?  Would it be better to have a command that returns an array
>> of structs for all known node roots, with an optional member describing
>> which device owns that node root?  Something like:
> 
> The code have a list of all named nodes but not a list of named nodes roots.
> Also it's difficult to get the device name for a named node because the bses don't
> have any backward pointers to their parents.
> It could be done by recursing into all the blockbackend bs but it's twisted.

Still worth thinking about how to structure things so we could add it in
the future if it turns out to be useful to management, but I can
understand why you aren't providing it right away.

> 
> In fact I am wondering if we really need something to spit out the named nodes
> topology in QMP for the simple reason that the names of the nodes are given by the
> management so the management should already know the topology.

There's one case where management might not know - if libvirtd gets
restarted while in the middle of an operation that was attempting to
create a named node, then on restart and reconnection to the monitor,
libvirt would want to query to see if the node actually got created or
if the command needs to be attempted again.  I'm not a fan of write-only
interfaces - and making management responsible to track all named nodes
with no way to query if qemu actually agrees with the topology that
management thinks it has commanded feels like a write-only interface.

-- 
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] [RFC V3 3/7] qapi: Add skeletton of command to query a drive bs graph.

[Benoît Canet]

Le Thursday 05 Dec 2013 à 07:38:37 (-0700), Eric Blake a écrit :
> On 12/05/2013 07:24 AM, Benoît Canet wrote:
> 
> >>
> >> Am I correct that it will be possible to have named nodes that are not
> >> currently associated with any device?  If so, how do we learn about
> >> those nodes?  Would it be better to have a command that returns an array
> >> of structs for all known node roots, with an optional member describing
> >> which device owns that node root?  Something like:
> > 
> > The code have a list of all named nodes but not a list of named nodes roots.
> > Also it's difficult to get the device name for a named node because the bses don't
> > have any backward pointers to their parents.
> > It could be done by recursing into all the blockbackend bs but it's twisted.
> 
> Still worth thinking about how to structure things so we could add it in
> the future if it turns out to be useful to management, but I can
> understand why you aren't providing it right away.
> 
> > 
> > In fact I am wondering if we really need something to spit out the named nodes
> > topology in QMP for the simple reason that the names of the nodes are given by the
> > management so the management should already know the topology.
> 
> There's one case where management might not know - if libvirtd gets
> restarted while in the middle of an operation that was attempting to
> create a named node, then on restart and reconnection to the monitor,
> libvirt would want to query to see if the node actually got created or
> if the command needs to be attempted again.  I'm not a fan of write-only
> interfaces - and making management responsible to track all named nodes
> with no way to query if qemu actually agrees with the topology that
> management thinks it has commanded feels like a write-only interface.

Would a command returning info about a specific named node be sufficient for
libvirt checks ?
It's far less complex to implement than exposing the whole graph.
We could also provide a simple command to list the names of the named nodes.

Best regards

Benoît

> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 

Re: [Qemu-devel] [RFC V3 3/7] qapi: Add skeletton of command to query a drive bs graph.

[Eric Blake]

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

On 12/05/2013 07:43 AM, Benoît Canet wrote:

>> There's one case where management might not know - if libvirtd gets
>> restarted while in the middle of an operation that was attempting to
>> create a named node, then on restart and reconnection to the monitor,
>> libvirt would want to query to see if the node actually got created or
>> if the command needs to be attempted again.  I'm not a fan of write-only
>> interfaces - and making management responsible to track all named nodes
>> with no way to query if qemu actually agrees with the topology that
>> management thinks it has commanded feels like a write-only interface.
> 
> Would a command returning info about a specific named node be sufficient for
> libvirt checks ?
> It's far less complex to implement than exposing the whole graph.
> We could also provide a simple command to list the names of the named nodes.

Yes, both of those ideas are useful; it still means management must
track the topology between the nodes, but it at least gives management
enough control to know which set of nodes exist to confirm which
operations have occurred.

-- 
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] [RFC V3 3/7] qapi: Add skeletton of command to query a drive bs graph.

[Benoît Canet]

Le Thursday 05 Dec 2013 à 07:59:22 (-0700), Eric Blake a écrit :
> On 12/05/2013 07:43 AM, Benoît Canet wrote:
> 
> >> There's one case where management might not know - if libvirtd gets
> >> restarted while in the middle of an operation that was attempting to
> >> create a named node, then on restart and reconnection to the monitor,
> >> libvirt would want to query to see if the node actually got created or
> >> if the command needs to be attempted again.  I'm not a fan of write-only
> >> interfaces - and making management responsible to track all named nodes
> >> with no way to query if qemu actually agrees with the topology that
> >> management thinks it has commanded feels like a write-only interface.
> > 
> > Would a command returning info about a specific named node be sufficient for
> > libvirt checks ?
> > It's far less complex to implement than exposing the whole graph.
> > We could also provide a simple command to list the names of the named nodes.
> 
> Yes, both of those ideas are useful; it still means management must
> track the topology between the nodes, but it at least gives management
> enough control to know which set of nodes exist to confirm which
> operations have occurred.

I will start by implementing the named nodes name list first.
Don't worry for the other solution: my customer want crypto filters and multi
BlockDriver state throttling as a filter so I will continue to enable filter
in a progressive way.

Best regards

Benoît
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 

[Qemu-devel] [RFC V3 4/7] qmp: Allow block_passwd to manipulate bs graph nodes.

[Benoît Canet]

Signed-off-by: Benoit Canet <benoit@irqsave.net>
---
 block.c               | 33 +++++++++++++++++++++++++++++++++
 blockdev.c            | 13 +++++++++----
 hmp.c                 |  2 +-
 include/block/block.h |  3 +++
 qapi-schema.json      |  9 +++++++--
 qmp-commands.hx       |  3 ++-
 6 files changed, 55 insertions(+), 8 deletions(-)

diff --git a/block.c b/block.c
index 3a0cb30..8016ff2 100644
--- a/block.c
+++ b/block.c
@@ -3176,6 +3176,39 @@ BlockDriverState *bdrv_find_node(const char *node_name)
     return NULL;
 }
 
+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) ||
+        (!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 824e718..aab370f 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)
 {
     BlockDriverState *bs;
+    Error *local_err = NULL;
     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 6426ca6..26c48e7 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -371,6 +371,9 @@ 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_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 938f8b9..60d3bd9 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 1.8)
 #
 # @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 fba15cd..a73b08f 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

Re: [Qemu-devel] [RFC V3 4/7] qmp: Allow block_passwd to manipulate bs graph nodes.

[Eric Blake]

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

On 12/03/2013 06:26 AM, Benoît Canet wrote:
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
>  
> +BlockDriverState * bdrv_lookup_bs(bool has_device, const char * device,
> +                                  bool has_node_name, const char * node_name,

Style: no space after * (3 instances)

> +                                  Error **errp)
> +{
> +    BlockDriverState *bs = NULL;
> +
> +    if ((has_device && has_node_name) ||
> +        (!has_device && !has_node_name)) {

Could be shortened to:

if (has_device == has_node_name) {

> +        error_setg(errp, "Use either device or node-name but not both.");

We tend to avoid trailing '.' on error messages

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

Again, no space after '*'

> +++ b/include/block/block.h
> @@ -371,6 +371,9 @@ 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_lookup_bs(bool has_device, const char * device,
> +                                  bool has_node_name, const char * node_name,
> +                                  Error **errp);

And again

> +++ 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 1.8)

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

Seems like a reasonable addition; shouldn't cause any back-compat
problems (older management tools will always provide the now-optional
'device').

Is it intentional that you are not exposing this new functionality in HMP?

-- 
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] [RFC V3 4/7] qmp: Allow block_passwd to manipulate bs graph nodes.

[Benoît Canet]

Le Wednesday 04 Dec 2013 à 16:56:05 (-0700), Eric Blake a écrit :
> On 12/03/2013 06:26 AM, Benoît Canet wrote:
> > Signed-off-by: Benoit Canet <benoit@irqsave.net>
> > ---
> >  
> > +BlockDriverState * bdrv_lookup_bs(bool has_device, const char * device,
> > +                                  bool has_node_name, const char * node_name,
> 
> Style: no space after * (3 instances)
> 
> > +                                  Error **errp)
> > +{
> > +    BlockDriverState *bs = NULL;
> > +
> > +    if ((has_device && has_node_name) ||
> > +        (!has_device && !has_node_name)) {
> 
> Could be shortened to:
> 
> if (has_device == has_node_name) {
> 
> > +        error_setg(errp, "Use either device or node-name but not both.");
> 
> We tend to avoid trailing '.' on error messages
> 
> >  
> > -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)
> 
> Again, no space after '*'
> 
> > +++ b/include/block/block.h
> > @@ -371,6 +371,9 @@ 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_lookup_bs(bool has_device, const char * device,
> > +                                  bool has_node_name, const char * node_name,
> > +                                  Error **errp);
> 
> And again
> 
> > +++ 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 1.8)
> 
> 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'} }
> 
> Seems like a reasonable addition; shouldn't cause any back-compat
> problems (older management tools will always provide the now-optional
> 'device').
> 
> Is it intentional that you are not exposing this new functionality in HMP?

Yes, I don't foresee any way to print the graph in HMP so I am limiting the
changes to QMP.

Best regards

Benoît

> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 

[Qemu-devel] [RFC V3 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 aab370f..e314d62 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 60d3bd9..92a7768 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 1.8)
 #
 # @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 a73b08f..6b309a1 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

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

[Eric Blake]

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

On 12/03/2013 06:26 AM, Benoît Canet wrote:
> 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 aab370f..e314d62 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,

More style issues (I'll quit pointing them out).

> +++ 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);

Again, is the decision to hide this from HMP intentional?  I can live
with that, but it's probably worth documenting in the commit message.

>  #
> -# @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 1.8)

2.0

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

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

[Benoît Canet]

---
 block.c                   | 64 +++++++++++++++++++++++++++++++++++++++++------
 block/blkverify.c         |  2 +-
 include/block/block.h     | 16 +++++++++---
 include/block/block_int.h |  9 ++++---
 4 files changed, 75 insertions(+), 16 deletions(-)

diff --git a/block.c b/block.c
index 8016ff2..0569cb2 100644
--- a/block.c
+++ b/block.c
@@ -4945,21 +4945,69 @@ 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_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_CANT_SNAPSHOT]) {
+        if (bs == candidate) {
+             return EXT_SNAPSHOT_ALLOWED;
+        } else {
+             return EXT_SNAPSHOT_FORBIDDEN;
+        }
     }
 
-    /* external snapshots are allowed by default */
-    return EXT_SNAPSHOT_ALLOWED;
+    if (!bs->drv->authorizations[BS_FILTER_PASS_DOWN]) {
+        return EXT_SNAPSHOT_FORBIDDEN;
+    }
+
+    if (!bs->file) {
+        return EXT_SNAPSHOT_FORBIDDEN;
+    }
+
+    return bdrv_recurse_check_ext_snapshot(bs->file, candidate);
 }
 
-ExtSnapshotPerm bdrv_check_ext_snapshot_forbidden(BlockDriverState *bs)
+ExtSnapshotPerm bdrv_recurse_check_ext_snapshot(BlockDriverState *bs,
+                                                BlockDriverState *candidate)
 {
+    if (bs->drv && bs->drv->bdrv_check_ext_snapshot) {
+        return bs->drv->bdrv_check_ext_snapshot(bs, candidate);
+    }
+
+    return bdrv_generic_check_ext_snapshot(bs, candidate);
+}
+
+/* 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
+ */
+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 e755e4e..b93017c 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -313,7 +313,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 26c48e7..73c59fe 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -280,16 +280,24 @@ int bdrv_amend_options(BlockDriverState *bs_new, QEMUOptionParameter *options);
 /* external snapshots */
 
 typedef enum {
-    EXT_SNAPSHOT_ALLOWED,
     EXT_SNAPSHOT_FORBIDDEN,
+    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
  */
-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..d9704f2 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -69,10 +69,13 @@ 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];
+    /* future complex block filters will implement the following to query their
+     * children to check if snapshoting is allowed on a bs of the graph
      */
-    ExtSnapshotPerm (*bdrv_check_ext_snapshot)(BlockDriverState *bs);
+    ExtSnapshotPerm (*bdrv_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

Re: [Qemu-devel] [RFC V3 6/7] block: Create authorizations mechanism for external snapshots.

[Fam Zheng]

On 2013年12月03日 21:26, Benoît Canet wrote:
> ---
>   block.c                   | 64 +++++++++++++++++++++++++++++++++++++++++------
>   block/blkverify.c         |  2 +-
>   include/block/block.h     | 16 +++++++++---
>   include/block/block_int.h |  9 ++++---
>   4 files changed, 75 insertions(+), 16 deletions(-)
>
> diff --git a/block.c b/block.c
> index 8016ff2..0569cb2 100644
> --- a/block.c
> +++ b/block.c
> @@ -4945,21 +4945,69 @@ 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_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_CANT_SNAPSHOT]) {
> +        if (bs == candidate) {
> +             return EXT_SNAPSHOT_ALLOWED;
> +        } else {
> +             return EXT_SNAPSHOT_FORBIDDEN;
> +        }
>       }
>
> -    /* external snapshots are allowed by default */
> -    return EXT_SNAPSHOT_ALLOWED;
> +    if (!bs->drv->authorizations[BS_FILTER_PASS_DOWN]) {
> +        return EXT_SNAPSHOT_FORBIDDEN;
> +    }
> +
> +    if (!bs->file) {
> +        return EXT_SNAPSHOT_FORBIDDEN;
> +    }
> +
> +    return bdrv_recurse_check_ext_snapshot(bs->file, candidate);
>   }
>
> -ExtSnapshotPerm bdrv_check_ext_snapshot_forbidden(BlockDriverState *bs)
> +ExtSnapshotPerm bdrv_recurse_check_ext_snapshot(BlockDriverState *bs,
> +                                                BlockDriverState *candidate)
>   {
> +    if (bs->drv && bs->drv->bdrv_check_ext_snapshot) {
> +        return bs->drv->bdrv_check_ext_snapshot(bs, candidate);
> +    }
> +
> +    return bdrv_generic_check_ext_snapshot(bs, candidate);
> +}
> +
> +/* 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
> + */
> +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 e755e4e..b93017c 100644
> --- a/block/blkverify.c
> +++ b/block/blkverify.c
> @@ -313,7 +313,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 26c48e7..73c59fe 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -280,16 +280,24 @@ int bdrv_amend_options(BlockDriverState *bs_new, QEMUOptionParameter *options);
>   /* external snapshots */
>
>   typedef enum {
> -    EXT_SNAPSHOT_ALLOWED,
>       EXT_SNAPSHOT_FORBIDDEN,
> +    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
>    */
> -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..d9704f2 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -69,10 +69,13 @@ 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];

OK, I see some overlap of work here with my image fleecing patch series:

http://lists.gnu.org/archive/html/qemu-devel/2013-11/msg03694.html

The disadvantage of using a bool array is that it can't keep track of 
multiple points in code that want to forbid the same operation. So I 
think an array of int is better here.

And what do you think of my posted interface, does it works for you?

Fam

> +    /* future complex block filters will implement the following to query their
> +     * children to check if snapshoting is allowed on a bs of the graph
>        */
> -    ExtSnapshotPerm (*bdrv_check_ext_snapshot)(BlockDriverState *bs);
> +    ExtSnapshotPerm (*bdrv_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);
>

Re: [Qemu-devel] [RFC V3 6/7] block: Create authorizations mechanism for external snapshots.

[Benoît Canet]

Le Wednesday 04 Dec 2013 à 11:35:51 (+0800), Fam Zheng a écrit :
> On 2013年12月03日 21:26, Benoît Canet wrote:
> >---
> >  block.c                   | 64 +++++++++++++++++++++++++++++++++++++++++------
> >  block/blkverify.c         |  2 +-
> >  include/block/block.h     | 16 +++++++++---
> >  include/block/block_int.h |  9 ++++---
> >  4 files changed, 75 insertions(+), 16 deletions(-)
> >
> >diff --git a/block.c b/block.c
> >index 8016ff2..0569cb2 100644
> >--- a/block.c
> >+++ b/block.c
> >@@ -4945,21 +4945,69 @@ 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_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_CANT_SNAPSHOT]) {
> >+        if (bs == candidate) {
> >+             return EXT_SNAPSHOT_ALLOWED;
> >+        } else {
> >+             return EXT_SNAPSHOT_FORBIDDEN;
> >+        }
> >      }
> >
> >-    /* external snapshots are allowed by default */
> >-    return EXT_SNAPSHOT_ALLOWED;
> >+    if (!bs->drv->authorizations[BS_FILTER_PASS_DOWN]) {
> >+        return EXT_SNAPSHOT_FORBIDDEN;
> >+    }
> >+
> >+    if (!bs->file) {
> >+        return EXT_SNAPSHOT_FORBIDDEN;
> >+    }
> >+
> >+    return bdrv_recurse_check_ext_snapshot(bs->file, candidate);
> >  }
> >
> >-ExtSnapshotPerm bdrv_check_ext_snapshot_forbidden(BlockDriverState *bs)
> >+ExtSnapshotPerm bdrv_recurse_check_ext_snapshot(BlockDriverState *bs,
> >+                                                BlockDriverState *candidate)
> >  {
> >+    if (bs->drv && bs->drv->bdrv_check_ext_snapshot) {
> >+        return bs->drv->bdrv_check_ext_snapshot(bs, candidate);
> >+    }
> >+
> >+    return bdrv_generic_check_ext_snapshot(bs, candidate);
> >+}
> >+
> >+/* 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
> >+ */
> >+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 e755e4e..b93017c 100644
> >--- a/block/blkverify.c
> >+++ b/block/blkverify.c
> >@@ -313,7 +313,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 26c48e7..73c59fe 100644
> >--- a/include/block/block.h
> >+++ b/include/block/block.h
> >@@ -280,16 +280,24 @@ int bdrv_amend_options(BlockDriverState *bs_new, QEMUOptionParameter *options);
> >  /* external snapshots */
> >
> >  typedef enum {
> >-    EXT_SNAPSHOT_ALLOWED,
> >      EXT_SNAPSHOT_FORBIDDEN,
> >+    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
> >   */
> >-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..d9704f2 100644
> >--- a/include/block/block_int.h
> >+++ b/include/block/block_int.h
> >@@ -69,10 +69,13 @@ 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];
> 
> OK, I see some overlap of work here with my image fleecing patch series:
> 
> http://lists.gnu.org/archive/html/qemu-devel/2013-11/msg03694.html
> 
> The disadvantage of using a bool array is that it can't keep track
> of multiple points in code that want to forbid the same operation.
> So I think an array of int is better here.
> 
> And what do you think of my posted interface, does it works for you?

Your is in BlockDriverState mine is in BlockDriver so there are not compatible.
I think we need to get advice from other people.

Best regards

Benoît

> 
> Fam
> 
> >+    /* future complex block filters will implement the following to query their
> >+     * children to check if snapshoting is allowed on a bs of the graph
> >       */
> >-    ExtSnapshotPerm (*bdrv_check_ext_snapshot)(BlockDriverState *bs);
> >+    ExtSnapshotPerm (*bdrv_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);
> >
> 
> 

Re: [Qemu-devel] [RFC V3 6/7] block: Create authorizations mechanism for external snapshots.

[Fam Zheng]

On 2013年12月03日 21:26, Benoît Canet wrote:
> ---
>   block.c                   | 64 +++++++++++++++++++++++++++++++++++++++++------
>   block/blkverify.c         |  2 +-
>   include/block/block.h     | 16 +++++++++---
>   include/block/block_int.h |  9 ++++---
>   4 files changed, 75 insertions(+), 16 deletions(-)
>
> diff --git a/block.c b/block.c
> index 8016ff2..0569cb2 100644
> --- a/block.c
> +++ b/block.c
> @@ -4945,21 +4945,69 @@ 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_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_CANT_SNAPSHOT]) {

This double negative feels hard to read for me.

> +        if (bs == candidate) {
> +             return EXT_SNAPSHOT_ALLOWED;
> +        } else {
> +             return EXT_SNAPSHOT_FORBIDDEN;
> +        }
>       }
>
> -    /* external snapshots are allowed by default */
> -    return EXT_SNAPSHOT_ALLOWED;
> +    if (!bs->drv->authorizations[BS_FILTER_PASS_DOWN]) {
> +        return EXT_SNAPSHOT_FORBIDDEN;
> +    }
> +
> +    if (!bs->file) {
> +        return EXT_SNAPSHOT_FORBIDDEN;
> +    }
> +
> +    return bdrv_recurse_check_ext_snapshot(bs->file, candidate);
>   }
>
> -ExtSnapshotPerm bdrv_check_ext_snapshot_forbidden(BlockDriverState *bs)
> +ExtSnapshotPerm bdrv_recurse_check_ext_snapshot(BlockDriverState *bs,
> +                                                BlockDriverState *candidate)
>   {
> +    if (bs->drv && bs->drv->bdrv_check_ext_snapshot) {
> +        return bs->drv->bdrv_check_ext_snapshot(bs, candidate);
> +    }

Maybe I'm missing something, but if a driver always returns positive 
permit, despite of what candidate is (or even it's relevant to bs), then 
doesn't it also affect other devices? because...

> +
> +    return bdrv_generic_check_ext_snapshot(bs, candidate);
> +}
> +
> +/* 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
> + */
> +ExtSnapshotPerm bdrv_check_ext_snapshot(BlockDriverState *candidate)
> +{
> +    BlockDriverState *bs;
> +
> +    /* walk down the bs forest recursively */
> +    QTAILQ_FOREACH(bs, &bdrv_states, device_list) {

this iterates through all the known graph trees (device_list), instead 
of limiting to only the device that candidate belongs to.

Why not just check candidate's permission bitmap and go down from it? If 
an ancestor need to disable its descendants, it could simply set 
permission bits of its children and recurse down.

Fam

Re: [Qemu-devel] [RFC V3 6/7] block: Create authorizations mechanism for external snapshots.

[Benoît Canet]

Le Wednesday 04 Dec 2013 à 11:47:22 (+0800), Fam Zheng a écrit :
> On 2013年12月03日 21:26, Benoît Canet wrote:
> >---
> >  block.c                   | 64 +++++++++++++++++++++++++++++++++++++++++------
> >  block/blkverify.c         |  2 +-
> >  include/block/block.h     | 16 +++++++++---
> >  include/block/block_int.h |  9 ++++---
> >  4 files changed, 75 insertions(+), 16 deletions(-)
> >
> >diff --git a/block.c b/block.c
> >index 8016ff2..0569cb2 100644
> >--- a/block.c
> >+++ b/block.c
> >@@ -4945,21 +4945,69 @@ 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_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_CANT_SNAPSHOT]) {
> 
> This double negative feels hard to read for me.
> 
> >+        if (bs == candidate) {
> >+             return EXT_SNAPSHOT_ALLOWED;
> >+        } else {
> >+             return EXT_SNAPSHOT_FORBIDDEN;
> >+        }
> >      }
> >
> >-    /* external snapshots are allowed by default */
> >-    return EXT_SNAPSHOT_ALLOWED;
> >+    if (!bs->drv->authorizations[BS_FILTER_PASS_DOWN]) {
> >+        return EXT_SNAPSHOT_FORBIDDEN;
> >+    }
> >+
> >+    if (!bs->file) {
> >+        return EXT_SNAPSHOT_FORBIDDEN;
> >+    }
> >+
> >+    return bdrv_recurse_check_ext_snapshot(bs->file, candidate);
> >  }
> >
> >-ExtSnapshotPerm bdrv_check_ext_snapshot_forbidden(BlockDriverState *bs)
> >+ExtSnapshotPerm bdrv_recurse_check_ext_snapshot(BlockDriverState *bs,
> >+                                                BlockDriverState *candidate)
> >  {
> >+    if (bs->drv && bs->drv->bdrv_check_ext_snapshot) {
> >+        return bs->drv->bdrv_check_ext_snapshot(bs, candidate);
> >+    }
> 
> Maybe I'm missing something, but if a driver always returns positive
> permit, despite of what candidate is (or even it's relevant to bs),
> then doesn't it also affect other devices? because...
> 
> >+
> >+    return bdrv_generic_check_ext_snapshot(bs, candidate);
> >+}
> >+
> >+/* 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
> >+ */
> >+ExtSnapshotPerm bdrv_check_ext_snapshot(BlockDriverState *candidate)
> >+{
> >+    BlockDriverState *bs;
> >+
> >+    /* walk down the bs forest recursively */
> >+    QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> 
> this iterates through all the known graph trees (device_list),
> instead of limiting to only the device that candidate belongs to.

The recursion termination success is candidate == bs.
This make sure that the scan of the other tree of the forest will not return any
spurious success.

> 
> Why not just check candidate's permission bitmap and go down from
> it? If an ancestor need to disable its descendants, it could simply
> set permission bits of its children and recurse down.

Yes I initially though about updating permission bit of each bs.
The problem is that the graph will evolve (taking a snapshot, adding throttling
filter) and it would be a mess to constansly update the permission bits.

Best regards

Benoît

> 
> Fam

Re: [Qemu-devel] [RFC V3 6/7] block: Create authorizations mechanism for external snapshots.

[Fam Zheng]

On 2013年12月04日 13:20, Benoît Canet wrote:
> Le Wednesday 04 Dec 2013 à 11:47:22 (+0800), Fam Zheng a écrit :
>> On 2013年12月03日 21:26, Benoît Canet wrote:
>>> ---
>>>   block.c                   | 64 +++++++++++++++++++++++++++++++++++++++++------
>>>   block/blkverify.c         |  2 +-
>>>   include/block/block.h     | 16 +++++++++---
>>>   include/block/block_int.h |  9 ++++---
>>>   4 files changed, 75 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/block.c b/block.c
>>> index 8016ff2..0569cb2 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -4945,21 +4945,69 @@ 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_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_CANT_SNAPSHOT]) {
>>
>> This double negative feels hard to read for me.
>>
>>> +        if (bs == candidate) {
>>> +             return EXT_SNAPSHOT_ALLOWED;
>>> +        } else {
>>> +             return EXT_SNAPSHOT_FORBIDDEN;
>>> +        }
>>>       }
>>>
>>> -    /* external snapshots are allowed by default */
>>> -    return EXT_SNAPSHOT_ALLOWED;
>>> +    if (!bs->drv->authorizations[BS_FILTER_PASS_DOWN]) {
>>> +        return EXT_SNAPSHOT_FORBIDDEN;
>>> +    }
>>> +
>>> +    if (!bs->file) {
>>> +        return EXT_SNAPSHOT_FORBIDDEN;
>>> +    }
>>> +
>>> +    return bdrv_recurse_check_ext_snapshot(bs->file, candidate);
>>>   }
>>>
>>> -ExtSnapshotPerm bdrv_check_ext_snapshot_forbidden(BlockDriverState *bs)
>>> +ExtSnapshotPerm bdrv_recurse_check_ext_snapshot(BlockDriverState *bs,
>>> +                                                BlockDriverState *candidate)
>>>   {
>>> +    if (bs->drv && bs->drv->bdrv_check_ext_snapshot) {
>>> +        return bs->drv->bdrv_check_ext_snapshot(bs, candidate);
>>> +    }
>>
>> Maybe I'm missing something, but if a driver always returns positive
>> permit, despite of what candidate is (or even it's relevant to bs),
>> then doesn't it also affect other devices? because...
>>
>>> +
>>> +    return bdrv_generic_check_ext_snapshot(bs, candidate);
>>> +}
>>> +
>>> +/* 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
>>> + */
>>> +ExtSnapshotPerm bdrv_check_ext_snapshot(BlockDriverState *candidate)
>>> +{
>>> +    BlockDriverState *bs;
>>> +
>>> +    /* walk down the bs forest recursively */
>>> +    QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
>>
>> this iterates through all the known graph trees (device_list),
>> instead of limiting to only the device that candidate belongs to.
>
> The recursion termination success is candidate == bs.
> This make sure that the scan of the other tree of the forest will not return any
> spurious success.
>

But the "candidate == bs" check is in bdrv_generic_check_ext_snapshot, 
which gets short-circuited by driver implementation if the driver 
implements it, in bdrv_recurse_check_ext_snapshot.

So if I have an "always yes" drv->bdrv_check_ext_snapshot and it happens 
to be the first one in bdrv_states, I will allow all snapshot operations.

Fam

Re: [Qemu-devel] [RFC V3 6/7] block: Create authorizations mechanism for external snapshots.

[Benoît Canet]

Le Wednesday 04 Dec 2013 à 14:12:19 (+0800), Fam Zheng a écrit :
> On 2013年12月04日 13:20, Benoît Canet wrote:
> >Le Wednesday 04 Dec 2013 à 11:47:22 (+0800), Fam Zheng a écrit :
> >>On 2013年12月03日 21:26, Benoît Canet wrote:
> >>>---
> >>>  block.c                   | 64 +++++++++++++++++++++++++++++++++++++++++------
> >>>  block/blkverify.c         |  2 +-
> >>>  include/block/block.h     | 16 +++++++++---
> >>>  include/block/block_int.h |  9 ++++---
> >>>  4 files changed, 75 insertions(+), 16 deletions(-)
> >>>
> >>>diff --git a/block.c b/block.c
> >>>index 8016ff2..0569cb2 100644
> >>>--- a/block.c
> >>>+++ b/block.c
> >>>@@ -4945,21 +4945,69 @@ 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_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_CANT_SNAPSHOT]) {
> >>
> >>This double negative feels hard to read for me.
> >>
> >>>+        if (bs == candidate) {
> >>>+             return EXT_SNAPSHOT_ALLOWED;
> >>>+        } else {
> >>>+             return EXT_SNAPSHOT_FORBIDDEN;
> >>>+        }
> >>>      }
> >>>
> >>>-    /* external snapshots are allowed by default */
> >>>-    return EXT_SNAPSHOT_ALLOWED;
> >>>+    if (!bs->drv->authorizations[BS_FILTER_PASS_DOWN]) {
> >>>+        return EXT_SNAPSHOT_FORBIDDEN;
> >>>+    }
> >>>+
> >>>+    if (!bs->file) {
> >>>+        return EXT_SNAPSHOT_FORBIDDEN;
> >>>+    }
> >>>+
> >>>+    return bdrv_recurse_check_ext_snapshot(bs->file, candidate);
> >>>  }
> >>>
> >>>-ExtSnapshotPerm bdrv_check_ext_snapshot_forbidden(BlockDriverState *bs)
> >>>+ExtSnapshotPerm bdrv_recurse_check_ext_snapshot(BlockDriverState *bs,
> >>>+                                                BlockDriverState *candidate)
> >>>  {
> >>>+    if (bs->drv && bs->drv->bdrv_check_ext_snapshot) {
> >>>+        return bs->drv->bdrv_check_ext_snapshot(bs, candidate);
> >>>+    }
> >>
> >>Maybe I'm missing something, but if a driver always returns positive
> >>permit, despite of what candidate is (or even it's relevant to bs),
> >>then doesn't it also affect other devices? because...
> >>
> >>>+
> >>>+    return bdrv_generic_check_ext_snapshot(bs, candidate);
> >>>+}
> >>>+
> >>>+/* 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
> >>>+ */
> >>>+ExtSnapshotPerm bdrv_check_ext_snapshot(BlockDriverState *candidate)
> >>>+{
> >>>+    BlockDriverState *bs;
> >>>+
> >>>+    /* walk down the bs forest recursively */
> >>>+    QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> >>
> >>this iterates through all the known graph trees (device_list),
> >>instead of limiting to only the device that candidate belongs to.
> >
> >The recursion termination success is candidate == bs.
> >This make sure that the scan of the other tree of the forest will not return any
> >spurious success.
> >
>
> But the "candidate == bs" check is in
> bdrv_generic_check_ext_snapshot, which gets short-circuited by
> driver implementation if the driver implements it, in
> bdrv_recurse_check_ext_snapshot.
>
> So if I have an "always yes" drv->bdrv_check_ext_snapshot and it
> happens to be the first one in bdrv_states, I will allow all
> snapshot operations.
>

My bad I forgot to document the drv_>bdrv_check_ext_snapshot.
It meant to be recursive and only for twisted block filter like this one (quorum):

static ExtSnapshotPerm quorum_check_ext_snapshot(BlockDriverState *bs,
                                                 BlockDriverState *candidate)
{
    BDRVQuorumState *s = bs->opaque;
    int i;

    for (i = 0; i < s->total; i++) {
        ExtSnapshotPerm perm = bdrv_recurse_check_ext_snapshot(s->bs[i],
                                                               candidate);
        if (perm == EXT_SNAPSHOT_ALLOWED) {
            return EXT_SNAPSHOT_ALLOWED;
        }
    }

    return EXT_SNAPSHOT_FORBIDDEN;
}

Maybe the callback needs a serious rename.

Best regards

Benoît

> Fam

Re: [Qemu-devel] [RFC V3 6/7] block: Create authorizations mechanism for external snapshots.

[Fam Zheng]

On 2013年12月04日 14:34, Benoît Canet wrote:
> Le Wednesday 04 Dec 2013 à 14:12:19 (+0800), Fam Zheng a écrit :
>> On 2013年12月04日 13:20, Benoît Canet wrote:
>>> Le Wednesday 04 Dec 2013 à 11:47:22 (+0800), Fam Zheng a écrit :
>>>> On 2013年12月03日 21:26, Benoît Canet wrote:
>>>>> ---
>>>>>   block.c                   | 64 +++++++++++++++++++++++++++++++++++++++++------
>>>>>   block/blkverify.c         |  2 +-
>>>>>   include/block/block.h     | 16 +++++++++---
>>>>>   include/block/block_int.h |  9 ++++---
>>>>>   4 files changed, 75 insertions(+), 16 deletions(-)
>>>>>
>>>>> diff --git a/block.c b/block.c
>>>>> index 8016ff2..0569cb2 100644
>>>>> --- a/block.c
>>>>> +++ b/block.c
>>>>> @@ -4945,21 +4945,69 @@ 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_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_CANT_SNAPSHOT]) {
>>>>
>>>> This double negative feels hard to read for me.
>>>>
>>>>> +        if (bs == candidate) {
>>>>> +             return EXT_SNAPSHOT_ALLOWED;
>>>>> +        } else {
>>>>> +             return EXT_SNAPSHOT_FORBIDDEN;
>>>>> +        }
>>>>>       }
>>>>>
>>>>> -    /* external snapshots are allowed by default */
>>>>> -    return EXT_SNAPSHOT_ALLOWED;
>>>>> +    if (!bs->drv->authorizations[BS_FILTER_PASS_DOWN]) {
>>>>> +        return EXT_SNAPSHOT_FORBIDDEN;
>>>>> +    }
>>>>> +
>>>>> +    if (!bs->file) {
>>>>> +        return EXT_SNAPSHOT_FORBIDDEN;
>>>>> +    }
>>>>> +
>>>>> +    return bdrv_recurse_check_ext_snapshot(bs->file, candidate);
>>>>>   }
>>>>>
>>>>> -ExtSnapshotPerm bdrv_check_ext_snapshot_forbidden(BlockDriverState *bs)
>>>>> +ExtSnapshotPerm bdrv_recurse_check_ext_snapshot(BlockDriverState *bs,
>>>>> +                                                BlockDriverState *candidate)
>>>>>   {
>>>>> +    if (bs->drv && bs->drv->bdrv_check_ext_snapshot) {
>>>>> +        return bs->drv->bdrv_check_ext_snapshot(bs, candidate);
>>>>> +    }
>>>>
>>>> Maybe I'm missing something, but if a driver always returns positive
>>>> permit, despite of what candidate is (or even it's relevant to bs),
>>>> then doesn't it also affect other devices? because...
>>>>
>>>>> +
>>>>> +    return bdrv_generic_check_ext_snapshot(bs, candidate);
>>>>> +}
>>>>> +
>>>>> +/* 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
>>>>> + */
>>>>> +ExtSnapshotPerm bdrv_check_ext_snapshot(BlockDriverState *candidate)
>>>>> +{
>>>>> +    BlockDriverState *bs;
>>>>> +
>>>>> +    /* walk down the bs forest recursively */
>>>>> +    QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
>>>>
>>>> this iterates through all the known graph trees (device_list),
>>>> instead of limiting to only the device that candidate belongs to.
>>>
>>> The recursion termination success is candidate == bs.
>>> This make sure that the scan of the other tree of the forest will not return any
>>> spurious success.
>>>
>>
>> But the "candidate == bs" check is in
>> bdrv_generic_check_ext_snapshot, which gets short-circuited by
>> driver implementation if the driver implements it, in
>> bdrv_recurse_check_ext_snapshot.
>>
>> So if I have an "always yes" drv->bdrv_check_ext_snapshot and it
>> happens to be the first one in bdrv_states, I will allow all
>> snapshot operations.
>>
>
> My bad I forgot to document the drv_>bdrv_check_ext_snapshot.
> It meant to be recursive and only for twisted block filter like this one (quorum):
>
> static ExtSnapshotPerm quorum_check_ext_snapshot(BlockDriverState *bs,
>                                                   BlockDriverState *candidate)
> {
>      BDRVQuorumState *s = bs->opaque;
>      int i;
>
>      for (i = 0; i < s->total; i++) {
>          ExtSnapshotPerm perm = bdrv_recurse_check_ext_snapshot(s->bs[i],
>                                                                 candidate);
>          if (perm == EXT_SNAPSHOT_ALLOWED) {
>              return EXT_SNAPSHOT_ALLOWED;
>          }
>      }
>
>      return EXT_SNAPSHOT_FORBIDDEN;
> }
>
> Maybe the callback needs a serious rename.
>

OK, I see how it works. Default is forbidden and you iterate on all the 
devices trying to find some BDS recognizes and returns "allow". This 
positive vote is so powerful and I hope no driver will ever abuse it in 
the future. :)

But I still think if "bs" doesn't "recognize candidate" (in other words, 
they are irrelevant to each other), it should return a 3rd value like 
"EXT_SNAPSHOT_NOTCARE", which is more intuitive.

Thanks for your explanation.

Fam

Re: [Qemu-devel] [RFC V3 6/7] block: Create authorizations mechanism for external snapshots.

[Benoît Canet]

Le Wednesday 04 Dec 2013 à 15:03:42 (+0800), Fam Zheng a écrit :
> On 2013年12月04日 14:34, Benoît Canet wrote:
> >Le Wednesday 04 Dec 2013 à 14:12:19 (+0800), Fam Zheng a écrit :
> >>On 2013年12月04日 13:20, Benoît Canet wrote:
> >>>Le Wednesday 04 Dec 2013 à 11:47:22 (+0800), Fam Zheng a écrit :
> >>>>On 2013年12月03日 21:26, Benoît Canet wrote:
> >>>>>---
> >>>>>  block.c                   | 64 +++++++++++++++++++++++++++++++++++++++++------
> >>>>>  block/blkverify.c         |  2 +-
> >>>>>  include/block/block.h     | 16 +++++++++---
> >>>>>  include/block/block_int.h |  9 ++++---
> >>>>>  4 files changed, 75 insertions(+), 16 deletions(-)
> >>>>>
> >>>>>diff --git a/block.c b/block.c
> >>>>>index 8016ff2..0569cb2 100644
> >>>>>--- a/block.c
> >>>>>+++ b/block.c
> >>>>>@@ -4945,21 +4945,69 @@ 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_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_CANT_SNAPSHOT]) {
> >>>>
> >>>>This double negative feels hard to read for me.
> >>>>
> >>>>>+        if (bs == candidate) {
> >>>>>+             return EXT_SNAPSHOT_ALLOWED;
> >>>>>+        } else {
> >>>>>+             return EXT_SNAPSHOT_FORBIDDEN;
> >>>>>+        }
> >>>>>      }
> >>>>>
> >>>>>-    /* external snapshots are allowed by default */
> >>>>>-    return EXT_SNAPSHOT_ALLOWED;
> >>>>>+    if (!bs->drv->authorizations[BS_FILTER_PASS_DOWN]) {
> >>>>>+        return EXT_SNAPSHOT_FORBIDDEN;
> >>>>>+    }
> >>>>>+
> >>>>>+    if (!bs->file) {
> >>>>>+        return EXT_SNAPSHOT_FORBIDDEN;
> >>>>>+    }
> >>>>>+
> >>>>>+    return bdrv_recurse_check_ext_snapshot(bs->file, candidate);
> >>>>>  }
> >>>>>
> >>>>>-ExtSnapshotPerm bdrv_check_ext_snapshot_forbidden(BlockDriverState *bs)
> >>>>>+ExtSnapshotPerm bdrv_recurse_check_ext_snapshot(BlockDriverState *bs,
> >>>>>+                                                BlockDriverState *candidate)
> >>>>>  {
> >>>>>+    if (bs->drv && bs->drv->bdrv_check_ext_snapshot) {
> >>>>>+        return bs->drv->bdrv_check_ext_snapshot(bs, candidate);
> >>>>>+    }
> >>>>
> >>>>Maybe I'm missing something, but if a driver always returns positive
> >>>>permit, despite of what candidate is (or even it's relevant to bs),
> >>>>then doesn't it also affect other devices? because...
> >>>>
> >>>>>+
> >>>>>+    return bdrv_generic_check_ext_snapshot(bs, candidate);
> >>>>>+}
> >>>>>+
> >>>>>+/* 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
> >>>>>+ */
> >>>>>+ExtSnapshotPerm bdrv_check_ext_snapshot(BlockDriverState *candidate)
> >>>>>+{
> >>>>>+    BlockDriverState *bs;
> >>>>>+
> >>>>>+    /* walk down the bs forest recursively */
> >>>>>+    QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> >>>>
> >>>>this iterates through all the known graph trees (device_list),
> >>>>instead of limiting to only the device that candidate belongs to.
> >>>
> >>>The recursion termination success is candidate == bs.
> >>>This make sure that the scan of the other tree of the forest will not return any
> >>>spurious success.
> >>>
> >>
> >>But the "candidate == bs" check is in
> >>bdrv_generic_check_ext_snapshot, which gets short-circuited by
> >>driver implementation if the driver implements it, in
> >>bdrv_recurse_check_ext_snapshot.
> >>
> >>So if I have an "always yes" drv->bdrv_check_ext_snapshot and it
> >>happens to be the first one in bdrv_states, I will allow all
> >>snapshot operations.
> >>
> >
> >My bad I forgot to document the drv_>bdrv_check_ext_snapshot.
> >It meant to be recursive and only for twisted block filter like this one (quorum):
> >
> >static ExtSnapshotPerm quorum_check_ext_snapshot(BlockDriverState *bs,
> >                                                  BlockDriverState *candidate)
> >{
> >     BDRVQuorumState *s = bs->opaque;
> >     int i;
> >
> >     for (i = 0; i < s->total; i++) {
> >         ExtSnapshotPerm perm = bdrv_recurse_check_ext_snapshot(s->bs[i],
> >                                                                candidate);
> >         if (perm == EXT_SNAPSHOT_ALLOWED) {
> >             return EXT_SNAPSHOT_ALLOWED;
> >         }
> >     }
> >
> >     return EXT_SNAPSHOT_FORBIDDEN;
> >}
> >
> >Maybe the callback needs a serious rename.
> >
> 
> OK, I see how it works. Default is forbidden and you iterate on all
> the devices trying to find some BDS recognizes and returns "allow".
> This positive vote is so powerful and I hope no driver will ever
> abuse it in the future. :)
I will add some explanations to the code to make it clearer.

> 
> But I still think if "bs" doesn't "recognize candidate" (in other
> words, they are irrelevant to each other), it should return a 3rd
> value like "EXT_SNAPSHOT_NOTCARE", which is more intuitive.
Good idea I will do this.

> 
> Thanks for your explanation.
> 
> Fam

[Qemu-devel] [RFC V3 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       | 49 +++++++++++++++++++++++++++++++++++++++++--------
 hmp.c            |  4 +++-
 qapi-schema.json | 13 ++++++++++---
 qmp-commands.hx  | 11 ++++++++++-
 4 files changed, 64 insertions(+), 13 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index e314d62..68d4ad1 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,
-                                bool has_format, const char *format,
-                                bool has_mode, enum NewImageMode mode,
-                                Error **errp)
+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, 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 92a7768..8d3ff1a 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 1.8)
 #
 # @snapshot-file: the target of the new image. A new file will be created.
 #
+# @snapshot-node-name: the graph node name of the new image (Since 1.8)
+#
 # @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 6b309a1..853cc67 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] [RFC V3 7/7] qmp: Allow to take external snapshots on bs graphs node.

[Fam Zheng]

On 2013年12月03日 21:26, Benoît Canet wrote:
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
>   blockdev.c       | 49 +++++++++++++++++++++++++++++++++++++++++--------
>   hmp.c            |  4 +++-
>   qapi-schema.json | 13 ++++++++++---
>   qmp-commands.hx  | 11 ++++++++++-
>   4 files changed, 64 insertions(+), 13 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index e314d62..68d4ad1 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,
> -                                bool has_format, const char *format,
> -                                bool has_mode, enum NewImageMode mode,
> -                                Error **errp)
> +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,

Same question with some other function declarations and definitions in 
this series: you are adding an extra whitespace between * and pointer 
name, is it intended?

Thanks,
Fam

Re: [Qemu-devel] [RFC V3 7/7] qmp: Allow to take external snapshots on bs graphs node.

[Benoît Canet]

Le Wednesday 04 Dec 2013 à 11:51:26 (+0800), Fam Zheng a écrit :
> On 2013年12月03日 21:26, Benoît Canet wrote:
> >Signed-off-by: Benoit Canet <benoit@irqsave.net>
> >---
> >  blockdev.c       | 49 +++++++++++++++++++++++++++++++++++++++++--------
> >  hmp.c            |  4 +++-
> >  qapi-schema.json | 13 ++++++++++---
> >  qmp-commands.hx  | 11 ++++++++++-
> >  4 files changed, 64 insertions(+), 13 deletions(-)
> >
> >diff --git a/blockdev.c b/blockdev.c
> >index e314d62..68d4ad1 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,
> >-                                bool has_format, const char *format,
> >-                                bool has_mode, enum NewImageMode mode,
> >-                                Error **errp)
> >+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,
> 
> Same question with some other function declarations and definitions
> in this series: you are adding an extra whitespace between * and
> pointer name, is it intended?

No I copy/pasted qmp generated prototypes.

Best regards

Benoît

> 
> Thanks,
> Fam
> 

Re: [Qemu-devel] [RFC V3 7/7] qmp: Allow to take external snapshots on bs graphs node.

[Eric Blake]

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

On 12/03/2013 06:26 AM, Benoît Canet wrote:
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
>  blockdev.c       | 49 +++++++++++++++++++++++++++++++++++++++++--------
>  hmp.c            |  4 +++-
>  qapi-schema.json | 13 ++++++++++---
>  qmp-commands.hx  | 11 ++++++++++-
>  4 files changed, 64 insertions(+), 13 deletions(-)
> 

> 
> +    if (has_node_name && !has_snapshot_node_name) {
> +        error_setg(errp, "New snapshot node name missing");
> +        return;
> +    }

Why is it okay to omit the node name when passing a device name (which
creates an anonymous node as the new root of the device tree) but not
when passing a node name?  Are you trying to guarantee that all
anonymous nodes can be reached from a device name, and that when taking
a snapshot from a node name the new node is not necessarily tied to a
device and must therefore be named?

> -# @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 1.8)
>  #
>  # @snapshot-file: the target of the new image. A new file will be created.
>  #
> +# @snapshot-node-name: the graph node name of the new image (Since 1.8)

2.0, also mark this one #optional

> +#
>  # @format: #optional the format of the snapshot image, default is 'qcow2'.

Unrelated to this patch, but @format is another field worth turning into
an enum instead of an open-coded string.

-- 
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] [RFC V3 7/7] qmp: Allow to take external snapshots on bs graphs node.

[Benoît Canet]

Le Wednesday 04 Dec 2013 à 17:11:26 (-0700), Eric Blake a écrit :
> On 12/03/2013 06:26 AM, Benoît Canet wrote:
> > Signed-off-by: Benoit Canet <benoit@irqsave.net>
> > ---
> >  blockdev.c       | 49 +++++++++++++++++++++++++++++++++++++++++--------
> >  hmp.c            |  4 +++-
> >  qapi-schema.json | 13 ++++++++++---
> >  qmp-commands.hx  | 11 ++++++++++-
> >  4 files changed, 64 insertions(+), 13 deletions(-)
> > 
> 
> > 
> > +    if (has_node_name && !has_snapshot_node_name) {
> > +        error_setg(errp, "New snapshot node name missing");
> > +        return;
> > +    }
> 
> Why is it okay to omit the node name when passing a device name (which
> creates an anonymous node as the new root of the device tree) but not
> when passing a node name?  Are you trying to guarantee that all
> anonymous nodes can be reached from a device name, and that when taking
> a snapshot from a node name the new node is not necessarily tied to a
> device and must therefore be named?

Yes bs device living just under block backend will ever be accessible via device
whereas other bs really need a node-name to be set to be manipulated.
Also it avoid adding a new mandatory field for the device case which is good for
compatibility with previous versions.

Best regards

Benoît

> 
> > -# @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 1.8)
> >  #
> >  # @snapshot-file: the target of the new image. A new file will be created.
> >  #
> > +# @snapshot-node-name: the graph node name of the new image (Since 1.8)
> 
> 2.0, also mark this one #optional
> 
> > +#
> >  # @format: #optional the format of the snapshot image, default is 'qcow2'.
> 
> Unrelated to this patch, but @format is another field worth turning into
> an enum instead of an open-coded string.
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>