Patchwork [v4,03/23] qdev: Drop ID matching from qtree paths

login
register
mail settings
Submitter Jan Kiszka
Date June 15, 2010, 10:38 p.m.
Message ID <fce0e55f5bf02172957365f5674e89a4d64257e4.1276641524.git.jan.kiszka@web.de>
Download mbox | patch
Permalink /patch/55811/
State New
Headers show

Comments

Jan Kiszka - June 15, 2010, 10:38 p.m.
From: Jan Kiszka <jan.kiszka@siemens.com>

Device IDs may conflict with device names or aliases. From now on we
only accept them outside qtree paths. This also makes dumping IDs in
qbus_list_dev/bus obsolete.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/qdev.c |   16 ++++------------
 1 files changed, 4 insertions(+), 12 deletions(-)
Markus Armbruster - June 23, 2010, 8:55 a.m.
[cc: kraxel]

Jan Kiszka <jan.kiszka@web.de> writes:

> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> Device IDs may conflict with device names or aliases. From now on we
> only accept them outside qtree paths. This also makes dumping IDs in
> qbus_list_dev/bus obsolete.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

I don't like this at all.

1. Specific problem:

   With the current code, multiple devices with the same driver work
   only if I take care: addressing by driver name gets me only the
   first, so I better set suitable IDs.

   With your patch, multiple devices with the same driver don't work, no
   matter what I do.

2. General principle:

   When I set an ID, I want the system to accept that ID in all contexts
   where it makes sense.  Ambiguity created by badly chosen IDs is *my*
   problem.
Jan Kiszka - June 23, 2010, 10:17 a.m.
Markus Armbruster wrote:
> [cc: kraxel]
> 
> Jan Kiszka <jan.kiszka@web.de> writes:
> 
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> Device IDs may conflict with device names or aliases. From now on we
>> only accept them outside qtree paths. This also makes dumping IDs in
>> qbus_list_dev/bus obsolete.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> 
> I don't like this at all.
> 
> 1. Specific problem:
> 
>    With the current code, multiple devices with the same driver work
>    only if I take care: addressing by driver name gets me only the
>    first, so I better set suitable IDs.
> 
>    With your patch, multiple devices with the same driver don't work, no
>    matter what I do.

[ you already found how this is resolved ]

> 
> 2. General principle:
> 
>    When I set an ID, I want the system to accept that ID in all contexts
>    where it makes sense.  Ambiguity created by badly chosen IDs is *my*
>    problem.

I disagree. IMO, QEMU should support the user to avoid such subtle
shadowing.

Jan
Markus Armbruster - June 23, 2010, 11:38 a.m.
Jan Kiszka <jan.kiszka@siemens.com> writes:

> Markus Armbruster wrote:
>> [cc: kraxel]
>> 
>> Jan Kiszka <jan.kiszka@web.de> writes:
>> 
>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>
>>> Device IDs may conflict with device names or aliases. From now on we
>>> only accept them outside qtree paths. This also makes dumping IDs in
>>> qbus_list_dev/bus obsolete.
>>>
>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> 
>> I don't like this at all.
>> 
>> 1. Specific problem:
>> 
>>    With the current code, multiple devices with the same driver work
>>    only if I take care: addressing by driver name gets me only the
>>    first, so I better set suitable IDs.
>> 
>>    With your patch, multiple devices with the same driver don't work, no
>>    matter what I do.
>
> [ you already found how this is resolved ]

Yes, but I don't like that the intermediate state is broken, and I don't
like the final state.

>> 2. General principle:
>> 
>>    When I set an ID, I want the system to accept that ID in all contexts
>>    where it makes sense.  Ambiguity created by badly chosen IDs is *my*
>>    problem.
>
> I disagree. IMO, QEMU should support the user to avoid such subtle
> shadowing.

IDs let me refer to my devices without having to look up ever-changing
instance numbers.  This is especially relevant when I want to do refer
to buses without using (possibly ambigious) bus names.

I want to be able to say bus=scsi-ctrl.2/scsi.0 instead of
/i440FX-pcihost/pci.0/lsi53c895a.<ever-changing-number>/scsi.0

That's much more valuable to me than a well-meant attempt at protecting
me from choosing IDs badly.

