[PATCH RFC 0/8] blockdev-replace

[PATCH RFC 0/8] blockdev-replace

[Vladimir Sementsov-Ogievskiy]

Hi all!

As a continuation of "Qemu block filter insertion/removal API"
discussion, here is my proposal of blockdev-replace.

The realization allows:

- replace children of different parents: BDS, block devices, block
  exports

- automatically replace all parents of specific BDS, excluding creating
  loops (like bdrv_replace_node())

- do several replacements in one transaction

It's an untested draft, so you may go to patch 8, to look at QAPI
addition.

Vladimir Sementsov-Ogievskiy (8):
  block-backend: blk_root(): drop const specifier on return type
  block: add BlockParentClass class
  block: realize BlockParentClass for BlockDriverState
  block/export: realize BlockParentClass functionality
  qdev: improve find_device_state() to distinguish simple not found case
  qdev: realize BlockParentClass
  block: improve bdrv_replace_node_noperm()
  qapi: add blockdev-replace command

 qapi/block-core.json           |  78 ++++++++++++++++
 include/block/block-parent.h   |  32 +++++++
 include/sysemu/block-backend.h |   2 +-
 block.c                        | 158 ++++++++++++++++++++++++++++++++-
 block/block-backend.c          |   2 +-
 block/block-parent.c           |  66 ++++++++++++++
 block/export/export.c          |  44 +++++++++
 softmmu/qdev-monitor.c         |  90 +++++++++++++++----
 block/meson.build              |   1 +
 9 files changed, 453 insertions(+), 20 deletions(-)
 create mode 100644 include/block/block-parent.h
 create mode 100644 block/block-parent.c

-- 
2.29.2

[PATCH 1/8] block-backend: blk_root(): drop const specifier on return type

[Vladimir Sementsov-Ogievskiy]

We'll need get non-const child pointer for graph modifications in
further commits.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/sysemu/block-backend.h | 2 +-
 block/block-backend.c          | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 9ac5f7bbd3..be03301123 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -270,7 +270,7 @@ int coroutine_fn blk_co_copy_range(BlockBackend *blk_in, int64_t off_in,
                                    int bytes, BdrvRequestFlags read_flags,
                                    BdrvRequestFlags write_flags);
 
-const BdrvChild *blk_root(BlockBackend *blk);
+BdrvChild *blk_root(BlockBackend *blk);
 
 int blk_make_empty(BlockBackend *blk, Error **errp);
 
diff --git a/block/block-backend.c b/block/block-backend.c
index deb55c272e..7a0a1e1ae2 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -2455,7 +2455,7 @@ int coroutine_fn blk_co_copy_range(BlockBackend *blk_in, int64_t off_in,
                               bytes, read_flags, write_flags);
 }
 
-const BdrvChild *blk_root(BlockBackend *blk)
+BdrvChild *blk_root(BlockBackend *blk)
 {
     return blk->root;
 }
-- 
2.29.2

[PATCH 2/8] block: add BlockParentClass class

[Vladimir Sementsov-Ogievskiy]

Add a class that will unify block parents for blockdev-replace
functionality we are going to add.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/block/block-parent.h | 32 +++++++++++++++++
 block/block-parent.c         | 66 ++++++++++++++++++++++++++++++++++++
 block/meson.build            |  1 +
 3 files changed, 99 insertions(+)
 create mode 100644 include/block/block-parent.h
 create mode 100644 block/block-parent.c

diff --git a/include/block/block-parent.h b/include/block/block-parent.h
new file mode 100644
index 0000000000..bd9c9d91e6
--- /dev/null
+++ b/include/block/block-parent.h
@@ -0,0 +1,32 @@
+/*
+ * Block Parent class
+ *
+ * Copyright (c) 2021 Virtuozzo International GmbH.
+ *
+ * Authors:
+ *  Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef BLOCK_PARENT_H
+#define BLOCK_PARENT_H
+
+#include "block/block.h"
+
+typedef struct BlockParentClass {
+    const char *name;
+
+    int (*find_child)(const char *parent_id, const char *child_name,
+                      BlockDriverState *child_bs, BdrvChild **child,
+                      Error **errp);
+    QTAILQ_ENTRY(BlockParentClass) next;
+} BlockParentClass;
+
+void block_parent_class_register(BlockParentClass *cls);
+
+BdrvChild *block_find_child(const char *parent_id, const char *child_name,
+                            BlockDriverState *child_bs, Error **errp);
+
+#endif /* BLOCK_PARENT_H */
diff --git a/block/block-parent.c b/block/block-parent.c
new file mode 100644
index 0000000000..73b6026c42
--- /dev/null
+++ b/block/block-parent.c
@@ -0,0 +1,66 @@
+/*
+ * Block Parent class
+ *
+ * Copyright (c) 2021 Virtuozzo International GmbH.
+ *
+ * Authors:
+ *  Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "block/block-parent.h"
+#include "qapi/error.h"
+
+static QTAILQ_HEAD(, BlockParentClass) block_parent_classes =
+    QTAILQ_HEAD_INITIALIZER(block_parent_classes);
+
+void block_parent_class_register(BlockParentClass *cls)
+{
+    QTAILQ_INSERT_HEAD(&block_parent_classes, cls, next);
+}
+
+BdrvChild *block_find_child(const char *parent_id, const char *child_name,
+                            BlockDriverState *child_bs, Error **errp)
+{
+    BdrvChild *found_child = NULL;
+    BlockParentClass *found_cls = NULL, *cls;
+
+    QTAILQ_FOREACH(cls, &block_parent_classes, next) {
+        int ret;
+        BdrvChild *c;
+
+        /*
+         * Note that .find_child must fail if parent is found but doesn't have
+         * corresponding child.
+         */
+        ret = cls->find_child(parent_id, child_name, child_bs, &c, errp);
+        if (ret < 0) {
+            return NULL;
+        }
+        if (ret == 0) {
+            continue;
+        }
+
+        if (!found_child) {
+            found_cls = cls;
+            found_child = c;
+            continue;
+        }
+
+        error_setg(errp, "{%s, %s} parent-child pair is ambiguous: it match "
+                   "both %s and %s", parent_id, child_name, found_cls->name,
+                   cls->name);
+        return NULL;
+    }
+
+    if (!found_child) {
+        error_setg(errp, "{%s, %s} parent-child pair not found", parent_id,
+                   child_name);
+        return NULL;
+    }
+
+    return found_child;
+}
diff --git a/block/meson.build b/block/meson.build
index 0450914c7a..5200e0ffce 100644
--- a/block/meson.build
+++ b/block/meson.build
@@ -10,6 +10,7 @@ block_ss.add(files(
   'blkverify.c',
   'block-backend.c',
   'block-copy.c',
+  'block-parent.c',
   'commit.c',
   'copy-on-read.c',
   'preallocate.c',
-- 
2.29.2

[PATCH 3/8] block: realize BlockParentClass for BlockDriverState

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/block.c b/block.c
index e97ce0b1c8..449f933661 100644
--- a/block.c
+++ b/block.c
@@ -26,6 +26,7 @@
 #include "qemu/osdep.h"
 #include "block/trace.h"
 #include "block/block_int.h"
+#include "block/block-parent.h"
 #include "block/blockjob.h"
 #include "block/fuse.h"
 #include "block/nbd.h"
@@ -7501,6 +7502,47 @@ int bdrv_make_empty(BdrvChild *c, Error **errp)
     return 0;
 }
 
+static int bdrv_find_child(const char *parent_id, const char *child_name,
+                           BlockDriverState *child_bs, BdrvChild **child,
+                           Error **errp)
+{
+    BdrvChild *c, *found = NULL;
+    BlockDriverState *bs;
+
+    bs = bdrv_find_node(parent_id);
+    if (!bs) {
+        return 0;
+    }
+
+    QLIST_FOREACH(c, &bs->children, next) {
+        if ((!child_name || strcmp(c->name, child_name) == 0) &&
+            (!child_bs || child_bs == c->bs))
+        {
+            if (found) {
+                error_setg(errp, "node '%s' has more than one matching child",
+                           parent_id);
+                return -EINVAL;
+            } else {
+                found = c;
+            }
+        }
+    }
+
+    if (!found) {
+        error_setg(errp, "node '%s' exists but it doesn't have matching child",
+                   parent_id);
+        return -EINVAL;
+    }
+
+    *child = found;
+    return 1;
+}
+
+BlockParentClass block_parent_bdrv = {
+    .name = "block driver",
+    .find_child = bdrv_find_child,
+};
+
 /*
  * Return the child that @bs acts as an overlay for, and from which data may be
  * copied in COW or COR operations.  Usually this is the backing file.
@@ -7653,3 +7695,10 @@ BlockDriverState *bdrv_backing_chain_next(BlockDriverState *bs)
 {
     return bdrv_skip_filters(bdrv_cow_bs(bdrv_skip_filters(bs)));
 }
+
+static void block_c_init(void)
+{
+    block_parent_class_register(&block_parent_bdrv);
+}
+
+block_init(block_c_init);
-- 
2.29.2

[PATCH 4/8] block/export: realize BlockParentClass functionality

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/export/export.c | 44 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/block/export/export.c b/block/export/export.c
index 6d3b9964c8..39b45feafe 100644
--- a/block/export/export.c
+++ b/block/export/export.c
@@ -14,6 +14,7 @@
 #include "qemu/osdep.h"
 
 #include "block/block.h"
+#include "block/block-parent.h"
 #include "sysemu/block-backend.h"
 #include "sysemu/iothread.h"
 #include "block/export.h"
@@ -362,3 +363,46 @@ BlockExportInfoList *qmp_query_block_exports(Error **errp)
 
     return head;
 }
+
+static int blk_exp_find_child(const char *parent_id, const char *child_name,
+                              BlockDriverState *child_bs,
+                              BdrvChild **child, Error **errp)
+{
+    BlockExport *exp;
+
+    exp = blk_exp_find(parent_id);
+    if (exp == NULL) {
+        return 0;
+    }
+
+    if (child_name && strcmp(child_name, "root")) {
+        error_setg(errp, "Block export may have only 'root' child");
+        return -EINVAL;
+    }
+
+    if (!exp->blk || !blk_root(exp->blk)) {
+        error_setg(errp, "Export '%s' does not have a block driver child",
+                   parent_id);
+        return -EINVAL;
+    }
+
+    if (child_bs && blk_bs(exp->blk) != child_bs) {
+        error_setg(errp, "Export '%s' root child doesn't match", parent_id);
+        return -EINVAL;
+    }
+
+    *child = blk_root(exp->blk);
+    return 1;
+}
+
+BlockParentClass block_parent_export = {
+    .name = "block export",
+    .find_child = blk_exp_find_child,
+};
+
+static void export_init(void)
+{
+    block_parent_class_register(&block_parent_export);
+}
+
+block_init(export_init);
-- 
2.29.2

[PATCH 5/8] qdev: improve find_device_state() to distinguish simple not found case

[Vladimir Sementsov-Ogievskiy]

We'll need this for realizing qdev_find_child() in the next commit.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 softmmu/qdev-monitor.c | 48 +++++++++++++++++++++++++++++-------------
 1 file changed, 33 insertions(+), 15 deletions(-)

diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 721dec2d82..0117989009 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -819,7 +819,12 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp)
     object_unref(OBJECT(dev));
 }
 
-static DeviceState *find_device_state(const char *id, Error **errp)
+/*
+ * Returns: 1 when found, @dev set
+ *          0 not found, @dev and @errp untouched
+ *         <0 error, or id is ambiguous, @errp set
+ */
+static int find_device_state(const char *id, DeviceState **dev, Error **errp)
 {
     Object *obj;
 
@@ -835,17 +840,16 @@ static DeviceState *find_device_state(const char *id, Error **errp)
     }
 
     if (!obj) {
-        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
-                  "Device '%s' not found", id);
-        return NULL;
+        return 0;
     }
 
     if (!object_dynamic_cast(obj, TYPE_DEVICE)) {
         error_setg(errp, "%s is not a hotpluggable device", id);
-        return NULL;
+        return -EINVAL;
     }
 
-    return DEVICE(obj);
+    *dev = DEVICE(obj);
+    return 1;
 }
 
 void qdev_unplug(DeviceState *dev, Error **errp)
