Now we have blockdev-replace QMP command, which depend on a possibility
to select any block parent (block node, block export, or qdev) by one
unique name. The command fails, if name is ambiguous (i.e., match
several parents of different types). In future we want to rid of this
ambiguity.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---

v11: rework separate warn_s into one common function
     check_existing_parent_id().

 block.c                            |  6 ++++++
 block/export/export.c              |  6 ++++++
 blockdev.c                         | 21 +++++++++++++++++++++
 docs/about/deprecated.rst          | 10 ++++++++++
 include/block/block-global-state.h |  2 ++
 stubs/check-existing-parent-id.c   |  6 ++++++
 stubs/meson.build                  |  1 +
 system/qdev-monitor.c              | 20 ++++++++++++++++++++
 8 files changed, 72 insertions(+)
 create mode 100644 stubs/check-existing-parent-id.c

diff --git a/block.c b/block.c
index 8254d57212..69674ad4ed 100644
--- a/block.c
+++ b/block.c
@@ -1618,6 +1618,7 @@ static void bdrv_assign_node_name(BlockDriverState *bs,
 {
     char *gen_node_name = NULL;
     GLOBAL_STATE_CODE();
+    Error *local_err;
 
     if (!node_name) {
         node_name = gen_node_name = id_generate(ID_BLOCK);
@@ -1649,6 +1650,11 @@ static void bdrv_assign_node_name(BlockDriverState *bs,
         goto out;
     }
 
+    if (!check_existing_parent_id(node_name, &local_err)) {
+        error_report_err(local_err);
+        local_err = NULL;
+    }
+
     /* copy node name into the bs and insert it into the graph list */
     pstrcpy(bs->node_name, sizeof(bs->node_name), node_name);
     QTAILQ_INSERT_TAIL(&graph_bdrv_states, bs, node_list);
diff --git a/block/export/export.c b/block/export/export.c
index 9169b43e13..174c86b4d2 100644
--- a/block/export/export.c
+++ b/block/export/export.c
@@ -96,6 +96,7 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp)
     AioContext *ctx;
     uint64_t perm;
     int ret;
+    Error *local_err = NULL;
 
     GLOBAL_STATE_CODE();
 
@@ -108,6 +109,11 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp)
         return NULL;
     }
 
+    if (!check_existing_parent_id(export->id, &local_err)) {
+        error_report_err(local_err);
+        local_err = NULL;
+    }
+
     drv = blk_exp_find_driver(export->type);
     if (!drv) {
         error_setg(errp, "No driver found for the requested export type");
diff --git a/blockdev.c b/blockdev.c
index 3082a5763c..643b2132c9 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3681,6 +3681,27 @@ out:
     bdrv_drain_all_end();
 }
 
