[Qemu-devel] [PATCH] qdev: Assign a default device ID when none is provided.

[Qemu-devel] [PATCH] qdev: Assign a default device ID when none is provided.

[Hani Benhabiles]

This would allow a user to be able to refer to the device when using commands
like device_del.

Signed-off-by: Hani Benhabiles <kroosec@gmail.com>
---
 qdev-monitor.c | 64 +++++++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 50 insertions(+), 14 deletions(-)

diff --git a/qdev-monitor.c b/qdev-monitor.c
index dc37a43..64b2b22 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -451,11 +451,54 @@ static BusState *qbus_find(const char *path)
     }
 }
 
+static int qbus_device_count(const BusState *bus, const char *typename)
+{
+    BusChild *kid;
+    int count = 0;
+
+    QTAILQ_FOREACH(kid, &bus->children, sibling) {
+        DeviceState *dev = kid->child;
+        BusState *dev_child;
+
+        if (!strcmp(typename, object_get_typename(OBJECT(dev)))) {
+            count++;
+        }
+
+        QLIST_FOREACH(dev_child, &dev->child_bus, sibling) {
+            count += qbus_device_count(dev_child, typename);
+        }
+    }
+
+    return count;
+}
+
+static gchar *assign_device_name(const char *typename, const DeviceState *dev)
+{
+    BusState *bus;
+    gchar *new_id;
+    int count = 0;
+
+    bus = sysbus_get_default();
+    count += qbus_device_count(bus, typename);
+
+    while (1) {
+        new_id = g_strdup_printf("%s.%d", typename, count - 1);
+        /* To not use an existing ID, if the user previously specified it. */
+        if (!qdev_find_recursive(sysbus_get_default(), new_id)) {
+            break;
+        }
+        count++;
+        g_free(new_id);
+    }
+
+    return new_id;
+}
+
 DeviceState *qdev_device_add(QemuOpts *opts)
 {
     ObjectClass *oc;
     DeviceClass *dc;
-    const char *driver, *path, *id;
+    const char *driver, *path;
     DeviceState *dev;
     BusState *bus = NULL;
     Error *err = NULL;
@@ -522,25 +565,18 @@ DeviceState *qdev_device_add(QemuOpts *opts)
         qdev_set_parent_bus(dev, bus);
     }
 