@@ -894,16 +898,25 @@ void qdev_unplug(DeviceState *dev, Error **errp)
 
 void qmp_device_del(const char *id, Error **errp)
 {
-    DeviceState *dev = find_device_state(id, errp);
-    if (dev != NULL) {
-        if (dev->pending_deleted_event) {
-            error_setg(errp, "Device %s is already in the "
-                             "process of unplug", id);
-            return;
+    int ret;
+    DeviceState *dev;
+
+    ret = find_device_state(id, &dev, errp);
+    if (ret <= 0) {
+        if (ret == 0) {
+            error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
+                      "Device '%s' not found", id);
         }
+        return;
+    }
 
-        qdev_unplug(dev, errp);
+    if (dev->pending_deleted_event) {
+        error_setg(errp, "Device %s is already in the "
+                         "process of unplug", id);
+        return;
     }
+
+    qdev_unplug(dev, errp);
 }
 
 void hmp_device_add(Monitor *mon, const QDict *qdict)
@@ -925,11 +938,16 @@ void hmp_device_del(Monitor *mon, const QDict *qdict)
 
 BlockBackend *blk_by_qdev_id(const char *id, Error **errp)
 {
+    int ret;
     DeviceState *dev;
     BlockBackend *blk;
 
-    dev = find_device_state(id, errp);
-    if (dev == NULL) {
+    ret = find_device_state(id, &dev, errp);
+    if (ret <= 0) {
+        if (ret == 0) {
+            error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
+                      "Device '%s' not found", id);
+        }
         return NULL;
     }
 
-- 
2.29.2

[PATCH 6/8] qdev: realize BlockParentClass

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 softmmu/qdev-monitor.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 0117989009..2e149aa9b8 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -18,6 +18,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "block/block-parent.h"
 #include "hw/sysbus.h"
 #include "monitor/hmp.h"
 #include "monitor/monitor.h"
@@ -1023,3 +1024,44 @@ bool qmp_command_available(const QmpCommand *cmd, Error **errp)
     }
     return true;
 }
+
+static int qdev_find_child(const char *parent_id, const char *child_name,
+                           BlockDriverState *child_bs,
+                           BdrvChild **child, Error **errp)
+{
+    int ret;
+    DeviceState *dev;
+    BlockBackend *blk;
+
+    ret = find_device_state(parent_id, &dev, errp);
+    if (ret <= 0) {
+        return ret;
+    }
+
+    blk = blk_by_dev(dev);
+    if (!blk || !blk_root(blk)) {
+        error_setg(errp, "Device '%s' does not have a block device backend",
+                   parent_id);
+        return -EINVAL;
+    }
+
+    if (child_bs && blk_bs(blk) != child_bs) {
+        error_setg(errp, "Root child of device '%s' doesn't match", parent_id);
+        return -EINVAL;
+    }
+
+    *child = blk_root(blk);
+    return 1;
+}
+
+BlockParentClass block_parent_qdev = {
+    .name = "qdev",
+    .find_child = qdev_find_child,
+};
+
+static void qdev_monitor_init(void)
+{
+    block_parent_class_register(&block_parent_qdev);
+}
+
+block_init(qdev_monitor_init);
-- 
2.29.2

[PATCH 7/8] block: improve bdrv_replace_node_noperm()

[Vladimir Sementsov-Ogievskiy]

Add optional block edge name filter and new mode: "exactly one", which
we are going to use soon.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/block.c b/block.c
index 449f933661..ae8c8c4032 100644
--- a/block.c
+++ b/block.c
@@ -4894,13 +4894,22 @@ static void bdrv_remove_filter_or_cow_child(BlockDriverState *bs,
 
 static int bdrv_replace_node_noperm(BlockDriverState *from,
                                     BlockDriverState *to,
-                                    bool auto_skip, Transaction *tran,
+                                    bool auto_skip,
+                                    const char *edge_name,
+                                    bool exactly_one,
+                                    Transaction *tran,
                                     Error **errp)
 {
     BdrvChild *c, *next;
+    bool found = false;
+
+    assert(!(auto_skip && exactly_one));
 
     QLIST_FOREACH_SAFE(c, &from->parents, next_parent, next) {
         assert(c->bs == from);
+        if (edge_name && strcmp(edge_name, c->name)) {
+            continue;
+        }
         if (!should_update_child(c, to)) {
             if (auto_skip) {
                 continue;
@@ -4914,9 +4923,19 @@ static int bdrv_replace_node_noperm(BlockDriverState *from,
                        c->name, from->node_name);
             return -EPERM;
         }
+        if (exactly_one && found) {
+            error_setg(errp, "More than one matching parents found");
+            return -EINVAL;
+        }
+        found = true;
         bdrv_replace_child_tran(c, to, tran);
     }
 
+    if (exactly_one && !found) {
+        error_setg(errp, "No one matching parents found");
+        return -EINVAL;
+    }
+
     return 0;
 }
 
@@ -4966,7 +4985,8 @@ static int bdrv_replace_node_common(BlockDriverState *from,
      * permissions based on new graph. If we fail, we'll roll-back the
      * replacement.
      */
-    ret = bdrv_replace_node_noperm(from, to, auto_skip, tran, errp);
+    ret = bdrv_replace_node_noperm(from, to, auto_skip, NULL, false,
+                                   tran, errp);
     if (ret < 0) {
         goto out;
     }
@@ -5035,7 +5055,8 @@ int bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
         goto out;
     }
 
-    ret = bdrv_replace_node_noperm(bs_top, bs_new, true, tran, errp);
+    ret = bdrv_replace_node_noperm(bs_top, bs_new, true, NULL, false,
+                                   tran, errp);
     if (ret < 0) {
         goto out;
     }
-- 
2.29.2

[PATCH 8/8] qapi: add blockdev-replace command

[Vladimir Sementsov-Ogievskiy]

Add command that can add and remove filters.

Key points of functionality:

What the command does is simply replace some BdrvChild.bs by some other
nodes. The tricky thing is selecting there BdrvChild objects.
To be able to select any kind of BdrvChild we use a generic parent_id,
which may be a node-name, or qdev id or block export id. In future we
may support block jobs.

Any kind of ambiguity leads to error. If we have both device named
device0 and block export named device0 and they both point to same BDS,
user can't replace root child of one of these parents. So, to be able
to do replacements, user should avoid duplicating names in different
parent namespaces.

So, command allows to replace any single child in the graph.

On the other hand we want to realize a kind of bdrv_replace_node(),
which works well when we want to replace all parents of some node. For
this kind of task @parents-mode argument implemented.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 qapi/block-core.json | 78 +++++++++++++++++++++++++++++++++++++++++
 block.c              | 82 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 160 insertions(+)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 675d8265eb..8059b96341 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -5433,3 +5433,81 @@
 { 'command': 'blockdev-snapshot-delete-internal-sync',
   'data': { 'device': 'str', '*id': 'str', '*name': 'str'},
   'returns': 'SnapshotInfo' }
+
+##
+# @BlockdevReplaceParentsMode:
+#
+# Alternative (to directly set @parent) way to chose parents in
+# @blockdev-replace
+#
+# @exactly-one: Exactly one parent should match a condition, otherwise
+#               @blockdev-replace fails.
+#
+# @all: All matching parents are taken into account. If replacing lead
+#       to loops in block graph, @blockdev-replace fails.
+#
+# @auto: Same as @all, but automatically skip replacing parents if it
+#        leads to loops in block graph.
+#
+# Since: 6.2
+##
+{ 'enum': 'BlockdevReplaceParentsMode',
+  'data': ['exactly-one', 'all', 'auto'] }
+
+##
+# @BlockdevReplace:
+#
+# Declaration of one replacement.
+#
+# @parent: id of parent. It may be qdev or block export or simple
+#          node-name. If id is ambiguous (for example node-name of
+#          some BDS equals to block export name), blockdev-replace
+#          fails. If not specified, blockdev-replace goes through
+#          @parents-mode scenario, see below. Note, that @parent and
+#          @parents-mode can't be specified simultaneously.
+#          If @parent is specified, only one edge is selected. If
+#          several edges match the condition, blockdev-replace fails.
+#
+# @edge: name of the child. If omitted, any child name matches.
+#
+# @child: node-name of the child. If omitted, any child matches.
+#         Must be present if @parent is not specified.
+#
+# @parents-mode: declares how to select edge (or edges) when @parent
+#                is omitted. Default is 'one'.
+#
+# Since: 6.2
+#
+# Examples:
+#
+# 1. Change root node of some device.
+#
+# Note, that @edge name is omitted, as
+# devices always has only one child. As well, no need in specifying
+# old @child.
+#
+# -> { "parent": "device0", "new-child": "some-node-name" }
+#
+# 2. Insert copy-before-write filter.
+#
+# Assume, after blockdev-add we have block-node 'source', with several
+# writing parents and one copy-before-write 'filter' parent. And we want
+# to actually insert the filter. We do:
+#
+# -> { "child": "source", "parent-mode": "auto", "new-child": "filter" }
+#
+# All parents of source would be switched to 'filter' node, except for
+# 'filter' node itself (otherwise, it will make a loop in block-graph).
+##
+{ 'struct': 'BlockdevReplace',
+  'data': { '*parent': 'str', '*edge': 'str', '*child': 'str',
+            '*parents-mode': 'BlockdevReplaceParentsMode',
+            'new-child': 'str' } }
+
+##
+# @blockdev-replace:
+#
+# Do one or several replacements transactionally.
+##
+{ 'command': 'blockdev-replace',
+  'data': { 'replacements': ['BlockdevReplace'] } }
diff --git a/block.c b/block.c
index ae8c8c4032..3bcb3152f1 100644
--- a/block.c
+++ b/block.c
@@ -41,6 +41,7 @@
 #include "qapi/qmp/qnull.h"
 #include "qapi/qmp/qstring.h"
 #include "qapi/qobject-output-visitor.h"
+#include "qapi/qapi-commands-block.h"
 #include "qapi/qapi-visit-block-core.h"
 #include "sysemu/block-backend.h"
 #include "qemu/notify.h"
@@ -7717,6 +7718,87 @@ BlockDriverState *bdrv_backing_chain_next(BlockDriverState *bs)
     return bdrv_skip_filters(bdrv_cow_bs(bdrv_skip_filters(bs)));
 }
 
+void qmp_blockdev_replace(BlockdevReplaceList *list, Error **errp)
+{
+    int ret;
+    Transaction *tran = tran_new();
+    g_autoptr(GHashTable) found = NULL;
+    g_autoptr(GSList) refresh_list = NULL;
+    g_autoptr(GSList) touched_list = NULL;
+    GSList *x;
+
+    for ( ; list; list = list->next) {
+        BlockdevReplace *repl = list->value;
+        BlockDriverState *child_bs = NULL, *new_child_bs;
+        BlockdevReplaceParentsMode mode;
+        BdrvChild *child;
+
+        if (repl->has_child) {
+            child_bs = bdrv_find_node(repl->child);
+            if (!child_bs) {
+                error_setg(errp, "Node '%s' not found", repl->child);
+                goto fail;
+            }
+        }
+
+        new_child_bs = bdrv_find_node(repl->new_child);
+        if (!new_child_bs) {
+            error_setg(errp, "Node '%s' not found", repl->new_child);
+            goto fail;
+        }
+
+        if (repl->has_parent) {
+            if (repl->has_parents_mode) {
+                error_setg(errp, "parent and parents-mode field must "
+                           "not be set simultaneously.");
+                goto fail;
+            }
+
+            child = block_find_child(repl->parent, repl->edge, child_bs, errp);
+            if (!child) {
+                goto fail;
+            }
+
+            touched_list = g_slist_prepend(touched_list, child->bs);
+            touched_list = g_slist_prepend(touched_list, new_child_bs);
+            bdrv_replace_child_tran(child, new_child_bs, tran);
+            continue;
+        }
+
+        if (!repl->has_child) {
+            error_setg(errp, "At least one of parent and child fields "
+                       "should be specified.");
+            goto fail;
+        }
+
+        mode = repl->has_parents_mode ? repl->parents_mode :
+            BLOCKDEV_REPLACE_PARENTS_MODE_EXACTLY_ONE;
+
+        touched_list = g_slist_prepend(touched_list, child_bs);
+        touched_list = g_slist_prepend(touched_list, new_child_bs);
+        ret = bdrv_replace_node_noperm(child_bs, new_child_bs,
+            mode == BLOCKDEV_REPLACE_PARENTS_MODE_AUTO,
+            repl->edge,
+            mode == BLOCKDEV_REPLACE_PARENTS_MODE_EXACTLY_ONE,
+            tran, errp);
+        if (ret < 0) {
+            goto fail;
+        }
+    }
+
+    for (x = touched_list; x; x = x->next) {
+        refresh_list = bdrv_topological_dfs(refresh_list, found, x->data);
+    }
+
+    ret = bdrv_list_refresh_perms(refresh_list, NULL, tran, errp);
+
+    tran_finalize(tran, ret);
+    return;
+
+fail:
+    tran_abort(tran);
+}
+
 static void block_c_init(void)
 {
     block_parent_class_register(&block_parent_bdrv);
-- 
2.29.2

Re: [PATCH RFC 0/8] blockdev-replace

[Vladimir Sementsov-Ogievskiy]

ping

02.08.2021 21:54, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
> 
> As a continuation of "Qemu block filter insertion/removal API"
> discussion, here is my proposal of blockdev-replace.
> 
> The realization allows:
> 
> - replace children of different parents: BDS, block devices, block
>    exports
> 
> - automatically replace all parents of specific BDS, excluding creating
>    loops (like bdrv_replace_node())
> 
> - do several replacements in one transaction
> 
> It's an untested draft, so you may go to patch 8, to look at QAPI
> addition.
> 
> Vladimir Sementsov-Ogievskiy (8):
>    block-backend: blk_root(): drop const specifier on return type
>    block: add BlockParentClass class
>    block: realize BlockParentClass for BlockDriverState
>    block/export: realize BlockParentClass functionality
>    qdev: improve find_device_state() to distinguish simple not found case
>    qdev: realize BlockParentClass
>    block: improve bdrv_replace_node_noperm()
>    qapi: add blockdev-replace command
> 
>   qapi/block-core.json           |  78 ++++++++++++++++
>   include/block/block-parent.h   |  32 +++++++
>   include/sysemu/block-backend.h |   2 +-
>   block.c                        | 158 ++++++++++++++++++++++++++++++++-
>   block/block-backend.c          |   2 +-
>   block/block-parent.c           |  66 ++++++++++++++
>   block/export/export.c          |  44 +++++++++
>   softmmu/qdev-monitor.c         |  90 +++++++++++++++----
>   block/meson.build              |   1 +
>   9 files changed, 453 insertions(+), 20 deletions(-)
>   create mode 100644 include/block/block-parent.h
>   create mode 100644 block/block-parent.c
> 


-- 
Best regards,
Vladimir

Re: [PATCH 2/8] block: add BlockParentClass class

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> Add a class that will unify block parents for blockdev-replace
> functionality we are going to add.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/block/block-parent.h | 32 +++++++++++++++++
>  block/block-parent.c         | 66 ++++++++++++++++++++++++++++++++++++
>  block/meson.build            |  1 +
>  3 files changed, 99 insertions(+)
>  create mode 100644 include/block/block-parent.h
>  create mode 100644 block/block-parent.c
>
> diff --git a/include/block/block-parent.h b/include/block/block-parent.h
> new file mode 100644
> index 0000000000..bd9c9d91e6
> --- /dev/null
> +++ b/include/block/block-parent.h
> @@ -0,0 +1,32 @@
> +/*
> + * Block Parent class
> + *
> + * Copyright (c) 2021 Virtuozzo International GmbH.
> + *
> + * Authors:
> + *  Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#ifndef BLOCK_PARENT_H
> +#define BLOCK_PARENT_H
> +
> +#include "block/block.h"
> +
> +typedef struct BlockParentClass {
> +    const char *name;
> +
> +    int (*find_child)(const char *parent_id, const char *child_name,
> +                      BlockDriverState *child_bs, BdrvChild **child,
> +                      Error **errp);

Callbacks should come with a contract.

> +    QTAILQ_ENTRY(BlockParentClass) next;
> +} BlockParentClass;
> +
> +void block_parent_class_register(BlockParentClass *cls);
> +
> +BdrvChild *block_find_child(const char *parent_id, const char *child_name,
> +                            BlockDriverState *child_bs, Error **errp);
> +
> +#endif /* BLOCK_PARENT_H */
> diff --git a/block/block-parent.c b/block/block-parent.c
> new file mode 100644
> index 0000000000..73b6026c42
> --- /dev/null
> +++ b/block/block-parent.c
> @@ -0,0 +1,66 @@
> +/*
> + * Block Parent class
> + *
> + * Copyright (c) 2021 Virtuozzo International GmbH.
> + *
> + * Authors:
> + *  Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "block/block-parent.h"
> +#include "qapi/error.h"
> +
> +static QTAILQ_HEAD(, BlockParentClass) block_parent_classes =
> +    QTAILQ_HEAD_INITIALIZER(block_parent_classes);
> +
> +void block_parent_class_register(BlockParentClass *cls)
> +{
> +    QTAILQ_INSERT_HEAD(&block_parent_classes, cls, next);
> +}
> +
> +BdrvChild *block_find_child(const char *parent_id, const char *child_name,
> +                            BlockDriverState *child_bs, Error **errp)
> +{
> +    BdrvChild *found_child = NULL;
> +    BlockParentClass *found_cls = NULL, *cls;
> +
> +    QTAILQ_FOREACH(cls, &block_parent_classes, next) {
> +        int ret;
> +        BdrvChild *c;
> +
> +        /*
> +         * Note that .find_child must fail if parent is found but doesn't have
> +         * corresponding child.
> +         */
> +        ret = cls->find_child(parent_id, child_name, child_bs, &c, errp);
> +        if (ret < 0) {
> +            return NULL;
> +        }
> +        if (ret == 0) {
> +            continue;
> +        }
> +
> +        if (!found_child) {
> +            found_cls = cls;
> +            found_child = c;
> +            continue;
> +        }
> +
> +        error_setg(errp, "{%s, %s} parent-child pair is ambiguous: it match "
> +                   "both %s and %s", parent_id, child_name, found_cls->name,
> +                   cls->name);
> +        return NULL;
> +    }
> +
> +    if (!found_child) {
> +        error_setg(errp, "{%s, %s} parent-child pair not found", parent_id,
> +                   child_name);
> +        return NULL;
> +    }
> +
> +    return found_child;
> +}