Throw in multiple PCI buses, and a "no IDs in qdev paths" law becomes
even more annoying.
Jan Kiszka - June 23, 2010, 12:15 p.m.
Markus Armbruster wrote:
> Jan Kiszka <jan.kiszka@siemens.com> writes:
> 
>> Markus Armbruster wrote:
>>> [cc: kraxel]
>>>
>>> Jan Kiszka <jan.kiszka@web.de> writes:
>>>
>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>
>>>> Device IDs may conflict with device names or aliases. From now on we
>>>> only accept them outside qtree paths. This also makes dumping IDs in
>>>> qbus_list_dev/bus obsolete.
>>>>
>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>> I don't like this at all.
>>>
>>> 1. Specific problem:
>>>
>>>    With the current code, multiple devices with the same driver work
>>>    only if I take care: addressing by driver name gets me only the
>>>    first, so I better set suitable IDs.
>>>
>>>    With your patch, multiple devices with the same driver don't work, no
>>>    matter what I do.
>> [ you already found how this is resolved ]
> 
> Yes, but I don't like that the intermediate state is broken, and I don't
> like the final state.
> 
>>> 2. General principle:
>>>
>>>    When I set an ID, I want the system to accept that ID in all contexts
>>>    where it makes sense.  Ambiguity created by badly chosen IDs is *my*
>>>    problem.
>> I disagree. IMO, QEMU should support the user to avoid such subtle
>> shadowing.
> 
> IDs let me refer to my devices without having to look up ever-changing
> instance numbers.  This is especially relevant when I want to do refer
> to buses without using (possibly ambigious) bus names.
> 
> I want to be able to say bus=scsi-ctrl.2/scsi.0 instead of
> /i440FX-pcihost/pci.0/lsi53c895a.<ever-changing-number>/scsi.0

It would once be something like /i440FX-pcihost/pci.0/@04.0/scsi.0 (with
optional extension of '@04.0' to '@04.0:lsi53c895a.0' for more verbose
interactive use), so at least no volatile naming here. But letting a
qdev path start with an ID can of course shorten things, specifically on
the command line where we have no completion.

> 
> That's much more valuable to me than a well-meant attempt at protecting
> me from choosing IDs badly.
> 
> Throw in multiple PCI buses, and a "no IDs in qdev paths" law becomes
> even more annoying.

No IDs _inside_ qdev paths, please. Letting them start is imaginable if
we find proper rules how to resolve the possible conflicts with
auto-generated bus aliases ('pci.0' = '/i440FX-pcihost/pci.0').

Let's look at it as Paul suggested: relative qdev path beginnings are
aliases of the corresponding absolute bits. When registering a device
with an ID, we also register an alias 'ID' = '/absolute/path/to/device'.
Here we can (and already do) reject the registration if 'ID' is not
unique. When registering a bus, we would add an alias 'busname' =
'/absolute/path/to/bus'. This can already fail due to the fact that bus
names need not be unique. But it could also fail if some silly guy
registered a device with the same ID before.

Now what to do? Register the alias nevertheless and select whatever
comes first in the alias list, possibly creating instable paths again?
Or reject the bus registration (i.e. the registration of the parent
device? Or silently skip the failing alias registration (which would
mean having no such alias at all once the other device gets hot-removed)?

Jan

Patch

diff --git a/hw/qdev.c b/hw/qdev.c
index c272c51..aa25155 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -497,8 +497,7 @@  static void qbus_list_bus(DeviceState *dev)
     BusState *child;
     const char *sep = " ";
 
-    error_printf("child busses at \"%s\":",
-                 dev->id ? dev->id : dev->info->name);
+    error_printf("child busses at \"%s\":", dev->info->name);
     QLIST_FOREACH(child, &dev->child_bus, sibling) {
         error_printf("%s\"%s\"", sep, child->name);
         sep = ", ";
@@ -514,8 +513,6 @@  static void qbus_list_dev(BusState *bus)
     error_printf("devices at \"%s\":", bus->name);
     QLIST_FOREACH(dev, &bus->children, sibling) {
         error_printf("%s\"%s\"", sep, dev->info->name);
-        if (dev->id)
-            error_printf("/\"%s\"", dev->id);
         sep = ", ";
     }
     error_printf("\n");
@@ -539,15 +536,10 @@  static DeviceState *qbus_find_dev(BusState *bus, char *elem)
 
     /*
      * try to match in order:
-     *   (1) instance id, if present
-     *   (2) driver name
-     *   (3) driver alias, if present
+     *   (1) driver name
+     *   (2) driver alias, if present
      */
-    QLIST_FOREACH(dev, &bus->children, sibling) {
-        if (dev->id  &&  strcmp(dev->id, elem) == 0) {
-            return dev;
-        }
-    }
+
     QLIST_FOREACH(dev, &bus->children, sibling) {
         if (strcmp(dev->info->name, elem) == 0) {
             return dev;