-    id = qemu_opts_id(opts);
-    if (id) {
-        dev->id = id;
+    dev->id = qemu_opts_id(opts);
+    if (!dev->id) {
+        qemu_opts_set_id(opts, assign_device_name(driver, dev));
+        dev->id = qemu_opts_id(opts);
     }
     if (qemu_opt_foreach(opts, set_property, dev, 1) != 0) {
         object_unparent(OBJECT(dev));
         object_unref(OBJECT(dev));
         return NULL;
     }
-    if (dev->id) {
-        object_property_add_child(qdev_get_peripheral(), dev->id,
-                                  OBJECT(dev), NULL);
-    } else {
-        static int anon_count;
-        gchar *name = g_strdup_printf("device[%d]", anon_count++);
-        object_property_add_child(qdev_get_peripheral_anon(), name,
-                                  OBJECT(dev), NULL);
-        g_free(name);
-    }
+    object_property_add_child(qdev_get_peripheral(), dev->id, OBJECT(dev),
+                              NULL);
     object_property_set_bool(OBJECT(dev), true, "realized", &err);
     if (err != NULL) {
         qerror_report_err(err);
-- 
1.8.3.2

Re: [Qemu-devel] [PATCH] qdev: Assign a default device ID when none is provided.

[Markus Armbruster]

Hani Benhabiles <kroosec@gmail.com> writes:

> This would allow a user to be able to refer to the device when using commands
> like device_del.
>
> Signed-off-by: Hani Benhabiles <kroosec@gmail.com>

No.

Device IDs belong to the user.  Any IDs the system picks automatically
can collide with the user's IDs.

Management applications assume that they can pick any ID they want.
Your patch can introduce ID collisions, and thus make existing
configurations fail.

If I remember correctly, a few legacy convenience options pick IDs for
historical reasons.  If you use them, you need to be aware of the IDs
they pick.  Management applications shouldn't use them.

We've discussed this a couple of times already, by the way.

Re: [Qemu-devel] [PATCH] qdev: Assign a default device ID when none is provided.

[Hani Benhabiles]

On Wed, Jan 08, 2014 at 08:36:06AM +0100, Markus Armbruster wrote:
> Hani Benhabiles <kroosec@gmail.com> writes:
> 
> > This would allow a user to be able to refer to the device when using commands
> > like device_del.
> >
> > Signed-off-by: Hani Benhabiles <kroosec@gmail.com>
> 
> No.
> 
> Device IDs belong to the user.  Any IDs the system picks automatically
> can collide with the user's IDs.
> 
> Management applications assume that they can pick any ID they want.
> Your patch can introduce ID collisions, and thus make existing
> configurations fail.
> 

How can it lead to ID collisions ?

For this reason, the loop in assign_device_name() specifically check that the ID
doesn't exist already and uses the next value if it does.

How would something like:
(qemu) device_add virtio-net-pci
==> ID: virtio-net-pci.0

Be more problematic than:
(qemu) device_add virtio-net-pci,id=virtio-net-pci.0

> If I remember correctly, a few legacy convenience options pick IDs for
> historical reasons.  If you use them, you need to be aware of the IDs
> they pick.  Management applications shouldn't use them.
> 
> We've discussed this a couple of times already, by the way.

Re: [Qemu-devel] [PATCH] qdev: Assign a default device ID when none is provided.

[Paolo Bonzini]

Il 08/01/2014 18:17, Hani Benhabiles ha scritto:
> For this reason, the loop in assign_device_name() specifically check that the ID
> doesn't exist already and uses the next value if it does.
> 
> How would something like:
> (qemu) device_add virtio-net-pci
> ==> ID: virtio-net-pci.0
> 
> Be more problematic than:
> (qemu) device_add virtio-net-pci,id=virtio-net-pci.0

(qemu) device_add virtio-net-pci
(qemu) device_add virtio-net-pci,id=virtio-net-pci.0

works without your patches, fails with them (IIUC).

Paolo

Re: [Qemu-devel] [PATCH] qdev: Assign a default device ID when none is provided.

[Hani Benhabiles]

On Wed, Jan 08, 2014 at 06:34:01PM +0100, Paolo Bonzini wrote:
> Il 08/01/2014 18:17, Hani Benhabiles ha scritto:
> > For this reason, the loop in assign_device_name() specifically check that the ID
> > doesn't exist already and uses the next value if it does.
> > 
> > How would something like:
> > (qemu) device_add virtio-net-pci
> > ==> ID: virtio-net-pci.0
> > 
> > Be more problematic than:
> > (qemu) device_add virtio-net-pci,id=virtio-net-pci.0
> 
> (qemu) device_add virtio-net-pci
> (qemu) device_add virtio-net-pci,id=virtio-net-pci.0
> 
> works without your patches, fails with them (IIUC).

It would fail with a clear and descriptive message (Duplicate ID) which is no
different than when a user uses tries to do so manually.

Without the patch, how does a user unplug a device for which he didn't specify
an ID (eg. forgotten, added quickly...)

It is even more confusing with other commands like info networks.

(qemu) device_add virtio-net-pci
(qemu) device_add virtio-net-pci,id=foo
(qemu) info network 
virtio-net-pci.0: index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:56
foo: index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:57
(qemu) device_del foo
(qemu) device_del virtio-net-pci.0
Device 'virtio-net-pci.0' not found

If the naming scheme is an issue, could something like starting it with one or
two underscores better suited ?

Cheers,

Hani.

Re: [Qemu-devel] [PATCH] qdev: Assign a default device ID when none is provided.

[Paolo Bonzini]

Il 09/01/2014 19:18, Hani Benhabiles ha scritto:
> On Wed, Jan 08, 2014 at 06:34:01PM +0100, Paolo Bonzini wrote:
>> Il 08/01/2014 18:17, Hani Benhabiles ha scritto:
>>> For this reason, the loop in assign_device_name() specifically check that the ID
>>> doesn't exist already and uses the next value if it does.
>>>
>>> How would something like:
>>> (qemu) device_add virtio-net-pci
>>> ==> ID: virtio-net-pci.0
>>>
>>> Be more problematic than:
>>> (qemu) device_add virtio-net-pci,id=virtio-net-pci.0
>>
>> (qemu) device_add virtio-net-pci
>> (qemu) device_add virtio-net-pci,id=virtio-net-pci.0
>>
>> works without your patches, fails with them (IIUC).
> 
> It would fail with a clear and descriptive message (Duplicate ID) which is no
> different than when a user uses tries to do so manually.

It would still be a regression.

> Without the patch, how does a user unplug a device for which he didn't specify
> an ID (eg. forgotten, added quickly...)

He doesn't. :)

> It is even more confusing with other commands like info networks.
> 
> (qemu) device_add virtio-net-pci
> (qemu) device_add virtio-net-pci,id=foo
> (qemu) info network 
> virtio-net-pci.0: index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:56
> foo: index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:57
> (qemu) device_del foo
> (qemu) device_del virtio-net-pci.0
> Device 'virtio-net-pci.0' not found

This could be considered a bug in "info network" too.  You might use a
different template such as "unnamed virtio-net-pci #1" in net/net.c's
assign_name.

Paolo

> If the naming scheme is an issue, could something like starting it with one or
> two underscores better suited ?
> 
> Cheers,
> 
> Hani.
> 
> 

Re: [Qemu-devel] [PATCH] qdev: Assign a default device ID when none is provided.

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 09/01/2014 19:18, Hani Benhabiles ha scritto:
>> On Wed, Jan 08, 2014 at 06:34:01PM +0100, Paolo Bonzini wrote:
>>> Il 08/01/2014 18:17, Hani Benhabiles ha scritto:
>>>> For this reason, the loop in assign_device_name() specifically
>>>> check that the ID
>>>> doesn't exist already and uses the next value if it does.
>>>>
>>>> How would something like:
>>>> (qemu) device_add virtio-net-pci
>>>> ==> ID: virtio-net-pci.0
>>>>
>>>> Be more problematic than:
>>>> (qemu) device_add virtio-net-pci,id=virtio-net-pci.0
>>>
>>> (qemu) device_add virtio-net-pci
>>> (qemu) device_add virtio-net-pci,id=virtio-net-pci.0
>>>
>>> works without your patches, fails with them (IIUC).
>> 
>> It would fail with a clear and descriptive message (Duplicate ID) which is no
>> different than when a user uses tries to do so manually.
>
> It would still be a regression.
>
>> Without the patch, how does a user unplug a device for which he didn't specify
>> an ID (eg. forgotten, added quickly...)
>
> He doesn't. :)
>
>> It is even more confusing with other commands like info networks.
>> 
>> (qemu) device_add virtio-net-pci
>> (qemu) device_add virtio-net-pci,id=foo
>> (qemu) info network 
>> virtio-net-pci.0:
>> index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:56
>> foo: index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:57
>> (qemu) device_del foo
>> (qemu) device_del virtio-net-pci.0
>> Device 'virtio-net-pci.0' not found
>
> This could be considered a bug in "info network" too.  You might use a
> different template such as "unnamed virtio-net-pci #1" in net/net.c's
> assign_name.
>
> Paolo
>
>> If the naming scheme is an issue, could something like starting it with one or
>> two underscores better suited ?

In the design of qdev, the path in the qtree is the address that always
works, and IDs are abbreviations the user can define for his
convenience.  Unfortunately, we botched qtree paths so badly, they're
effectively unusable.  We consequently never made them available as
command arguments.  They only occur as values of the bus property, if I
remember correctly.

One possible recovery plan is to give up on paths, and auto-generate
IDs.

Another one is to ditch the unusable legacy qtree paths, and adopt the
"real" paths instead, namely the QOM paths.  I like this plan better.

Commands taking a device ID could be extended to take a path in the QOM
graph instead of an ID.

In the human monitor, it could perhaps work like this:

    IDs consist of letters, digits, '-', '.', '_', starting with a
    letter (see id_wellformed())
    
    If the argument doesn't contain '/', interpret it as ID.
    Else, if it starts with '/', interpret it as QOM path anchored at
    "the root" (which needs to be defined).
    Else, split it at the first '/', and interpret the prefix as ID, and
    the suffix as as QOM path anchored at whatever has that ID.

Requires means to inspect the QOM graph to be usable.

Re: [Qemu-devel] [PATCH] qdev: Assign a default device ID when none is provided.

[Andreas Färber]

Am 10.01.2014 10:09, schrieb Markus Armbruster:
> Commands taking a device ID could be extended to take a path in the QOM
> graph instead of an ID.
> 
> In the human monitor, it could perhaps work like this:
> 
>     IDs consist of letters, digits, '-', '.', '_', starting with a
>     letter (see id_wellformed())
>     
>     If the argument doesn't contain '/', interpret it as ID.
>     Else, if it starts with '/', interpret it as QOM path anchored at
>     "the root" (which needs to be defined).
>     Else, split it at the first '/', and interpret the prefix as ID, and
>     the suffix as as QOM path anchored at whatever has that ID.
> 
> Requires means to inspect the QOM graph to be usable.

qom-list is available for that purpose today.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH] qdev: Assign a default device ID when none is provided.

[Markus Armbruster]

Andreas Färber <afaerber@suse.de> writes:

> Am 10.01.2014 10:09, schrieb Markus Armbruster:
>> Commands taking a device ID could be extended to take a path in the QOM
>> graph instead of an ID.
>> 
>> In the human monitor, it could perhaps work like this:
>> 
>>     IDs consist of letters, digits, '-', '.', '_', starting with a
>>     letter (see id_wellformed())
>>     
>>     If the argument doesn't contain '/', interpret it as ID.
>>     Else, if it starts with '/', interpret it as QOM path anchored at
>>     "the root" (which needs to be defined).
>>     Else, split it at the first '/', and interpret the prefix as ID, and
>>     the suffix as as QOM path anchored at whatever has that ID.
>> 
>> Requires means to inspect the QOM graph to be usable.
>
> qom-list is available for that purpose today.

We'd need something in the human monitor, too.