Especially when the callback can return NULL with and without setting an
error!

> diff --git a/block/meson.build b/block/meson.build
> index 0450914c7a..5200e0ffce 100644
> --- a/block/meson.build
> +++ b/block/meson.build
> @@ -10,6 +10,7 @@ block_ss.add(files(
>    'blkverify.c',
>    'block-backend.c',
>    'block-copy.c',
> +  'block-parent.c',
>    'commit.c',
>    'copy-on-read.c',
>    'preallocate.c',

Re: [PATCH 2/8] block: add BlockParentClass class

[Vladimir Sementsov-Ogievskiy]

16.09.2021 11:34, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> 
>> Add a class that will unify block parents for blockdev-replace
>> functionality we are going to add.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   include/block/block-parent.h | 32 +++++++++++++++++
>>   block/block-parent.c         | 66 ++++++++++++++++++++++++++++++++++++
>>   block/meson.build            |  1 +
>>   3 files changed, 99 insertions(+)
>>   create mode 100644 include/block/block-parent.h
>>   create mode 100644 block/block-parent.c
>>
>> diff --git a/include/block/block-parent.h b/include/block/block-parent.h
>> new file mode 100644
>> index 0000000000..bd9c9d91e6
>> --- /dev/null
>> +++ b/include/block/block-parent.h
>> @@ -0,0 +1,32 @@
>> +/*
>> + * Block Parent class
>> + *
>> + * Copyright (c) 2021 Virtuozzo International GmbH.
>> + *
>> + * Authors:
>> + *  Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + */
>> +
>> +#ifndef BLOCK_PARENT_H
>> +#define BLOCK_PARENT_H
>> +
>> +#include "block/block.h"
>> +
>> +typedef struct BlockParentClass {
>> +    const char *name;
>> +
>> +    int (*find_child)(const char *parent_id, const char *child_name,
>> +                      BlockDriverState *child_bs, BdrvChild **child,
>> +                      Error **errp);
> 
> Callbacks should come with a contract.

will add

> 
>> +    QTAILQ_ENTRY(BlockParentClass) next;
>> +} BlockParentClass;
>> +
>> +void block_parent_class_register(BlockParentClass *cls);
>> +
>> +BdrvChild *block_find_child(const char *parent_id, const char *child_name,
>> +                            BlockDriverState *child_bs, Error **errp);
>> +
>> +#endif /* BLOCK_PARENT_H */
>> diff --git a/block/block-parent.c b/block/block-parent.c
>> new file mode 100644
>> index 0000000000..73b6026c42
>> --- /dev/null
>> +++ b/block/block-parent.c
>> @@ -0,0 +1,66 @@
>> +/*
>> + * Block Parent class
>> + *
>> + * Copyright (c) 2021 Virtuozzo International GmbH.
>> + *
>> + * Authors:
>> + *  Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "block/block-parent.h"
>> +#include "qapi/error.h"
>> +
>> +static QTAILQ_HEAD(, BlockParentClass) block_parent_classes =
>> +    QTAILQ_HEAD_INITIALIZER(block_parent_classes);
>> +
>> +void block_parent_class_register(BlockParentClass *cls)
>> +{
>> +    QTAILQ_INSERT_HEAD(&block_parent_classes, cls, next);
>> +}
>> +
>> +BdrvChild *block_find_child(const char *parent_id, const char *child_name,
>> +                            BlockDriverState *child_bs, Error **errp)
>> +{
>> +    BdrvChild *found_child = NULL;
>> +    BlockParentClass *found_cls = NULL, *cls;
>> +
>> +    QTAILQ_FOREACH(cls, &block_parent_classes, next) {
>> +        int ret;
>> +        BdrvChild *c;
>> +
>> +        /*
>> +         * Note that .find_child must fail if parent is found but doesn't have
>> +         * corresponding child.
>> +         */
>> +        ret = cls->find_child(parent_id, child_name, child_bs, &c, errp);
>> +        if (ret < 0) {
>> +            return NULL;
>> +        }
>> +        if (ret == 0) {
>> +            continue;
>> +        }
>> +
>> +        if (!found_child) {
>> +            found_cls = cls;
>> +            found_child = c;
>> +            continue;
>> +        }
>> +
>> +        error_setg(errp, "{%s, %s} parent-child pair is ambiguous: it match "
>> +                   "both %s and %s", parent_id, child_name, found_cls->name,
>> +                   cls->name);
>> +        return NULL;
>> +    }
>> +
>> +    if (!found_child) {
>> +        error_setg(errp, "{%s, %s} parent-child pair not found", parent_id,
>> +                   child_name);
>> +        return NULL;
>> +    }
>> +
>> +    return found_child;
>> +}
> 
> Especially when the callback can return NULL with and without setting an
> error!

Hmm. Callback returns int.

And this block_find_child() function return NULL only together with errp set.

> 
>> diff --git a/block/meson.build b/block/meson.build
>> index 0450914c7a..5200e0ffce 100644
>> --- a/block/meson.build
>> +++ b/block/meson.build
>> @@ -10,6 +10,7 @@ block_ss.add(files(
>>     'blkverify.c',
>>     'block-backend.c',
>>     'block-copy.c',
>> +  'block-parent.c',
>>     'commit.c',
>>     'copy-on-read.c',
>>     'preallocate.c',
> 


-- 
Best regards,
Vladimir

Re: [PATCH 5/8] qdev: improve find_device_state() to distinguish simple not found case

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> We'll need this for realizing qdev_find_child() in the next commit.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  softmmu/qdev-monitor.c | 48 +++++++++++++++++++++++++++++-------------
>  1 file changed, 33 insertions(+), 15 deletions(-)
>
> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
> index 721dec2d82..0117989009 100644
> --- a/softmmu/qdev-monitor.c
> +++ b/softmmu/qdev-monitor.c
> @@ -819,7 +819,12 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp)
>      object_unref(OBJECT(dev));
>  }
>  
> -static DeviceState *find_device_state(const char *id, Error **errp)
> +/*
> + * Returns: 1 when found, @dev set
> + *          0 not found, @dev and @errp untouched
> + *         <0 error, or id is ambiguous, @errp set
> + */
> +static int find_device_state(const char *id, DeviceState **dev, Error **errp)
>  {
>      Object *obj;
>  
> @@ -835,17 +840,16 @@ static DeviceState *find_device_state(const char *id, Error **errp)
>      }
>  
>      if (!obj) {
> -        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
> -                  "Device '%s' not found", id);
> -        return NULL;
> +        return 0;
>      }
>  
>      if (!object_dynamic_cast(obj, TYPE_DEVICE)) {
>          error_setg(errp, "%s is not a hotpluggable device", id);
> -        return NULL;
> +        return -EINVAL;
>      }
>  
> -    return DEVICE(obj);
> +    *dev = DEVICE(obj);
> +    return 1;
>  }
>  
>  void qdev_unplug(DeviceState *dev, Error **errp)
> @@ -894,16 +898,25 @@ void qdev_unplug(DeviceState *dev, Error **errp)
>  
>  void qmp_device_del(const char *id, Error **errp)
>  {
> -    DeviceState *dev = find_device_state(id, errp);
> -    if (dev != NULL) {
> -        if (dev->pending_deleted_event) {
> -            error_setg(errp, "Device %s is already in the "
> -                             "process of unplug", id);
> -            return;
> +    int ret;
> +    DeviceState *dev;
> +
> +    ret = find_device_state(id, &dev, errp);
> +    if (ret <= 0) {
> +        if (ret == 0) {
> +            error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
> +                      "Device '%s' not found", id);
>          }
> +        return;
> +    }
>  
> -        qdev_unplug(dev, errp);
> +    if (dev->pending_deleted_event) {
> +        error_setg(errp, "Device %s is already in the "
> +                         "process of unplug", id);
> +        return;
>      }
> +
> +    qdev_unplug(dev, errp);
>  }
>  
>  void hmp_device_add(Monitor *mon, const QDict *qdict)
> @@ -925,11 +938,16 @@ void hmp_device_del(Monitor *mon, const QDict *qdict)
>  
>  BlockBackend *blk_by_qdev_id(const char *id, Error **errp)
>  {
> +    int ret;
>      DeviceState *dev;
>      BlockBackend *blk;
>  
> -    dev = find_device_state(id, errp);
> -    if (dev == NULL) {
> +    ret = find_device_state(id, &dev, errp);
> +    if (ret <= 0) {
> +        if (ret == 0) {
> +            error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
> +                      "Device '%s' not found", id);
> +        }
>          return NULL;
>      }

Awkward.

Before, find_device_state() either finds something (and returns it) or
doesn't (and sets @errp).

Afterward, it can fail to find in two ways, and only one of it sets
@errp.  The existing callers laboriously fuse the two back together.
The next commit adds a caller that doesn't.

Failure modes that need to be handled differently are often the result
of a function doing too much.  Let's have a closer look at this one
before the patch:

    static DeviceState *find_device_state(const char *id, Error **errp)
    {
        Object *obj;

        if (id[0] == '/') {
            obj = object_resolve_path(id, NULL);

This interprets @id as a QOM path, and tries to resolve it.

On failure, @obj becomes NULL.  On success, it points to an object of
arbitrary type.

        } else {
            char *root_path = object_get_canonical_path(qdev_get_peripheral());
            char *path = g_strdup_printf("%s/%s", root_path, id);

            g_free(root_path);
            obj = object_resolve_path_type(path, TYPE_DEVICE, NULL);
            g_free(path);

This interprets @id as qdev ID, maps it to a QOM path, and tries to
resolve it to a TYPE_DEVICE.  Fails when the path doesn't resolve, and
when it resolves to something that isn't a TYPE_DEVICE.  The latter
can't happen as long as we put only devices under /machine/peripheral/.

On failure, @obj becomes NULL.  On success, it points to a TYPE_DEVICE
object.

        }

        if (!obj) {
            error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
                      "Device '%s' not found", id);
            return NULL;
        }

        if (!object_dynamic_cast(obj, TYPE_DEVICE)) {
            error_setg(errp, "%s is not a hotpluggable device", id);
            return NULL;
        }

Unclean.

If we somehow ended up with a non-device /machine/peripheral/foo, then
find_device_state("foo", errp) would fail the first way, but
find_device_state("/machine/peripheral/foo", errp) would fail the second
way.  They should fail the exact same way instead.

        return DEVICE(obj);
    }

Better:

    static DeviceState *find_device_state(const char *id, Error **errp)
    {
        Object *obj;
        DeviceState *dev;

        if (id[0] == '/') {
            obj = object_resolve_path(id, NULL);
        } else {
            obj = object_resolve_path_component(qdev_get_peripheral(), id);
        }

        if (!obj) {
            error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
                      "Device '%s' not found", id);
            return NULL;
        }

        dev = (DeviceState *)object_dynamic_cast(obj, TYPE_DEVICE);
        if (!dev) {
            error_setg(errp, "%s is not a hotpluggable device", id);
            return NULL;
        }

        return dev;
    }