+
+bool check_existing_parent_id(const char *id, Error **errp)
+{
+    if (bdrv_find_node(id)) {
+        error_setg(errp, "block node with id '%s' already exist", id);
+        return false;
+    }
+
+    if (blk_by_qdev_id(id, NULL)) {
+        error_setg(errp, "block device with id '%s' already exist", id);
+        return false;
+    }
+
+    if (blk_by_export_id(id, NULL)) {
+        error_setg(errp, "block export with id '%s' already exist", id);
+        return false;
+    }
+
+    return true;
+}
+
 void qmp_blockdev_replace(const char *parent, const char *child,
                           const char *new_child, Error **errp)
 {
diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 88efa3aa80..18bb1eeafc 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -551,3 +551,13 @@ command documentation for details on the ``fdset`` usage.
 
 The ``zero-blocks`` capability was part of the block migration which
 doesn't exist anymore since it was removed in QEMU v9.1.
+
+Identifiers
+-----------
+
+Possibility to intersect qdev ids/paths, block node names, and block
+export names namespaces is deprecated. In future that would be
+abandoned and all block exports, block nodes and devices will have
+unique names. Now, reusing the name for another type of object (for
+example, creating block-node with node-name equal to existing qdev
+id) produce a warning.
diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h
index ed89999f0f..5014324241 100644
--- a/include/block/block-global-state.h
+++ b/include/block/block-global-state.h
@@ -318,4 +318,6 @@ void bdrv_unregister_buf(BlockDriverState *bs, void *host, size_t size);
 
 void bdrv_cancel_in_flight(BlockDriverState *bs);
 
+bool check_existing_parent_id(const char *id, Error **errp);
+
 #endif /* BLOCK_GLOBAL_STATE_H */
diff --git a/stubs/check-existing-parent-id.c b/stubs/check-existing-parent-id.c
new file mode 100644
index 0000000000..ef5ea3f26d
--- /dev/null
+++ b/stubs/check-existing-parent-id.c
@@ -0,0 +1,6 @@
+#include "qemu/osdep.h"
+#include "block/block-global-state.h"
+
+bool check_existing_parent_id(const char *id, Error **errp) {
+    return true;
+}
diff --git a/stubs/meson.build b/stubs/meson.build
index 30536ec8fa..3bd8649bd1 100644
--- a/stubs/meson.build
+++ b/stubs/meson.build
@@ -16,6 +16,7 @@ if have_block
   stub_ss.add(files('blk-commit-all.c'))
   stub_ss.add(files('blk-exp-close-all.c'))
   stub_ss.add(files('blk-by-qdev-id.c'))
+  stub_ss.add(files('check-existing-parent-id.c'))
   stub_ss.add(files('blk-exp-find-by-blk.c'))
   stub_ss.add(files('blockdev-close-all-bdrv-states.c'))
   stub_ss.add(files('change-state-handler.c'))
diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
index be18902bb2..ca14135191 100644
--- a/system/qdev-monitor.c
+++ b/system/qdev-monitor.c
@@ -593,6 +593,7 @@ static BusState *qbus_find(const char *path, Error **errp)
 const char *qdev_set_id(DeviceState *dev, char *id, Error **errp)
 {
     ObjectProperty *prop;
+    Error *local_err = NULL;
 
     assert(!dev->id && !dev->realized);
 
@@ -601,6 +602,18 @@ const char *qdev_set_id(DeviceState *dev, char *id, Error **errp)
      * has no parent
      */
     if (id) {
+        g_autofree char *fullpath = g_strdup_printf("/peripheral/%s", id);
+
+        if (!check_existing_parent_id(id, &local_err)) {
+            error_report_err(local_err);
+            local_err = NULL;
+        }
+
+        if (!check_existing_parent_id(fullpath, &local_err)) {
+            error_report_err(local_err);
+            local_err = NULL;
+        }
+
         prop = object_property_try_add_child(qdev_get_peripheral(), id,
                                              OBJECT(dev), NULL);
         if (prop) {
@@ -613,6 +626,13 @@ const char *qdev_set_id(DeviceState *dev, char *id, Error **errp)
     } else {
         static int anon_count;
         gchar *name = g_strdup_printf("device[%d]", anon_count++);
+        g_autofree char *fullpath = g_strdup_printf("/peripheral-anon/%s", id);
+
+        if (!check_existing_parent_id(fullpath, &local_err)) {
+            error_report_err(local_err);
+            local_err = NULL;
+        }
+
         prop = object_property_add_child(qdev_get_peripheral_anon(), name,
                                          OBJECT(dev));
         g_free(name);
-- 
2.52.0

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:

> Now we have blockdev-replace QMP command, which depend on a possibility
> to select any block parent (block node, block export, or qdev) by one
> unique name. The command fails, if name is ambiguous (i.e., match
> several parents of different types). In future we want to rid of this
> ambiguity.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

We have numerous kinds of IDs, i.e. names chosen by the user than need
to be unique, but only among the same kind.  For instance, you can't
name two block nodes "foo", but you can name a block node, a block
export, a qdev, and a network backend "foo".  Using the same ID for
multiple things is of course a bad idea.  Permitting it was also a bad
idea.

Your patch rectifies this design mistake, but only partially.  Consider:

* IDs need to be unique with their kind.  This is what we have now.  I
  don't like it.

* IDs need to be unique among their kind and possibly some set of
  additional kinds.  This is where your patch takes us.  I like it even
  less, to be honest.

* IDs need to be unique, period.  This is what I'd like to have.

Thoughts?

On 04.02.26 15:38, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
> 
>> Now we have blockdev-replace QMP command, which depend on a possibility
>> to select any block parent (block node, block export, or qdev) by one
>> unique name. The command fails, if name is ambiguous (i.e., match
>> several parents of different types). In future we want to rid of this
>> ambiguity.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> 
> We have numerous kinds of IDs, i.e. names chosen by the user than need
> to be unique, but only among the same kind.  For instance, you can't
> name two block nodes "foo", but you can name a block node, a block
> export, a qdev, and a network backend "foo".  Using the same ID for
> multiple things is of course a bad idea.  Permitting it was also a bad
> idea.
> 
> Your patch rectifies this design mistake, but only partially.  Consider:
> 
> * IDs need to be unique with their kind.  This is what we have now.  I
>    don't like it.
> 
> * IDs need to be unique among their kind and possibly some set of
>    additional kinds.  This is where your patch takes us.  I like it even
>    less, to be honest.
> 
> * IDs need to be unique, period.  This is what I'd like to have.
> 

I like it. Is it enough to write it so simple in deprecation doc? Or should
we still list all such ids we have in QEMU?

It may be something like:

Any kind of IDs you use to reference objects in QEMU must be unique, any
used ID must reference exactly one object. This includes, but is not limited
to qdev full and relative to "/peripheral/" paths, block-node and block-export
names.


-- 
Best regards,
Vladimir