Patchwork [v4,02/23] qdev: Restrict direct bus addressing via its name

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

Comments

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

We allow to address a bus only using its local name. This is ambiguous
but unfortunately so handy that people (specifically libvirt) will
likely complain if bus=pci.0 needs to be replaced with
bus=/i440FX-pcihost/pci.0 all over the place. So keep this for now but
drop at least support for starting a qtree walks with an abbreviated bus
name.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/qdev.c |   22 +++++++---------------
 1 files changed, 7 insertions(+), 15 deletions(-)
Markus Armbruster - June 23, 2010, 8:45 a.m.
Jan Kiszka <jan.kiszka@web.de> writes:

> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> We allow to address a bus only using its local name. This is ambiguous
> but unfortunately so handy that people (specifically libvirt) will
> likely complain if bus=pci.0 needs to be replaced with
> bus=/i440FX-pcihost/pci.0 all over the place. So keep this for now but
> drop at least support for starting a qtree walks with an abbreviated bus
> name.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

Again, affects only -device and device_add option bus.

Before, an argument started either with '/' (path anchored at root) or a
bus name (path anchored at that bus).

Now, an argument started either with '/' (path anchored at root) or it
is a bus name.

If we decide to add sane relative paths to qdev, i.e. paths anchored at
unique ID, we get a slight anomaly here:

  bus=a        bus name, not a relative path
  bus=a/b      relative path anchored at node with unique ID a.

Works for me.
Jan Kiszka - June 23, 2010, 10:17 a.m.
Markus Armbruster wrote:
> Jan Kiszka <jan.kiszka@web.de> writes:
> 
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> We allow to address a bus only using its local name. This is ambiguous
>> but unfortunately so handy that people (specifically libvirt) will
>> likely complain if bus=pci.0 needs to be replaced with
>> bus=/i440FX-pcihost/pci.0 all over the place. So keep this for now but
>> drop at least support for starting a qtree walks with an abbreviated bus
>> name.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> 
> Again, affects only -device and device_add option bus.
> 
> Before, an argument started either with '/' (path anchored at root) or a
> bus name (path anchored at that bus).
> 
> Now, an argument started either with '/' (path anchored at root) or it
> is a bus name.
> 
> If we decide to add sane relative paths to qdev, i.e. paths anchored at
> unique ID, we get a slight anomaly here:
> 
>   bus=a        bus name, not a relative path
>   bus=a/b      relative path anchored at node with unique ID a.
> 
> Works for me.

If we allow ID-anchoring, we are in ambiguous land again unless we
reject IDs that happen to match any bus name in the system. Even then,
the problem will be that the aliases based on bus names need to be
auto-generated (for backward compatibility) while IDs are user-assigned.
If the user comes first, auto-generation will innocently produce an ugly
conflict and we will either have to reject bus instantiation or live
with the conflict.

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

> Markus Armbruster wrote:
>> Jan Kiszka <jan.kiszka@web.de> writes:
>> 
>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>
>>> We allow to address a bus only using its local name. This is ambiguous
>>> but unfortunately so handy that people (specifically libvirt) will
>>> likely complain if bus=pci.0 needs to be replaced with
>>> bus=/i440FX-pcihost/pci.0 all over the place. So keep this for now but
>>> drop at least support for starting a qtree walks with an abbreviated bus
>>> name.
>>>
>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> 
>> Again, affects only -device and device_add option bus.
>> 
>> Before, an argument started either with '/' (path anchored at root) or a
>> bus name (path anchored at that bus).
>> 
>> Now, an argument started either with '/' (path anchored at root) or it
>> is a bus name.
>> 
>> If we decide to add sane relative paths to qdev, i.e. paths anchored at
>> unique ID, we get a slight anomaly here:
>> 
>>   bus=a        bus name, not a relative path
>>   bus=a/b      relative path anchored at node with unique ID a.
>> 
>> Works for me.
>
> If we allow ID-anchoring, we are in ambiguous land again unless we
> reject IDs that happen to match any bus name in the system. Even then,
> the problem will be that the aliases based on bus names need to be
> auto-generated (for backward compatibility) while IDs are user-assigned.
> If the user comes first, auto-generation will innocently produce an ugly
> conflict and we will either have to reject bus instantiation or live
> with the conflict.

No.

For backward compatibility, we need bus=a to be interpreted as bus name,
not as qdev path.  In other words, we overload bus to be either bus name
or qdev path.  It's a wart, but unavoidable unless we restrict bus to
bus names and add a separate option for paths.

As long as we require all qdev paths to be absolute, the wart is
relatively small.  The rule to resolve the overloading is simple: if the
value starts with '/', then it's a qdev path, else it's a bus name.

With relative qdev paths, the wart becomes more a bit more prominent.
The rule to resolve the overloading becomes: if the value contains '/',
it's a qdev path, else it's a bus name.

I can't see ambiguity here.

Patch

diff --git a/hw/qdev.c b/hw/qdev.c
index 7c4f039..c272c51 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -564,25 +564,17 @@  static DeviceState *qbus_find_dev(BusState *bus, char *elem)
 static BusState *qbus_find(const char *path)
 {
     DeviceState *dev;
-    BusState *bus;
+    BusState *bus = main_system_bus;
     char elem[128];
-    int pos, len;
+    int len, pos = 0;
 
-    /* find start element */
-    if (path[0] == '/') {
-        bus = main_system_bus;
-        pos = 0;
-    } else {
-        if (sscanf(path, "%127[^/]%n", elem, &len) != 1) {
-            assert(!path[0]);
-            elem[0] = len = 0;
-        }
-        bus = qbus_find_recursive(main_system_bus, elem, NULL);
+    /* search for bus name recursively if path is not absolute */
+    if (path[0] != '/') {
+        bus = qbus_find_recursive(bus, path, NULL);
         if (!bus) {
-            qerror_report(QERR_BUS_NOT_FOUND, elem);
-            return NULL;
+            qerror_report(QERR_BUS_NOT_FOUND, path);
         }
-        pos = len;
+        return bus;
     }
 
     for (;;) {