I'll post this as a cleanup patch.

Note that this function does two things, one after the other, namely
1. resolve a "qdev ID or qom path" string, and 2. convert to
TYPE_DEVICE, with error checking.

Factor out the core of 1. into its own helper resolve_id_or_qom_path(),
and the next commit can do something like

    obj = resolve_id_or_qom_path(parent_id);
    if (!obj) {
        return 0;
    }

    dev = object_dynamic_cast(obj, TYPE_DEVICE);
    if (!dev) {
        error_setg(errp, ...);
        return -EINVAL;
    }

Re: [PATCH 5/8] qdev: improve find_device_state() to distinguish simple not found case

[Vladimir Sementsov-Ogievskiy]

16.09.2021 13:48, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> 
>> We'll need this for realizing qdev_find_child() in the next commit.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   softmmu/qdev-monitor.c | 48 +++++++++++++++++++++++++++++-------------
>>   1 file changed, 33 insertions(+), 15 deletions(-)
>>
>> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
>> index 721dec2d82..0117989009 100644
>> --- a/softmmu/qdev-monitor.c
>> +++ b/softmmu/qdev-monitor.c
>> @@ -819,7 +819,12 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp)
>>       object_unref(OBJECT(dev));
>>   }
>>   
>> -static DeviceState *find_device_state(const char *id, Error **errp)
>> +/*
>> + * Returns: 1 when found, @dev set
>> + *          0 not found, @dev and @errp untouched
>> + *         <0 error, or id is ambiguous, @errp set
>> + */
>> +static int find_device_state(const char *id, DeviceState **dev, Error **errp)
>>   {
>>       Object *obj;
>>   
>> @@ -835,17 +840,16 @@ static DeviceState *find_device_state(const char *id, Error **errp)
>>       }
>>   
>>       if (!obj) {
>> -        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
>> -                  "Device '%s' not found", id);
>> -        return NULL;
>> +        return 0;
>>       }
>>   
>>       if (!object_dynamic_cast(obj, TYPE_DEVICE)) {
>>           error_setg(errp, "%s is not a hotpluggable device", id);
>> -        return NULL;
>> +        return -EINVAL;
>>       }
>>   
>> -    return DEVICE(obj);
>> +    *dev = DEVICE(obj);
>> +    return 1;
>>   }
>>   
>>   void qdev_unplug(DeviceState *dev, Error **errp)
>> @@ -894,16 +898,25 @@ void qdev_unplug(DeviceState *dev, Error **errp)
>>   
>>   void qmp_device_del(const char *id, Error **errp)
>>   {
>> -    DeviceState *dev = find_device_state(id, errp);
>> -    if (dev != NULL) {
>> -        if (dev->pending_deleted_event) {
>> -            error_setg(errp, "Device %s is already in the "
>> -                             "process of unplug", id);
>> -            return;
>> +    int ret;
>> +    DeviceState *dev;
>> +
>> +    ret = find_device_state(id, &dev, errp);
>> +    if (ret <= 0) {
>> +        if (ret == 0) {
>> +            error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
>> +                      "Device '%s' not found", id);
>>           }
>> +        return;
>> +    }
>>   
>> -        qdev_unplug(dev, errp);
>> +    if (dev->pending_deleted_event) {
>> +        error_setg(errp, "Device %s is already in the "
>> +                         "process of unplug", id);
>> +        return;
>>       }
>> +
>> +    qdev_unplug(dev, errp);
>>   }
>>   
>>   void hmp_device_add(Monitor *mon, const QDict *qdict)
>> @@ -925,11 +938,16 @@ void hmp_device_del(Monitor *mon, const QDict *qdict)
>>   
>>   BlockBackend *blk_by_qdev_id(const char *id, Error **errp)
>>   {
>> +    int ret;
>>       DeviceState *dev;
>>       BlockBackend *blk;
>>   
>> -    dev = find_device_state(id, errp);
>> -    if (dev == NULL) {
>> +    ret = find_device_state(id, &dev, errp);
>> +    if (ret <= 0) {
>> +        if (ret == 0) {
>> +            error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
>> +                      "Device '%s' not found", id);
>> +        }
>>           return NULL;
>>       }
> 
> Awkward.
> 
> Before, find_device_state() either finds something (and returns it) or
> doesn't (and sets @errp).
> 
> Afterward, it can fail to find in two ways, and only one of it sets
> @errp.  The existing callers laboriously fuse the two back together.
> The next commit adds a caller that doesn't.
> 
> Failure modes that need to be handled differently are often the result
> of a function doing too much.  Let's have a closer look at this one
> before the patch:
> 
>      static DeviceState *find_device_state(const char *id, Error **errp)
>      {
>          Object *obj;
> 
>          if (id[0] == '/') {
>              obj = object_resolve_path(id, NULL);
> 
> This interprets @id as a QOM path, and tries to resolve it.
> 
> On failure, @obj becomes NULL.  On success, it points to an object of
> arbitrary type.
> 
>          } else {
>              char *root_path = object_get_canonical_path(qdev_get_peripheral());
>              char *path = g_strdup_printf("%s/%s", root_path, id);
> 
>              g_free(root_path);
>              obj = object_resolve_path_type(path, TYPE_DEVICE, NULL);
>              g_free(path);
> 
> This interprets @id as qdev ID, maps it to a QOM path, and tries to
> resolve it to a TYPE_DEVICE.  Fails when the path doesn't resolve, and
> when it resolves to something that isn't a TYPE_DEVICE.  The latter
> can't happen as long as we put only devices under /machine/peripheral/.
> 
> On failure, @obj becomes NULL.  On success, it points to a TYPE_DEVICE
> object.
> 
>          }
> 
>          if (!obj) {
>              error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
>                        "Device '%s' not found", id);
>              return NULL;
>          }
> 
>          if (!object_dynamic_cast(obj, TYPE_DEVICE)) {
>              error_setg(errp, "%s is not a hotpluggable device", id);
>              return NULL;
>          }
> 
> Unclean.
> 
> If we somehow ended up with a non-device /machine/peripheral/foo, then
> find_device_state("foo", errp) would fail the first way, but
> find_device_state("/machine/peripheral/foo", errp) would fail the second
> way.  They should fail the exact same way instead.
> 
>          return DEVICE(obj);
>      }
> 
> Better:
> 
>      static DeviceState *find_device_state(const char *id, Error **errp)
>      {
>          Object *obj;
>          DeviceState *dev;
> 
>          if (id[0] == '/') {
>              obj = object_resolve_path(id, NULL);
>          } else {
>              obj = object_resolve_path_component(qdev_get_peripheral(), id);
>          }
> 
>          if (!obj) {
>              error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
>                        "Device '%s' not found", id);
>              return NULL;
>          }
> 
>          dev = (DeviceState *)object_dynamic_cast(obj, TYPE_DEVICE);
>          if (!dev) {
>              error_setg(errp, "%s is not a hotpluggable device", id);
>              return NULL;
>          }
> 
>          return dev;
>      }

Looks simpler)

> 
> I'll post this as a cleanup patch.
> 
> Note that this function does two things, one after the other, namely
> 1. resolve a "qdev ID or qom path" string, and 2. convert to
> TYPE_DEVICE, with error checking.
> 
> Factor out the core of 1. into its own helper resolve_id_or_qom_path(),
> and the next commit can do something like
> 
>      obj = resolve_id_or_qom_path(parent_id);
>      if (!obj) {
>          return 0;
>      }
> 
>      dev = object_dynamic_cast(obj, TYPE_DEVICE);
>      if (!dev) {
>          error_setg(errp, ...);
>          return -EINVAL;
>      }
> 

Thanks for working that out, I'll go this way in v2

-- 
Best regards,
Vladimir

Re: [PATCH 2/8] block: add BlockParentClass class

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> Add a class that will unify block parents for blockdev-replace
> functionality we are going to add.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/block/block-parent.h | 32 +++++++++++++++++
>  block/block-parent.c         | 66 ++++++++++++++++++++++++++++++++++++
>  block/meson.build            |  1 +
>  3 files changed, 99 insertions(+)
>  create mode 100644 include/block/block-parent.h
>  create mode 100644 block/block-parent.c
>
> diff --git a/include/block/block-parent.h b/include/block/block-parent.h
> new file mode 100644
> index 0000000000..bd9c9d91e6
> --- /dev/null
> +++ b/include/block/block-parent.h
> @@ -0,0 +1,32 @@
> +/*
> + * Block Parent class
> + *
> + * Copyright (c) 2021 Virtuozzo International GmbH.
> + *
> + * Authors:
> + *  Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#ifndef BLOCK_PARENT_H
> +#define BLOCK_PARENT_H
> +
> +#include "block/block.h"
> +
> +typedef struct BlockParentClass {
> +    const char *name;
> +
> +    int (*find_child)(const char *parent_id, const char *child_name,
> +                      BlockDriverState *child_bs, BdrvChild **child,
> +                      Error **errp);
> +    QTAILQ_ENTRY(BlockParentClass) next;
> +} BlockParentClass;
> +
> +void block_parent_class_register(BlockParentClass *cls);
> +
> +BdrvChild *block_find_child(const char *parent_id, const char *child_name,
> +                            BlockDriverState *child_bs, Error **errp);
> +
> +#endif /* BLOCK_PARENT_H */
> diff --git a/block/block-parent.c b/block/block-parent.c
> new file mode 100644
> index 0000000000..73b6026c42
> --- /dev/null
> +++ b/block/block-parent.c
> @@ -0,0 +1,66 @@
> +/*
> + * Block Parent class
> + *
> + * Copyright (c) 2021 Virtuozzo International GmbH.
> + *
> + * Authors:
> + *  Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "block/block-parent.h"
> +#include "qapi/error.h"
> +
> +static QTAILQ_HEAD(, BlockParentClass) block_parent_classes =
> +    QTAILQ_HEAD_INITIALIZER(block_parent_classes);
> +
> +void block_parent_class_register(BlockParentClass *cls)
> +{
> +    QTAILQ_INSERT_HEAD(&block_parent_classes, cls, next);
> +}
> +
> +BdrvChild *block_find_child(const char *parent_id, const char *child_name,
> +                            BlockDriverState *child_bs, Error **errp)
> +{
> +    BdrvChild *found_child = NULL;
> +    BlockParentClass *found_cls = NULL, *cls;
> +
> +    QTAILQ_FOREACH(cls, &block_parent_classes, next) {
> +        int ret;
> +        BdrvChild *c;
> +
> +        /*
> +         * Note that .find_child must fail if parent is found but doesn't have
> +         * corresponding child.
> +         */
> +        ret = cls->find_child(parent_id, child_name, child_bs, &c, errp);
> +        if (ret < 0) {
> +            return NULL;

Interesting: when one method fails, the entire function fails, even when
other methods succeed.  The contract should probably spell this out.

> +        }
> +        if (ret == 0) {
> +            continue;
> +        }
> +
> +        if (!found_child) {
> +            found_cls = cls;
> +            found_child = c;
> +            continue;
> +        }
> +
> +        error_setg(errp, "{%s, %s} parent-child pair is ambiguous: it match "
> +                   "both %s and %s", parent_id, child_name, found_cls->name,
> +                   cls->name);
> +        return NULL;
> +    }

