Patchwork [v2,02/15] qdev: Fix scanning across single-bus devices

login
register
mail settings
Submitter Jan Kiszka
Date May 22, 2010, 8:17 a.m.
Message ID <c375fba941d2c62025d5e60a85d4db2a301e30b1.1274516288.git.jan.kiszka@web.de>
Download mbox | patch
Permalink /patch/53239/
State New
Headers show

Comments

Jan Kiszka - May 22, 2010, 8:17 a.m.
From: Jan Kiszka <jan.kiszka@siemens.com>

As long as we allow /dev.1 as shortcut for /dev1/bus1, we also have to
make sure that /dev1/dev2 works for /dev1/bus1/dev2/bus2 - as long as
there is only one child bus per device.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 docs/qdev-device-use.txt |    4 ++++
 hw/qdev.c                |   12 +++++++++++-
 2 files changed, 15 insertions(+), 1 deletions(-)
Markus Armbruster - May 29, 2010, 7:38 a.m.
[cc: kraxel]

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

> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> As long as we allow /dev.1 as shortcut for /dev1/bus1, we also have to
> make sure that /dev1/dev2 works for /dev1/bus1/dev2/bus2 - as long as
> there is only one child bus per device.

We auto-root a path not starting with '/' via convention "first
component is ID then" (I like that).  We auto-complete a path ending
with a device when we want a bus (a bit too clever).  Auto-inserting bus
names in the middle is too clever by half!

Such cleverness can be convenient in the human monitor, but all it adds
to QMP is complexity.

I'm worried about possibly ambigous interpretation of paths.  Would be
bad if a path could name different node after we add something to the
tree.  I *think* your fine print makes that impossible, but it's not
exactly obvious.  Heck, I could be wrong.

Wouldn't it be better to put the convenience into the interactive
completion function?  That way we keep it out of QMP.

Subject is misleading, it's a feature, not a bug fix.
Jan Kiszka - May 29, 2010, 7:56 a.m.
Markus Armbruster wrote:
> [cc: kraxel]
> 
> Jan Kiszka <jan.kiszka@web.de> writes:
> 
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> As long as we allow /dev.1 as shortcut for /dev1/bus1, we also have to
>> make sure that /dev1/dev2 works for /dev1/bus1/dev2/bus2 - as long as
>> there is only one child bus per device.
> 
> We auto-root a path not starting with '/' via convention "first
> component is ID then" (I like that).  We auto-complete a path ending
> with a device when we want a bus (a bit too clever).  Auto-inserting bus
> names in the middle is too clever by half!
> 
> Such cleverness can be convenient in the human monitor, but all it adds
> to QMP is complexity.
> 
> I'm worried about possibly ambigous interpretation of paths.  Would be
> bad if a path could name different node after we add something to the
> tree.  I *think* your fine print makes that impossible, but it's not
> exactly obvious.  Heck, I could be wrong.

For the above example, that would mean a bus called 'dev2' would have to
be added to dev1. If that makes sense is one question. The other is why
this should be more broken than the case that this happens to a bus
specified at the end of some path?

> 
> Wouldn't it be better to put the convenience into the interactive
> completion function?  That way we keep it out of QMP.

That would leave user interfaces over QMP broken behind, or at least
complicate their implementations as they would have to expand the qtree
path on their own to achieve consistency.

> 
> Subject is misleading, it's a feature, not a bug fix.

Depends on the POV. For me it is a highly confusing inconsistency in the
way you can specify qtree paths. IMHO, we either have to drop this path
abbreviation or apply it to the whole path. As it's intention is
(according to my understanding) to ease the usage, an unintuitive
construction role is fairly counterproductive.

Jan
Gerd Hoffmann - May 31, 2010, 9:45 a.m.
On 05/29/10 09:38, Markus Armbruster wrote:
> [cc: kraxel]
>
> Jan Kiszka<jan.kiszka@web.de>  writes:
>
>> From: Jan Kiszka<jan.kiszka@siemens.com>
>>
>> As long as we allow /dev.1 as shortcut for /dev1/bus1, we also have to
>> make sure that /dev1/dev2 works for /dev1/bus1/dev2/bus2 - as long as
>> there is only one child bus per device.
>
> We auto-root a path not starting with '/' via convention "first
> component is ID then" (I like that).  We auto-complete a path ending
> with a device when we want a bus (a bit too clever).

... only if there is a single child bus only (i.e. it isn't ambiguous). 
  Which is the common case though.

> Auto-inserting bus
> names in the middle is too clever by half!

Yea.  I have to agree here.  It can only work reliably if you make sure 
all your devices have unique ids.  In which case there is no need to 
reference dev1 at all, you can just use the "first component is ID then" 
mechanism for dev2.

cheers,
   Gerd

Patch

diff --git a/docs/qdev-device-use.txt b/docs/qdev-device-use.txt
index f252c8e..9ac1fa1 100644
--- a/docs/qdev-device-use.txt
+++ b/docs/qdev-device-use.txt
@@ -20,6 +20,10 @@  bus named pci.0.  To put a FOO device into its slot 4, use -device
 FOO,bus=/i440FX-pcihost/pci.0,addr=4.  The abbreviated form bus=pci.0
 also works as long as the bus name is unique.
 
+Furthermore, if a device only hosts a single bus, the bus name can be
+omitted in the path.  Example: /i440FX-pcihost/PIIX3 abbreviates
+/i440FX-pcihost/pci.0/PIIX3/isa.0 as none of the buses has siblings.
+
 Note: the USB device address can't be controlled at this time.
 
 === Block Devices ===
diff --git a/hw/qdev.c b/hw/qdev.c
index aa2ce01..2e50531 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -557,7 +557,7 @@  static DeviceState *qbus_find_dev(BusState *bus, char *elem)
 
 static BusState *qbus_find(const char *path)
 {
-    DeviceState *dev;
+    DeviceState *dev, *next_dev;
     BusState *bus;
     char elem[128];
     int pos, len;
@@ -603,6 +603,7 @@  static BusState *qbus_find(const char *path)
             return NULL;
         }
 
+search_dev_bus:
         assert(path[pos] == '/' || !path[pos]);
         while (path[pos] == '/') {
             pos++;
@@ -633,6 +634,15 @@  static BusState *qbus_find(const char *path)
         pos += len;
         bus = qbus_find_bus(dev, elem);
         if (!bus) {
+            if (dev->num_child_bus == 1) {
+                /* Last element might have been a short-cut to a device on
+                 * the single child bus of the parent device. */
+                next_dev = qbus_find_dev(QTAILQ_FIRST(&dev->child_bus), elem);
+                if (next_dev) {
+                    dev = next_dev;
+                    goto search_dev_bus;
+                }
+            }
             qerror_report(QERR_BUS_NOT_FOUND, elem);
             if (!monitor_cur_is_qmp()) {
                 qbus_list_bus(dev);