Patchwork [v4,01/23] qdev: Rework qtree path abbreviations

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

Comments

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

Path abbreviations suffer from the inconsistency that bus names can only
be omitted at the end of a path. Drop this special rule, and also remove
the now obsolete QERR_DEVICE_MULTIPLE_BUSSES.

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

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

> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> Path abbreviations suffer from the inconsistency that bus names can only
> be omitted at the end of a path. Drop this special rule, and also remove
> the now obsolete QERR_DEVICE_MULTIPLE_BUSSES.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

Affects only -device and device_add option bus.  I support this.

> ---
>  hw/qdev.c |   22 ++++------------------
>  qerror.c  |    4 ----
>  qerror.h  |    3 ---
>  3 files changed, 4 insertions(+), 25 deletions(-)
>
> diff --git a/hw/qdev.c b/hw/qdev.c
> index 61f999c..7c4f039 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -608,32 +608,18 @@ static BusState *qbus_find(const char *path)
>              }
>              return NULL;
>          }
> +        if (dev->num_child_bus == 0) {
> +            qerror_report(QERR_DEVICE_NO_BUS, elem);
> +            return NULL;
> +        }

I'd kill this check as well.  The QERR_BUS_NOT_FOUND further down
suffices.

>  
>          assert(path[pos] == '/' || !path[pos]);
>          while (path[pos] == '/') {
>              pos++;
>          }
> -        if (path[pos] == '\0') {
> -            /* last specified element is a device.  If it has exactly
> -             * one child bus accept it nevertheless */
> -            switch (dev->num_child_bus) {
> -            case 0:
> -                qerror_report(QERR_DEVICE_NO_BUS, elem);
> -                return NULL;
> -            case 1:
> -                return QLIST_FIRST(&dev->child_bus);
> -            default:
> -                qerror_report(QERR_DEVICE_MULTIPLE_BUSSES, elem);
> -                if (!monitor_cur_is_qmp()) {
> -                    qbus_list_bus(dev);
> -                }
> -                return NULL;
> -            }
> -        }
>  
>          /* find bus */
>          if (sscanf(path+pos, "%127[^/]%n", elem, &len) != 1) {
> -            assert(0);
>              elem[0] = len = 0;
>          }
>          pos += len;
[...]
Markus Armbruster - June 23, 2010, 9:32 a.m.
Hmm, what happens if the path denotes a device instead of a bus?

qemu-system-x86_64: -device e1000,bus=/i440FX-pcihost: Bus '' not found

Not so nice.

Patch

diff --git a/hw/qdev.c b/hw/qdev.c
index 61f999c..7c4f039 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -608,32 +608,18 @@  static BusState *qbus_find(const char *path)
             }
             return NULL;
         }
+        if (dev->num_child_bus == 0) {
+            qerror_report(QERR_DEVICE_NO_BUS, elem);
+            return NULL;
+        }
 
         assert(path[pos] == '/' || !path[pos]);
         while (path[pos] == '/') {
             pos++;
         }
-        if (path[pos] == '\0') {
-            /* last specified element is a device.  If it has exactly
-             * one child bus accept it nevertheless */
-            switch (dev->num_child_bus) {
-            case 0:
-                qerror_report(QERR_DEVICE_NO_BUS, elem);
-                return NULL;
-            case 1:
-                return QLIST_FIRST(&dev->child_bus);
-            default:
-                qerror_report(QERR_DEVICE_MULTIPLE_BUSSES, elem);
-                if (!monitor_cur_is_qmp()) {
-                    qbus_list_bus(dev);
-                }
-                return NULL;
-            }
-        }
 
         /* find bus */
         if (sscanf(path+pos, "%127[^/]%n", elem, &len) != 1) {
-            assert(0);
             elem[0] = len = 0;
         }
         pos += len;
diff --git a/qerror.c b/qerror.c
index 44d0bf8..786b5dc 100644
--- a/qerror.c
+++ b/qerror.c
@@ -77,10 +77,6 @@  static const QErrorStringTable qerror_table[] = {
         .desc      = "Device '%(device)' is locked",
     },
     {
-        .error_fmt = QERR_DEVICE_MULTIPLE_BUSSES,
-        .desc      = "Device '%(device)' has multiple child busses",
-    },
-    {
         .error_fmt = QERR_DEVICE_NOT_ACTIVE,
         .desc      = "Device '%(device)' has not been activated by the guest",
     },
diff --git a/qerror.h b/qerror.h
index 77ae574..88474fb 100644
--- a/qerror.h
+++ b/qerror.h
@@ -73,9 +73,6 @@  QError *qobject_to_qerror(const QObject *obj);
 #define QERR_DEVICE_LOCKED \
     "{ 'class': 'DeviceLocked', 'data': { 'device': %s } }"
 
-#define QERR_DEVICE_MULTIPLE_BUSSES \
-    "{ 'class': 'DeviceMultipleBusses', 'data': { 'device': %s } }"
-
 #define QERR_DEVICE_NOT_ACTIVE \
     "{ 'class': 'DeviceNotActive', 'data': { 'device': %s } }"