Style recommendation: if very much prefer

           if bad
               error out
           normal

over

           if ok
               normal
           bad

In this case:

           if (found_child) {
               error_setg(...);
               return 0;
           }

           found_cls = cls;
           found_child = c;
       }

> +
> +    if (!found_child) {
> +        error_setg(errp, "{%s, %s} parent-child pair not found", parent_id,
> +                   child_name);
> +        return NULL;
> +    }
> +
> +    return found_child;
> +}
> diff --git a/block/meson.build b/block/meson.build
> index 0450914c7a..5200e0ffce 100644
> --- a/block/meson.build
> +++ b/block/meson.build
> @@ -10,6 +10,7 @@ block_ss.add(files(
>    'blkverify.c',
>    'block-backend.c',
>    'block-copy.c',
> +  'block-parent.c',
>    'commit.c',
>    'copy-on-read.c',
>    'preallocate.c',

Re: [PATCH 6/8] qdev: realize BlockParentClass

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  softmmu/qdev-monitor.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
>
> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
> index 0117989009..2e149aa9b8 100644
> --- a/softmmu/qdev-monitor.c
> +++ b/softmmu/qdev-monitor.c
> @@ -18,6 +18,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "block/block-parent.h"
>  #include "hw/sysbus.h"
>  #include "monitor/hmp.h"
>  #include "monitor/monitor.h"
> @@ -1023,3 +1024,44 @@ bool qmp_command_available(const QmpCommand *cmd, Error **errp)
>      }
>      return true;
>  }
> +
> +static int qdev_find_child(const char *parent_id, const char *child_name,
> +                           BlockDriverState *child_bs,
> +                           BdrvChild **child, Error **errp)
> +{
> +    int ret;
> +    DeviceState *dev;
> +    BlockBackend *blk;
> +
> +    ret = find_device_state(parent_id, &dev, errp);
> +    if (ret <= 0) {
> +        return ret;
> +    }
> +
> +    blk = blk_by_dev(dev);
> +    if (!blk || !blk_root(blk)) {
> +        error_setg(errp, "Device '%s' does not have a block device backend",
> +                   parent_id);
> +        return -EINVAL;
> +    }
> +
> +    if (child_bs && blk_bs(blk) != child_bs) {
> +        error_setg(errp, "Root child of device '%s' doesn't match", parent_id);

What is a "root child", and why would a user care?

The contract missing in PATCH 2 leaves me floundering.

> +        return -EINVAL;
> +    }
> +
> +    *child = blk_root(blk);
> +    return 1;
> +}
> +
> +BlockParentClass block_parent_qdev = {
> +    .name = "qdev",
> +    .find_child = qdev_find_child,
> +};
> +
> +static void qdev_monitor_init(void)
> +{
> +    block_parent_class_register(&block_parent_qdev);
> +}
> +
> +block_init(qdev_monitor_init);

Re: [PATCH 8/8] qapi: add blockdev-replace command

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> Add command that can add and remove filters.
>
> Key points of functionality:
>
> What the command does is simply replace some BdrvChild.bs by some other
> nodes. The tricky thing is selecting there BdrvChild objects.
> To be able to select any kind of BdrvChild we use a generic parent_id,
> which may be a node-name, or qdev id or block export id. In future we
> may support block jobs.
>
> Any kind of ambiguity leads to error. If we have both device named
> device0 and block export named device0 and they both point to same BDS,
> user can't replace root child of one of these parents. So, to be able
> to do replacements, user should avoid duplicating names in different
> parent namespaces.
>
> So, command allows to replace any single child in the graph.
>
> On the other hand we want to realize a kind of bdrv_replace_node(),
> which works well when we want to replace all parents of some node. For
> this kind of task @parents-mode argument implemented.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  qapi/block-core.json | 78 +++++++++++++++++++++++++++++++++++++++++
>  block.c              | 82 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 160 insertions(+)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 675d8265eb..8059b96341 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -5433,3 +5433,81 @@
>  { 'command': 'blockdev-snapshot-delete-internal-sync',
>    'data': { 'device': 'str', '*id': 'str', '*name': 'str'},
>    'returns': 'SnapshotInfo' }
> +
> +##
> +# @BlockdevReplaceParentsMode:
> +#
> +# Alternative (to directly set @parent) way to chose parents in
> +# @blockdev-replace
> +#
> +# @exactly-one: Exactly one parent should match a condition, otherwise
> +#               @blockdev-replace fails.
> +#
> +# @all: All matching parents are taken into account. If replacing lead
> +#       to loops in block graph, @blockdev-replace fails.
> +#
> +# @auto: Same as @all, but automatically skip replacing parents if it
> +#        leads to loops in block graph.
> +#
> +# Since: 6.2
> +##
> +{ 'enum': 'BlockdevReplaceParentsMode',
> +  'data': ['exactly-one', 'all', 'auto'] }
> +
> +##
> +# @BlockdevReplace:
> +#
> +# Declaration of one replacement.

Replacement of what?  A node in the block graph?

> +#
> +# @parent: id of parent. It may be qdev or block export or simple
> +#          node-name.

It may also be a QOM path, because find_device_state() interprets
arguments starting with '/' as QOM paths.

When is a node name "simple"?

Suggest: It may be a qdev ID, a QOM path, a block export ID, or a node
name.

The trouble is of course that we're merging three separate name spaces.

Aside: a single name space for IDs would be so much saner, but we
screwed that up long ago.

>                        If id is ambiguous (for example node-name of
> +#          some BDS equals to block export name), blockdev-replace
> +#          fails.

Is there a way out of this situation, or are is replacement simply
impossible then?

>                    If not specified, blockdev-replace goes through
> +#          @parents-mode scenario, see below. Note, that @parent and
> +#          @parents-mode can't be specified simultaneously.

What if neither is specified?  Hmm, @parents-mode has a default, so
that's what we get.

> +#          If @parent is specified, only one edge is selected. If
> +#          several edges match the condition, blockdev-replace fails.
> +#
> +# @edge: name of the child. If omitted, any child name matches.
> +#
> +# @child: node-name of the child. If omitted, any child matches.
> +#         Must be present if @parent is not specified.

Is @child useful when @parent is present?

What's the difference between "name of the child" and "node name of the
child"?

> +#
> +# @parents-mode: declares how to select edge (or edges) when @parent
> +#                is omitted. Default is 'one'.

'exactly-one'

Minor combinatorial explosion.  There are four optional arguments, one
of them an enum, and only some combination of argument presence and enum
value are valid.  For a serious review, I'd have to make a table of
combinations, then think through every valid row.

Have you considered making this type a union?  Can turn some of your
semantic constraints into syntactical ones.  Say you turn
BlockdevReplaceParentsMode into a tag enum by adding value 'by-id'.
Then branch 'by-id' has member @parent, and the others don't.

> +#
> +# Since: 6.2
> +#
> +# Examples:
> +#
> +# 1. Change root node of some device.
> +#
> +# Note, that @edge name is omitted, as

Scratch "name".

Odd line break.

> +# devices always has only one child. As well, no need in specifying
> +# old @child.

"the old @child".

> +#
> +# -> { "parent": "device0", "new-child": "some-node-name" }
> +#
> +# 2. Insert copy-before-write filter.
> +#
> +# Assume, after blockdev-add we have block-node 'source', with several
> +# writing parents and one copy-before-write 'filter' parent. And we want
> +# to actually insert the filter. We do:
> +#
> +# -> { "child": "source", "parent-mode": "auto", "new-child": "filter" }
> +#
> +# All parents of source would be switched to 'filter' node, except for
> +# 'filter' node itself (otherwise, it will make a loop in block-graph).

Good examples.  I think we need more, to give us an idea on the use
cases for the combinatorial explosion.  I need to know them to be able
to review the interface.

> +##
> +{ 'struct': 'BlockdevReplace',
> +  'data': { '*parent': 'str', '*edge': 'str', '*child': 'str',
> +            '*parents-mode': 'BlockdevReplaceParentsMode',
> +            'new-child': 'str' } }
> +
> +##
> +# @blockdev-replace:
> +#
> +# Do one or several replacements transactionally.
> +##
> +{ 'command': 'blockdev-replace',
> +  'data': { 'replacements': ['BlockdevReplace'] } }

Ignorant question: integration with transaction.json makes no sense?

[...]

Re: [PATCH 8/8] qapi: add blockdev-replace command

[Vladimir Sementsov-Ogievskiy]

Thanks a lot for reviewing!

20.09.2021 09:44, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> 
>> Add command that can add and remove filters.
>>
>> Key points of functionality:
>>
>> What the command does is simply replace some BdrvChild.bs by some other
>> nodes. The tricky thing is selecting there BdrvChild objects.
>> To be able to select any kind of BdrvChild we use a generic parent_id,
>> which may be a node-name, or qdev id or block export id. In future we
>> may support block jobs.
>>
>> Any kind of ambiguity leads to error. If we have both device named
>> device0 and block export named device0 and they both point to same BDS,
>> user can't replace root child of one of these parents. So, to be able
>> to do replacements, user should avoid duplicating names in different
>> parent namespaces.
>>
>> So, command allows to replace any single child in the graph.
>>
>> On the other hand we want to realize a kind of bdrv_replace_node(),
>> which works well when we want to replace all parents of some node. For
>> this kind of task @parents-mode argument implemented.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   qapi/block-core.json | 78 +++++++++++++++++++++++++++++++++++++++++
>>   block.c              | 82 ++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 160 insertions(+)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 675d8265eb..8059b96341 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -5433,3 +5433,81 @@
>>   { 'command': 'blockdev-snapshot-delete-internal-sync',
>>     'data': { 'device': 'str', '*id': 'str', '*name': 'str'},
>>     'returns': 'SnapshotInfo' }
>> +
>> +##
>> +# @BlockdevReplaceParentsMode:
>> +#
>> +# Alternative (to directly set @parent) way to chose parents in
>> +# @blockdev-replace
>> +#
>> +# @exactly-one: Exactly one parent should match a condition, otherwise
>> +#               @blockdev-replace fails.
>> +#
>> +# @all: All matching parents are taken into account. If replacing lead
>> +#       to loops in block graph, @blockdev-replace fails.
>> +#
>> +# @auto: Same as @all, but automatically skip replacing parents if it
>> +#        leads to loops in block graph.
>> +#
>> +# Since: 6.2
>> +##
>> +{ 'enum': 'BlockdevReplaceParentsMode',
>> +  'data': ['exactly-one', 'all', 'auto'] }
>> +
>> +##
>> +# @BlockdevReplace:
>> +#
>> +# Declaration of one replacement.
> 
> Replacement of what?  A node in the block graph?

A specific child node in one or in several edges

> 
>> +#
>> +# @parent: id of parent. It may be qdev or block export or simple
>> +#          node-name.
> 
> It may also be a QOM path, because find_device_state() interprets
> arguments starting with '/' as QOM paths.
> 
> When is a node name "simple"?
> 
> Suggest: It may be a qdev ID, a QOM path, a block export ID, or a node
> name.

OK

> 
> The trouble is of course that we're merging three separate name spaces.

Yes. Alternatively we can also add an enum of node-type [bds, device, export[, job]], and select graph nodes more explicit (by pair of id/path/name and type)

But if we have to use these things in one context, it seems good to enforce users use different names for them. And in this way, we can avoid strict typing.

> 
> Aside: a single name space for IDs would be so much saner, but we
> screwed that up long ago.
> 
>>                         If id is ambiguous (for example node-name of
>> +#          some BDS equals to block export name), blockdev-replace
>> +#          fails.
> 
> Is there a way out of this situation, or are is replacement simply
> impossible then?

In my idea, it's simply impossible. If someone want to use this new interface, he should care to use different names for different things.

> 
>>                     If not specified, blockdev-replace goes through
>> +#          @parents-mode scenario, see below. Note, that @parent and
>> +#          @parents-mode can't be specified simultaneously.
> 
> What if neither is specified?  Hmm, @parents-mode has a default, so
> that's what we get.
> 
>> +#          If @parent is specified, only one edge is selected. If
>> +#          several edges match the condition, blockdev-replace fails.
>> +#
>> +# @edge: name of the child. If omitted, any child name matches.
>> +#
>> +# @child: node-name of the child. If omitted, any child matches.
>> +#         Must be present if @parent is not specified.
> 
> Is @child useful when @parent is present?

You may specify @child and @parent, to replace child in specific edge. Or @parent and @edge. Or all three fields: just to be strict.

> 
> What's the difference between "name of the child" and "node name of the
> child"?

Although we have to deal with different kinds of nodes (BDS, exports, blks, ...),
children are always BDS.

But, may be in the context, it's better say "id of the child".

> 
>> +#
>> +# @parents-mode: declares how to select edge (or edges) when @parent
>> +#                is omitted. Default is 'one'.
> 
> 'exactly-one'
> 
> Minor combinatorial explosion.  There are four optional arguments, one
> of them an enum, and only some combination of argument presence and enum
> value are valid.  For a serious review, I'd have to make a table of
> combinations, then think through every valid row.
> 
> Have you considered making this type a union?  Can turn some of your
> semantic constraints into syntactical ones.  Say you turn
> BlockdevReplaceParentsMode into a tag enum by adding value 'by-id'.
> Then branch 'by-id' has member @parent, and the others don't.


OK. Now, after some time passed, I see that some additional clarifications are needed. Even for me :)

So, the actual modes I have in mind:

1. Replacement for backup: we want to inject copy-before-write filter F above some node X, so that all parents of node X start to access X through filter F. But we want automatically skip parents if modifications leads to loops in the graph (so, we can first carete node F with X as a child, than do replacement, and don't replace child of F by F :).

That's  parents-mode=auto & parent=None & edge=None & child=X

2. Replacement of any specific edge in the graph.

Edge may be specified in different ways: by parent, by child, by edge, and by some combinations of these things. It seems reasonable to allow any combination, if it specifies exactly one field.. Assume we have A -- backing --> B relation in the graph, and want to replace B by filter F in that relation.

2.1 Specify parent:

We may specify all information bits, to be sure that we do what we want and for high probability to fail if we have wrong impression about what's going on in the graph:

parents-mode=None & parent=A & edge=backing & child=B

We can omit edge:

parents-mode=None & parent=A & edge=None & child=B

  - that should fail as ambiguous if B is "double child" of A, with two edges from A to B. But I think, that's unused combination for now)

Or we can omit child

parents-mode=None & parent=A & edge=backing & child=None

  - that should work well, as node shouldn't have more than one backing child.

and we can omit both edge and child:

parents-mode=None & parent=A & edge=None & child=None

  - that will work only if A has exactly one child and fails otherwise. So, that's bad for format nodes but good for filters and for block devices.

2.2 Don't specify parent but specify child:

parents-mode=exactly-one & parent=None & edge=backing & child=B

  - works if B has only one parent with B as backing child

parents-mode=exactly-one & parent=None & edge=None & child=B

  - works if B has only one parent

======================

Now, what's more?

parents-mode=auto & parent=None & edge=root & child=X

- replace only child only for root parents of X  -  may make sense


And all other combinations are

parents-mode=ANY & parent=None & edge=ANY & child=None

  - don't specify neither parent nor child. That works bad with any mode.. Theoretically, we still can support it by looking through the whole graph. If edge=backing and we only only one backing edge in the whole graph we can serve the request.. But we can simply fail and not care.

=====================

What's bad, is that 2.1 and 2.2 are not symmetrical. So, right, it seems better to turn it into union:

1. mode = auto

Replace child in all it's parents where edge match to @edge and avoiding creating loops in the graph

child: required, specify child
edge: optional, if specified, do replacement only in such edges

2. mode = one-edge

Replace child in exactly one edge. If more than one edge matches - re[ace nothing and fail.

parent: optional
edge: optional
child: optional

  - all fields optional, but user is responsible to not be ambiguous. Still, we can enforce that at least one of @parent and @child should be specified.

> 
>> +#
>> +# Since: 6.2
>> +#
>> +# Examples:
>> +#
>> +# 1. Change root node of some device.
>> +#
>> +# Note, that @edge name is omitted, as
> 
> Scratch "name".
> 
> Odd line break.
> 
>> +# devices always has only one child. As well, no need in specifying
>> +# old @child.
> 
> "the old @child".
> 
>> +#
>> +# -> { "parent": "device0", "new-child": "some-node-name" }
>> +#
>> +# 2. Insert copy-before-write filter.
>> +#
>> +# Assume, after blockdev-add we have block-node 'source', with several
>> +# writing parents and one copy-before-write 'filter' parent. And we want
>> +# to actually insert the filter. We do:
>> +#
>> +# -> { "child": "source", "parent-mode": "auto", "new-child": "filter" }
>> +#
>> +# All parents of source would be switched to 'filter' node, except for
>> +# 'filter' node itself (otherwise, it will make a loop in block-graph).
> 
> Good examples.  I think we need more, to give us an idea on the use
> cases for the combinatorial explosion.  I need to know them to be able
> to review the interface.
> 
>> +##
>> +{ 'struct': 'BlockdevReplace',
>> +  'data': { '*parent': 'str', '*edge': 'str', '*child': 'str',
>> +            '*parents-mode': 'BlockdevReplaceParentsMode',
>> +            'new-child': 'str' } }
>> +
>> +##
>> +# @blockdev-replace:
>> +#
>> +# Do one or several replacements transactionally.
>> +##
>> +{ 'command': 'blockdev-replace',
>> +  'data': { 'replacements': ['BlockdevReplace'] } }
> 
> Ignorant question: integration with transaction.json makes no sense?

Recently we allowed do several reopens in one blockdev-reopen. So, it's reasonable to behave same way in blockdev-replace.

Still, I think combination of different commands in a transaction make sense too. So, in my thought, transaction support for blockdev-* graph modification commands is a TODO.

> 
> [...]
> 


-- 
Best regards,
Vladimir

Re: [PATCH 8/8] qapi: add blockdev-replace command

[Vladimir Sementsov-Ogievskiy]

02.08.2021 21:54, Vladimir Sementsov-Ogievskiy wrote:
> Add command that can add and remove filters.
> 
> Key points of functionality:
> 
> What the command does is simply replace some BdrvChild.bs by some other
> nodes. The tricky thing is selecting there BdrvChild objects.
> To be able to select any kind of BdrvChild we use a generic parent_id,
> which may be a node-name, or qdev id or block export id. In future we
> may support block jobs.
> 
> Any kind of ambiguity leads to error. If we have both device named
> device0 and block export named device0 and they both point to same BDS,
> user can't replace root child of one of these parents. So, to be able
> to do replacements, user should avoid duplicating names in different
> parent namespaces.
> 
> So, command allows to replace any single child in the graph.
> 
> On the other hand we want to realize a kind of bdrv_replace_node(),
> which works well when we want to replace all parents of some node. For
> this kind of task @parents-mode argument implemented.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   qapi/block-core.json | 78 +++++++++++++++++++++++++++++++++++++++++
>   block.c              | 82 ++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 160 insertions(+)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 675d8265eb..8059b96341 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -5433,3 +5433,81 @@
>   { 'command': 'blockdev-snapshot-delete-internal-sync',
>     'data': { 'device': 'str', '*id': 'str', '*name': 'str'},
>     'returns': 'SnapshotInfo' }
> +
> +##
> +# @BlockdevReplaceParentsMode:
> +#
> +# Alternative (to directly set @parent) way to chose parents in
> +# @blockdev-replace
> +#
> +# @exactly-one: Exactly one parent should match a condition, otherwise
> +#               @blockdev-replace fails.
> +#
> +# @all: All matching parents are taken into account. If replacing lead
> +#       to loops in block graph, @blockdev-replace fails.
> +#
> +# @auto: Same as @all, but automatically skip replacing parents if it
> +#        leads to loops in block graph.
> +#
> +# Since: 6.2
> +##
> +{ 'enum': 'BlockdevReplaceParentsMode',
> +  'data': ['exactly-one', 'all', 'auto'] }
> +
> +##
> +# @BlockdevReplace:
> +#
> +# Declaration of one replacement.
> +#
> +# @parent: id of parent. It may be qdev or block export or simple
> +#          node-name. If id is ambiguous (for example node-name of
> +#          some BDS equals to block export name), blockdev-replace
> +#          fails. If not specified, blockdev-replace goes through
> +#          @parents-mode scenario, see below. Note, that @parent and
> +#          @parents-mode can't be specified simultaneously.
> +#          If @parent is specified, only one edge is selected. If
> +#          several edges match the condition, blockdev-replace fails.
> +#
> +# @edge: name of the child. If omitted, any child name matches.
> +#
> +# @child: node-name of the child. If omitted, any child matches.
> +#         Must be present if @parent is not specified.
> +#
> +# @parents-mode: declares how to select edge (or edges) when @parent
> +#                is omitted. Default is 'one'.
> +#
> +# Since: 6.2
> +#
> +# Examples:
> +#
> +# 1. Change root node of some device.
> +#
> +# Note, that @edge name is omitted, as
> +# devices always has only one child. As well, no need in specifying
> +# old @child.
> +#
> +# -> { "parent": "device0", "new-child": "some-node-name" }
> +#
> +# 2. Insert copy-before-write filter.
> +#
> +# Assume, after blockdev-add we have block-node 'source', with several
> +# writing parents and one copy-before-write 'filter' parent. And we want
> +# to actually insert the filter. We do:
> +#
> +# -> { "child": "source", "parent-mode": "auto", "new-child": "filter" }
> +#
> +# All parents of source would be switched to 'filter' node, except for
> +# 'filter' node itself (otherwise, it will make a loop in block-graph).
> +##
> +{ 'struct': 'BlockdevReplace',
> +  'data': { '*parent': 'str', '*edge': 'str', '*child': 'str',
> +            '*parents-mode': 'BlockdevReplaceParentsMode',
> +            'new-child': 'str' } }


We also can make 'new-child' a 'BlockdevRef' and allow creating and inserting the filter in one command.


-- 
Best regards,
Vladimir

Re: [PATCH 8/8] qapi: add blockdev-replace command

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> Thanks a lot for reviewing!
>
> 20.09.2021 09:44, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>> 
>>> Add command that can add and remove filters.
>>>
>>> Key points of functionality:
>>>
>>> What the command does is simply replace some BdrvChild.bs by some other
>>> nodes. The tricky thing is selecting there BdrvChild objects.
>>> To be able to select any kind of BdrvChild we use a generic parent_id,
>>> which may be a node-name, or qdev id or block export id. In future we
>>> may support block jobs.
>>>
>>> Any kind of ambiguity leads to error. If we have both device named
>>> device0 and block export named device0 and they both point to same BDS,
>>> user can't replace root child of one of these parents. So, to be able
>>> to do replacements, user should avoid duplicating names in different
>>> parent namespaces.
>>>
>>> So, command allows to replace any single child in the graph.
>>>
>>> On the other hand we want to realize a kind of bdrv_replace_node(),
>>> which works well when we want to replace all parents of some node. For
>>> this kind of task @parents-mode argument implemented.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   qapi/block-core.json | 78 +++++++++++++++++++++++++++++++++++++++++
>>>   block.c              | 82 ++++++++++++++++++++++++++++++++++++++++++++
>>>   2 files changed, 160 insertions(+)
>>>
>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>> index 675d8265eb..8059b96341 100644
>>> --- a/qapi/block-core.json
>>> +++ b/qapi/block-core.json
>>> @@ -5433,3 +5433,81 @@
>>>   { 'command': 'blockdev-snapshot-delete-internal-sync',
>>>     'data': { 'device': 'str', '*id': 'str', '*name': 'str'},
>>>     'returns': 'SnapshotInfo' }
>>> +
>>> +##
>>> +# @BlockdevReplaceParentsMode:
>>> +#
>>> +# Alternative (to directly set @parent) way to chose parents in
>>> +# @blockdev-replace
>>> +#
>>> +# @exactly-one: Exactly one parent should match a condition, otherwise
>>> +#               @blockdev-replace fails.
>>> +#
>>> +# @all: All matching parents are taken into account. If replacing lead
>>> +#       to loops in block graph, @blockdev-replace fails.
>>> +#
>>> +# @auto: Same as @all, but automatically skip replacing parents if it
>>> +#        leads to loops in block graph.
>>> +#
>>> +# Since: 6.2
>>> +##
>>> +{ 'enum': 'BlockdevReplaceParentsMode',
>>> +  'data': ['exactly-one', 'all', 'auto'] }
>>> +
>>> +##
>>> +# @BlockdevReplace:
>>> +#
>>> +# Declaration of one replacement.
>> 
>> Replacement of what?  A node in the block graph?
>
> A specific child node in one or in several edges

Spell that out in the doc comment, please.

>> 
>>> +#
>>> +# @parent: id of parent. It may be qdev or block export or simple
>>> +#          node-name.
>> 
>> It may also be a QOM path, because find_device_state() interprets
>> arguments starting with '/' as QOM paths.
>> 
>> When is a node name "simple"?
>> 
>> Suggest: It may be a qdev ID, a QOM path, a block export ID, or a node
>> name.
>
> OK
>
>> 
>> The trouble is of course that we're merging three separate name spaces.
>
> Yes. Alternatively we can also add an enum of node-type [bds, device, export[, job]], and select graph nodes more explicit (by pair of id/path/name and type)
>
> But if we have to use these things in one context, it seems good to enforce users use different names for them. And in this way, we can avoid strict typing.

Is there precedence in QMP for merging ID name spaces, or for selecting
a name space?

>> Aside: a single name space for IDs would be so much saner, but we
>> screwed that up long ago.

Throwing some of the multiple name spaces together some of the time
feels like another mistake.

>> 
>>>                         If id is ambiguous (for example node-name of
>>> +#          some BDS equals to block export name), blockdev-replace
>>> +#          fails.
>> 
>> Is there a way out of this situation, or are is replacement simply
>> impossible then?
>
> In my idea, it's simply impossible. If someone want to use this new interface, he should care to use different names for different things.

Reminds me of device_del, which simply could not delete a device without
an ID.  Made many users go "oh" (or possibly a more colorful version
thereof), until daniel fixed it in commit 6287d827d4 "monitor: allow
device_del to accept QOM paths" for v2.5.

>> 
>>>                     If not specified, blockdev-replace goes through
>>> +#          @parents-mode scenario, see below. Note, that @parent and
>>> +#          @parents-mode can't be specified simultaneously.
>> 
>> What if neither is specified?  Hmm, @parents-mode has a default, so
>> that's what we get.
>> 
>>> +#          If @parent is specified, only one edge is selected. If
>>> +#          several edges match the condition, blockdev-replace fails.
>>> +#
>>> +# @edge: name of the child. If omitted, any child name matches.
>>> +#
>>> +# @child: node-name of the child. If omitted, any child matches.
>>> +#         Must be present if @parent is not specified.
>> 
>> Is @child useful when @parent is present?
>
> You may specify @child and @parent, to replace child in specific edge. Or @parent and @edge. Or all three fields: just to be strict.
>
>> 
>> What's the difference between "name of the child" and "node name of the
>> child"?
>
> Although we have to deal with different kinds of nodes (BDS, exports, blks, ...),
> children are always BDS.
>
> But, may be in the context, it's better say "id of the child".

I'm confused about the difference between "@edge: name of the child",
and "@child: node-name of the child".

>> 
>>> +#
>>> +# @parents-mode: declares how to select edge (or edges) when @parent
>>> +#                is omitted. Default is 'one'.
>> 
>> 'exactly-one'
>> 
>> Minor combinatorial explosion.  There are four optional arguments, one
>> of them an enum, and only some combination of argument presence and enum
>> value are valid.  For a serious review, I'd have to make a table of
>> combinations, then think through every valid row.
>> 
>> Have you considered making this type a union?  Can turn some of your
>> semantic constraints into syntactical ones.  Say you turn
>> BlockdevReplaceParentsMode into a tag enum by adding value 'by-id'.
>> Then branch 'by-id' has member @parent, and the others don't.
>
>
> OK. Now, after some time passed, I see that some additional clarifications are needed. Even for me :)

Sounds familiar :)

> So, the actual modes I have in mind:
>
> 1. Replacement for backup: we want to inject copy-before-write filter F above some node X, so that all parents of node X start to access X through filter F. But we want automatically skip parents if modifications leads to loops in the graph (so, we can first carete node F with X as a child, than do replacement, and don't replace child of F by F :).
>
> That's  parents-mode=auto & parent=None & edge=None & child=X
>
> 2. Replacement of any specific edge in the graph.
>
> Edge may be specified in different ways: by parent, by child, by edge, and by some combinations of these things. It seems reasonable to allow any combination, if it specifies exactly one field.. Assume we have A -- backing --> B relation in the graph, and want to replace B by filter F in that relation.

An edge always goes from a source node (a.k.a. parent) to a target node
(a.k.a. child).

Each edge from a source node has a unique name in the source node, such
as "backing".

Correct?

The obvious way to identify an edge is (source node name, edge name).

Throwing in the target name is redundant.  Observation, not criticism.

All other ways can be ambigous:

    (source node name, target node name), because multiple edges can
    connect the two.

    (edge name, target node name), because multiple source nodes can use
    the same edge name to connect to the target node.

    ...

Even ways that can be ambigous need not be in a specific graph:

    Just source node name suffices when there is only one edge leaving
    it.

    Even just edge name can theoretically suffice.

    ...

Do we really *need* this much flexibility?  Why can't we simply require
(source node name, edge name), and call it a day?

> 2.1 Specify parent:
>
> We may specify all information bits, to be sure that we do what we want and for high probability to fail if we have wrong impression about what's going on in the graph:
>
> parents-mode=None & parent=A & edge=backing & child=B
>
> We can omit edge:
>
> parents-mode=None & parent=A & edge=None & child=B
>
>   - that should fail as ambiguous if B is "double child" of A, with two edges from A to B. But I think, that's unused combination for now)
>
> Or we can omit child
>
> parents-mode=None & parent=A & edge=backing & child=None
>
>   - that should work well, as node shouldn't have more than one backing child.
>
> and we can omit both edge and child:
>
> parents-mode=None & parent=A & edge=None & child=None
>
>   - that will work only if A has exactly one child and fails otherwise. So, that's bad for format nodes but good for filters and for block devices.
>
> 2.2 Don't specify parent but specify child:
>
> parents-mode=exactly-one & parent=None & edge=backing & child=B
>
>   - works if B has only one parent with B as backing child
>
> parents-mode=exactly-one & parent=None & edge=None & child=B
>
>   - works if B has only one parent
>
> ======================
>
> Now, what's more?
>
> parents-mode=auto & parent=None & edge=root & child=X
>
> - replace only child only for root parents of X  -  may make sense
>
>
> And all other combinations are
>
> parents-mode=ANY & parent=None & edge=ANY & child=None
>
>   - don't specify neither parent nor child. That works bad with any mode.. Theoretically, we still can support it by looking through the whole graph. If edge=backing and we only only one backing edge in the whole graph we can serve the request.. But we can simply fail and not care.
>
> =====================
>
> What's bad, is that 2.1 and 2.2 are not symmetrical. So, right, it seems better to turn it into union:
>
> 1. mode = auto
>
> Replace child in all it's parents where edge match to @edge and avoiding creating loops in the graph
>
> child: required, specify child
> edge: optional, if specified, do replacement only in such edges

This is almost the same as a transaction of one-edge replacements for
all parents, optionally filtered by @edge.

They differ when the parents can change spontaneously.  The transaction
then might be for a stale set of parents.  Can this happen?

The other difference is of course that having to enumerate the edges
could be bothersome.  Some amount of bother is okay.  QMP provides basic
building blocks.  When we try to provide more, we tend to fail.

> 2. mode = one-edge
>
> Replace child in exactly one edge. If more than one edge matches - re[ace nothing and fail.
>
> parent: optional
> edge: optional
> child: optional
>
>   - all fields optional, but user is responsible to not be ambiguous. Still, we can enforce that at least one of @parent and @child should be specified.

Do we really need this much flexibility in edge selection?

>
>> 
>>> +#
>>> +# Since: 6.2
>>> +#
>>> +# Examples:
>>> +#
>>> +# 1. Change root node of some device.
>>> +#
>>> +# Note, that @edge name is omitted, as
>> 
>> Scratch "name".
>> 
>> Odd line break.
>> 
>>> +# devices always has only one child. As well, no need in specifying
>>> +# old @child.
>> 
>> "the old @child".
>> 
>>> +#
>>> +# -> { "parent": "device0", "new-child": "some-node-name" }
>>> +#
>>> +# 2. Insert copy-before-write filter.
>>> +#
>>> +# Assume, after blockdev-add we have block-node 'source', with several
>>> +# writing parents and one copy-before-write 'filter' parent. And we want
>>> +# to actually insert the filter. We do:
>>> +#
>>> +# -> { "child": "source", "parent-mode": "auto", "new-child": "filter" }
>>> +#
>>> +# All parents of source would be switched to 'filter' node, except for
>>> +# 'filter' node itself (otherwise, it will make a loop in block-graph).
>> 
>> Good examples.  I think we need more, to give us an idea on the use
>> cases for the combinatorial explosion.  I need to know them to be able
>> to review the interface.
>> 
>>> +##
>>> +{ 'struct': 'BlockdevReplace',
>>> +  'data': { '*parent': 'str', '*edge': 'str', '*child': 'str',
>>> +            '*parents-mode': 'BlockdevReplaceParentsMode',
>>> +            'new-child': 'str' } }
>>> +
>>> +##
>>> +# @blockdev-replace:
>>> +#
>>> +# Do one or several replacements transactionally.
>>> +##
>>> +{ 'command': 'blockdev-replace',
>>> +  'data': { 'replacements': ['BlockdevReplace'] } }
>> 
>> Ignorant question: integration with transaction.json makes no sense?
>
> Recently we allowed do several reopens in one blockdev-reopen. So, it's reasonable to behave same way in blockdev-replace.

I didn't see that going in.  I trust reopening multiple in one
transaction is useful, but commit 3908b7a899 fails to explain why.
Mistake; we should *insist* on capturing the "why" in the commit
message.

I dislike having multiple ways to do the same thing (here:
transactions).  If there are reasons why the transaction command cannot
be used, fine, provide another suitable interface.  But when the
existing interface serves, please don't reinvent it.

> Still, I think combination of different commands in a transaction make sense too. So, in my thought, transaction support for blockdev-* graph modification commands is a TODO.
>
>> 
>> [...]
>> 

Re: [PATCH 8/8] qapi: add blockdev-replace command

[Vladimir Sementsov-Ogievskiy]

23.09.2021 13:09, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> 
>> Thanks a lot for reviewing!
>>
>> 20.09.2021 09:44, Markus Armbruster wrote:
>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>>
>>>> Add command that can add and remove filters.
>>>>
>>>> Key points of functionality:
>>>>
>>>> What the command does is simply replace some BdrvChild.bs by some other
>>>> nodes. The tricky thing is selecting there BdrvChild objects.
>>>> To be able to select any kind of BdrvChild we use a generic parent_id,
>>>> which may be a node-name, or qdev id or block export id. In future we
>>>> may support block jobs.
>>>>
>>>> Any kind of ambiguity leads to error. If we have both device named
>>>> device0 and block export named device0 and they both point to same BDS,
>>>> user can't replace root child of one of these parents. So, to be able
>>>> to do replacements, user should avoid duplicating names in different
>>>> parent namespaces.
>>>>
>>>> So, command allows to replace any single child in the graph.
>>>>
>>>> On the other hand we want to realize a kind of bdrv_replace_node(),
>>>> which works well when we want to replace all parents of some node. For
>>>> this kind of task @parents-mode argument implemented.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> ---
>>>>    qapi/block-core.json | 78 +++++++++++++++++++++++++++++++++++++++++
>>>>    block.c              | 82 ++++++++++++++++++++++++++++++++++++++++++++
>>>>    2 files changed, 160 insertions(+)
>>>>
>>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>>> index 675d8265eb..8059b96341 100644
>>>> --- a/qapi/block-core.json
>>>> +++ b/qapi/block-core.json
>>>> @@ -5433,3 +5433,81 @@
>>>>    { 'command': 'blockdev-snapshot-delete-internal-sync',
>>>>      'data': { 'device': 'str', '*id': 'str', '*name': 'str'},
>>>>      'returns': 'SnapshotInfo' }
>>>> +
>>>> +##
>>>> +# @BlockdevReplaceParentsMode:
>>>> +#
>>>> +# Alternative (to directly set @parent) way to chose parents in
>>>> +# @blockdev-replace
>>>> +#
>>>> +# @exactly-one: Exactly one parent should match a condition, otherwise
>>>> +#               @blockdev-replace fails.
>>>> +#
>>>> +# @all: All matching parents are taken into account. If replacing lead
>>>> +#       to loops in block graph, @blockdev-replace fails.
>>>> +#
>>>> +# @auto: Same as @all, but automatically skip replacing parents if it
>>>> +#        leads to loops in block graph.
>>>> +#
>>>> +# Since: 6.2
>>>> +##
>>>> +{ 'enum': 'BlockdevReplaceParentsMode',
>>>> +  'data': ['exactly-one', 'all', 'auto'] }
>>>> +
>>>> +##
>>>> +# @BlockdevReplace:
>>>> +#
>>>> +# Declaration of one replacement.
>>>
>>> Replacement of what?  A node in the block graph?
>>
>> A specific child node in one or in several edges
> 
> Spell that out in the doc comment, please.
> 
>>>
>>>> +#
>>>> +# @parent: id of parent. It may be qdev or block export or simple
>>>> +#          node-name.
>>>
>>> It may also be a QOM path, because find_device_state() interprets
>>> arguments starting with '/' as QOM paths.
>>>
>>> When is a node name "simple"?
>>>
>>> Suggest: It may be a qdev ID, a QOM path, a block export ID, or a node
>>> name.
>>
>> OK
>>
>>>
>>> The trouble is of course that we're merging three separate name spaces.
>>
>> Yes. Alternatively we can also add an enum of node-type [bds, device, export[, job]], and select graph nodes more explicit (by pair of id/path/name and type)
>>
>> But if we have to use these things in one context, it seems good to enforce users use different names for them. And in this way, we can avoid strict typing.
> 
> Is there precedence in QMP for merging ID name spaces, or for selecting
> a name space?

Hmm, I didn't see neither of it.

> 
>>> Aside: a single name space for IDs would be so much saner, but we
>>> screwed that up long ago.
> 
> Throwing some of the multiple name spaces together some of the time
> feels like another mistake.
> 
>>>
>>>>                          If id is ambiguous (for example node-name of
>>>> +#          some BDS equals to block export name), blockdev-replace
>>>> +#          fails.
>>>
>>> Is there a way out of this situation, or are is replacement simply
>>> impossible then?
>>
>> In my idea, it's simply impossible. If someone want to use this new interface, he should care to use different names for different things.
> 
> Reminds me of device_del, which simply could not delete a device without
> an ID.  Made many users go "oh" (or possibly a more colorful version
> thereof), until daniel fixed it in commit 6287d827d4 "monitor: allow
> device_del to accept QOM paths" for v2.5.
> 
>>>
>>>>                      If not specified, blockdev-replace goes through
>>>> +#          @parents-mode scenario, see below. Note, that @parent and
>>>> +#          @parents-mode can't be specified simultaneously.
>>>
>>> What if neither is specified?  Hmm, @parents-mode has a default, so
>>> that's what we get.
>>>
>>>> +#          If @parent is specified, only one edge is selected. If
>>>> +#          several edges match the condition, blockdev-replace fails.
>>>> +#
>>>> +# @edge: name of the child. If omitted, any child name matches.
>>>> +#
>>>> +# @child: node-name of the child. If omitted, any child matches.
>>>> +#         Must be present if @parent is not specified.
>>>
>>> Is @child useful when @parent is present?
>>
>> You may specify @child and @parent, to replace child in specific edge. Or @parent and @edge. Or all three fields: just to be strict.
>>
>>>
>>> What's the difference between "name of the child" and "node name of the
>>> child"?
>>
>> Although we have to deal with different kinds of nodes (BDS, exports, blks, ...),
>> children are always BDS.
>>
>> But, may be in the context, it's better say "id of the child".
> 
> I'm confused about the difference between "@edge: name of the child",
> and "@child: node-name of the child".
> 
>>>
>>>> +#
>>>> +# @parents-mode: declares how to select edge (or edges) when @parent
>>>> +#                is omitted. Default is 'one'.
>>>
>>> 'exactly-one'
>>>
>>> Minor combinatorial explosion.  There are four optional arguments, one
>>> of them an enum, and only some combination of argument presence and enum
>>> value are valid.  For a serious review, I'd have to make a table of
>>> combinations, then think through every valid row.
>>>
>>> Have you considered making this type a union?  Can turn some of your
>>> semantic constraints into syntactical ones.  Say you turn
>>> BlockdevReplaceParentsMode into a tag enum by adding value 'by-id'.
>>> Then branch 'by-id' has member @parent, and the others don't.
>>
>>
>> OK. Now, after some time passed, I see that some additional clarifications are needed. Even for me :)
> 
> Sounds familiar :)
> 
>> So, the actual modes I have in mind:
>>
>> 1. Replacement for backup: we want to inject copy-before-write filter F above some node X, so that all parents of node X start to access X through filter F. But we want automatically skip parents if modifications leads to loops in the graph (so, we can first carete node F with X as a child, than do replacement, and don't replace child of F by F :).
>>
>> That's  parents-mode=auto & parent=None & edge=None & child=X
>>
>> 2. Replacement of any specific edge in the graph.
>>
>> Edge may be specified in different ways: by parent, by child, by edge, and by some combinations of these things. It seems reasonable to allow any combination, if it specifies exactly one field.. Assume we have A -- backing --> B relation in the graph, and want to replace B by filter F in that relation.
> 
> An edge always goes from a source node (a.k.a. parent) to a target node
> (a.k.a. child).
> 
> Each edge from a source node has a unique name in the source node, such
> as "backing".
> 
> Correct?

I'm not sure.. Of course node can't have several backing children.. Quorum cares to name children differently.

But for example, block-stream job may have several children named simply "intermediate node".

But block-jobs children is so internal feature, that I'm not sure we can allow user simply replace them. That's why this series doesn't allow select jobs as parents.

> 
> The obvious way to identify an edge is (source node name, edge name).
> 
> Throwing in the target name is redundant.  Observation, not criticism.
> 
> All other ways can be ambigous:
> 
>      (source node name, target node name), because multiple edges can
>      connect the two.

Still, I have never seen such a use case)

> 
>      (edge name, target node name), because multiple source nodes can use
>      the same edge name to connect to the target node.
> 
>      ...
> 
> Even ways that can be ambigous need not be in a specific graph:
> 
>      Just source node name suffices when there is only one edge leaving
>      it.
> 
>      Even just edge name can theoretically suffice.
> 
>      ...
> 
> Do we really *need* this much flexibility?  Why can't we simply require
> (source node name, edge name), and call it a day?

I don't know) That just what come into my mind. It's simple enough to restrict the flexibility for now and add it in future if needed.

> 
>> 2.1 Specify parent:
>>
>> We may specify all information bits, to be sure that we do what we want and for high probability to fail if we have wrong impression about what's going on in the graph:
>>
>> parents-mode=None & parent=A & edge=backing & child=B
>>
>> We can omit edge:
>>
>> parents-mode=None & parent=A & edge=None & child=B
>>
>>    - that should fail as ambiguous if B is "double child" of A, with two edges from A to B. But I think, that's unused combination for now)
>>
>> Or we can omit child
>>
>> parents-mode=None & parent=A & edge=backing & child=None
>>
>>    - that should work well, as node shouldn't have more than one backing child.
>>
>> and we can omit both edge and child:
>>
>> parents-mode=None & parent=A & edge=None & child=None
>>
>>    - that will work only if A has exactly one child and fails otherwise. So, that's bad for format nodes but good for filters and for block devices.
>>
>> 2.2 Don't specify parent but specify child:
>>
>> parents-mode=exactly-one & parent=None & edge=backing & child=B
>>
>>    - works if B has only one parent with B as backing child
>>
>> parents-mode=exactly-one & parent=None & edge=None & child=B
>>
>>    - works if B has only one parent
>>
>> ======================
>>
>> Now, what's more?
>>
>> parents-mode=auto & parent=None & edge=root & child=X
>>
>> - replace only child only for root parents of X  -  may make sense
>>
>>
>> And all other combinations are
>>
>> parents-mode=ANY & parent=None & edge=ANY & child=None
>>
>>    - don't specify neither parent nor child. That works bad with any mode.. Theoretically, we still can support it by looking through the whole graph. If edge=backing and we only only one backing edge in the whole graph we can serve the request.. But we can simply fail and not care.
>>
>> =====================
>>
>> What's bad, is that 2.1 and 2.2 are not symmetrical. So, right, it seems better to turn it into union:
>>
>> 1. mode = auto
>>
>> Replace child in all it's parents where edge match to @edge and avoiding creating loops in the graph
>>
>> child: required, specify child
>> edge: optional, if specified, do replacement only in such edges
> 
> This is almost the same as a transaction of one-edge replacements for
> all parents, optionally filtered by @edge.
> 
> They differ when the parents can change spontaneously.  The transaction
> then might be for a stale set of parents.  Can this happen?
> 
> The other difference is of course that having to enumerate the edges
> could be bothersome.  Some amount of bother is okay.  QMP provides basic
> building blocks.  When we try to provide more, we tend to fail.
> 
>> 2. mode = one-edge
>>
>> Replace child in exactly one edge. If more than one edge matches - re[ace nothing and fail.
>>
>> parent: optional
>> edge: optional
>> child: optional
>>
>>    - all fields optional, but user is responsible to not be ambiguous. Still, we can enforce that at least one of @parent and @child should be specified.
> 
> Do we really need this much flexibility in edge selection?
> 
>>
>>>
>>>> +#
>>>> +# Since: 6.2
>>>> +#
>>>> +# Examples:
>>>> +#
>>>> +# 1. Change root node of some device.
>>>> +#
>>>> +# Note, that @edge name is omitted, as
>>>
>>> Scratch "name".
>>>
>>> Odd line break.
>>>
>>>> +# devices always has only one child. As well, no need in specifying
>>>> +# old @child.
>>>
>>> "the old @child".
>>>
>>>> +#
>>>> +# -> { "parent": "device0", "new-child": "some-node-name" }
>>>> +#
>>>> +# 2. Insert copy-before-write filter.
>>>> +#
>>>> +# Assume, after blockdev-add we have block-node 'source', with several
>>>> +# writing parents and one copy-before-write 'filter' parent. And we want
>>>> +# to actually insert the filter. We do:
>>>> +#
>>>> +# -> { "child": "source", "parent-mode": "auto", "new-child": "filter" }
>>>> +#
>>>> +# All parents of source would be switched to 'filter' node, except for
>>>> +# 'filter' node itself (otherwise, it will make a loop in block-graph).
>>>
>>> Good examples.  I think we need more, to give us an idea on the use
>>> cases for the combinatorial explosion.  I need to know them to be able
>>> to review the interface.
>>>
>>>> +##
>>>> +{ 'struct': 'BlockdevReplace',
>>>> +  'data': { '*parent': 'str', '*edge': 'str', '*child': 'str',
>>>> +            '*parents-mode': 'BlockdevReplaceParentsMode',
>>>> +            'new-child': 'str' } }
>>>> +
>>>> +##
>>>> +# @blockdev-replace:
>>>> +#
>>>> +# Do one or several replacements transactionally.
>>>> +##
>>>> +{ 'command': 'blockdev-replace',
>>>> +  'data': { 'replacements': ['BlockdevReplace'] } }
>>>
>>> Ignorant question: integration with transaction.json makes no sense?
>>
>> Recently we allowed do several reopens in one blockdev-reopen. So, it's reasonable to behave same way in blockdev-replace.
> 
> I didn't see that going in.  I trust reopening multiple in one
> transaction is useful, but commit 3908b7a899 fails to explain why.
> Mistake; we should *insist* on capturing the "why" in the commit
> message.

The reason was that to remove filter, we should do two replacements in one transaction, otherwise filter may conflict with original parent after replacement..

But finally, I had to add "if (!QLIST_EMPTY(&bs->parents))" hack to cbw_child_perm() of copy-before-write filter, so it should be possible to remove the filter in two steps: 1. replace child in original parent 2. remove the filter. (and filter will not conflict, thanks to the hack).

And that time we thought blockdev-reopen is good for manipulation with filters. Now it's obvious that it is not.


> 
> I dislike having multiple ways to do the same thing (here:
> transactions).  If there are reasons why the transaction command cannot
> be used, fine, provide another suitable interface.  But when the
> existing interface serves, please don't reinvent it.
> 
>> Still, I think combination of different commands in a transaction make sense too. So, in my thought, transaction support for blockdev-* graph modification commands is a TODO.
>>
>>>
>>> [...]
>>>
> 


Oh, that's all makes more questions than answers :)

1. It's OK to use one BlockdevReplace instead of a list and concentrate on transaction support. That's a mission I keep in mind: moving qapi transactions to use util/transactions.c engine for native integration with modern block modifications.

2. It's OK to limit "one-edge" flexibility, anyway, I don't know do we need it or not.

Still, are use sure that for user it will be simpler to replace root node by qdev path then by node-name? Both variants allow to determine the edge in the graph : qdiv --root--> node-name. But node-name may be preferable in graph operations.
Hmm, on the other hand, if user rely on possibility to specify edge by child, he'll make implementation which will fail to support several qdev parents for one driver node. So, maybe you are right, better not allow it.

3. I'm not sure that we can avoid "auto" mode. It makes inserting copy-before-write filter rather simple. If we force user to specify all parents by hand, it may complicate implementation in libvirt. Note also that we don't have a good way to query all parents of the node..

4. And I'm not sure about id namespaces merging, how much is it bad. Now I tend to agree that it seems unsafe.

Do we want force users use different names globally, or instead use pairs (type, id) to identify them? Like ("qdev", "<qdev_id>"), ("export", "<export-name>"), ("driver", "node-name")..


Peter, Kevin, what do you think about this all?


-- 
Best regards,
Vladimir