Patchwork [v4,07/23] qdev: Allow device specification by qtree path for device_del

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

Comments

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

Allow to specify the device to be removed via device_del not only by ID
but also by its full or abbreviated qtree path. For this purpose,
qdev_find is introduced which combines walking the qtree with searching
for device IDs if required.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/qdev.c       |   75 ++++++++++++++++++++++++++++++++++++++++++++-----------
 qemu-monitor.hx |   10 +++---
 2 files changed, 65 insertions(+), 20 deletions(-)
Markus Armbruster - June 23, 2010, 9:37 a.m.
[cc: kraxel]
Jan Kiszka <jan.kiszka@web.de> writes:

> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> Allow to specify the device to be removed via device_del not only by ID
> but also by its full or abbreviated qtree path. For this purpose,
> qdev_find is introduced which combines walking the qtree with searching
> for device IDs if required.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

I like accepting paths there.

Your previous decision to abolish ID in qdev paths makes plain ID a
special case here instead of a relative path that happens to be short.

> ---
>  hw/qdev.c       |   75 ++++++++++++++++++++++++++++++++++++++++++++-----------
>  qemu-monitor.hx |   10 +++---
>  2 files changed, 65 insertions(+), 20 deletions(-)
>
> diff --git a/hw/qdev.c b/hw/qdev.c
> index ac450cf..2d1d171 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -39,7 +39,7 @@ DeviceInfo *device_info_list;
>  
>  static BusState *qbus_find_recursive(BusState *bus, const char *name,
>                                       const BusInfo *info);
> -static BusState *qbus_find(const char *path);
> +static BusState *qbus_find(const char *path, bool report_errors);
>  
>  /* Register a new device type.  */
>  void qdev_register(DeviceInfo *info)
> @@ -217,7 +217,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>      /* find bus */
>      path = qemu_opt_get(opts, "bus");
>      if (path != NULL) {
> -        bus = qbus_find(path);
> +        bus = qbus_find(path, true);
>          if (!bus) {
>              return NULL;
>          }
> @@ -475,7 +475,7 @@ static BusState *qbus_find_recursive(BusState *bus, const char *name,
>      return NULL;
>  }
>  
> -static DeviceState *qdev_find_recursive(BusState *bus, const char *id)
> +static DeviceState *qdev_find_id_recursive(BusState *bus, const char *id)
>  {
>      DeviceState *dev, *ret;
>      BusState *child;
> @@ -484,7 +484,7 @@ static DeviceState *qdev_find_recursive(BusState *bus, const char *id)
>          if (dev->id && strcmp(dev->id, id) == 0)
>              return dev;
>          QTAILQ_FOREACH(child, &dev->child_bus, sibling) {
> -            ret = qdev_find_recursive(child, id);
> +            ret = qdev_find_id_recursive(child, id);
>              if (ret) {
>                  return ret;
>              }
> @@ -590,7 +590,7 @@ static DeviceState *qbus_find_dev(BusState *bus, const char *elem)
>      return NULL;
>  }
>  
> -static BusState *qbus_find(const char *path)
> +static BusState *qbus_find(const char *path, bool report_errors)
>  {
>      DeviceState *dev;
>      BusState *bus = main_system_bus;
> @@ -600,7 +600,7 @@ static BusState *qbus_find(const char *path)
>      /* search for bus name recursively if path is not absolute */
>      if (path[0] != '/') {
>          bus = qbus_find_recursive(bus, path, NULL);
> -        if (!bus) {
> +        if (!bus && report_errors) {
>              qerror_report(QERR_BUS_NOT_FOUND, path);
>          }
>          return bus;
> @@ -623,12 +623,16 @@ static BusState *qbus_find(const char *path)
>          pos += len;
>          dev = qbus_find_dev(bus, elem);
>          if (!dev) {
> -            qerror_report(QERR_DEVICE_NOT_FOUND, elem);
> -            qbus_list_dev(bus);
> +            if (report_errors) {
> +                qerror_report(QERR_DEVICE_NOT_FOUND, elem);
> +                qbus_list_dev(bus);
> +            }
>              return NULL;
>          }
>          if (dev->num_child_bus == 0) {
> -            qerror_report(QERR_DEVICE_NO_BUS, elem);
> +            if (report_errors) {
> +                qerror_report(QERR_DEVICE_NO_BUS, elem);
> +            }
>              return NULL;
>          }
>  
> @@ -644,13 +648,55 @@ static BusState *qbus_find(const char *path)
>          pos += len;
>          bus = qbus_find_bus(dev, elem);
>          if (!bus) {
> -            qerror_report(QERR_BUS_NOT_FOUND, elem);
> -            qbus_list_bus(dev);
> +            if (report_errors) {
> +                qerror_report(QERR_BUS_NOT_FOUND, elem);
> +                qbus_list_bus(dev);
> +            }
>              return NULL;
>          }
>      }
>  }
>  
> +static DeviceState *qdev_find(const char *path)
> +{
> +    const char *dev_name;
> +    DeviceState *dev;
> +    char *bus_path;
> +    BusState *bus;
> +
> +    /* search for unique ID recursively if path is not absolute */
> +    if (path[0] != '/') {
> +        dev = qdev_find_id_recursive(main_system_bus, path);
> +        if (!dev) {
> +            qerror_report(QERR_DEVICE_NOT_FOUND, path);
> +        }
> +        return dev;
> +    }
> +
> +    dev_name = strrchr(path, '/') + 1;
> +
> +    bus_path = qemu_strdup(path);
> +    bus_path[dev_name - path] = 0;
> +
> +    bus = qbus_find(bus_path, false);
> +    qemu_free(bus_path);
> +    if (!bus) {
> +        /* retry with full path to generate correct error message */
> +        bus = qbus_find(path, true);
> +        if (!bus) {
> +            return NULL;
> +        }
> +        dev_name = "";
> +    }

Awkward.

What happens here is you hack off the last path component (because it
must be a device), then resolve the rest as bus, suppressing errors.  If
it fails, you resolve the complete path as bus without error
suppression.  Since the first try failed, the second try must surely run
into the same failure before it reaches the last path component.  So it
doesn't matter that the whole path is supposed to be a device, not a
bus.

This leads to suboptimal error messages for arguments that are actually
buses, not devices.  For instance, /i440FX-pcihost/pci.0/piix3-ide/ide.0
will try to resolve /i440FX-pcihost/pci.0/piix3-ide as bus, fail, and
report it as "Bus '' not found." (I think).

I don't like separate qdev path resolution for buses and devices.  Just
resolve it, and if what you got is not of the kind you want, report
that.

> +
> +    dev = qbus_find_dev(bus, dev_name);
> +    if (!dev) {
> +        qerror_report(QERR_DEVICE_NOT_FOUND, dev_name);
> +        qbus_list_dev(bus);
> +    }
> +    return dev;
> +}
> +
>  void qbus_create_inplace(BusState *bus, BusInfo *info,
>                           DeviceState *parent, const char *name)
>  {
[...]

Patch

diff --git a/hw/qdev.c b/hw/qdev.c
index ac450cf..2d1d171 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -39,7 +39,7 @@  DeviceInfo *device_info_list;
 
 static BusState *qbus_find_recursive(BusState *bus, const char *name,
                                      const BusInfo *info);
-static BusState *qbus_find(const char *path);
+static BusState *qbus_find(const char *path, bool report_errors);
 
 /* Register a new device type.  */
 void qdev_register(DeviceInfo *info)
@@ -217,7 +217,7 @@  DeviceState *qdev_device_add(QemuOpts *opts)
     /* find bus */
     path = qemu_opt_get(opts, "bus");
     if (path != NULL) {
-        bus = qbus_find(path);
+        bus = qbus_find(path, true);
         if (!bus) {
             return NULL;
         }
@@ -475,7 +475,7 @@  static BusState *qbus_find_recursive(BusState *bus, const char *name,
     return NULL;
 }
 
-static DeviceState *qdev_find_recursive(BusState *bus, const char *id)
+static DeviceState *qdev_find_id_recursive(BusState *bus, const char *id)
 {
     DeviceState *dev, *ret;
     BusState *child;
@@ -484,7 +484,7 @@  static DeviceState *qdev_find_recursive(BusState *bus, const char *id)
         if (dev->id && strcmp(dev->id, id) == 0)
             return dev;
         QTAILQ_FOREACH(child, &dev->child_bus, sibling) {
-            ret = qdev_find_recursive(child, id);
+            ret = qdev_find_id_recursive(child, id);
             if (ret) {
                 return ret;
             }
@@ -590,7 +590,7 @@  static DeviceState *qbus_find_dev(BusState *bus, const char *elem)
     return NULL;
 }
 
-static BusState *qbus_find(const char *path)
+static BusState *qbus_find(const char *path, bool report_errors)
 {
     DeviceState *dev;
     BusState *bus = main_system_bus;
@@ -600,7 +600,7 @@  static BusState *qbus_find(const char *path)
     /* search for bus name recursively if path is not absolute */
     if (path[0] != '/') {
         bus = qbus_find_recursive(bus, path, NULL);
-        if (!bus) {
+        if (!bus && report_errors) {
             qerror_report(QERR_BUS_NOT_FOUND, path);
         }
         return bus;
@@ -623,12 +623,16 @@  static BusState *qbus_find(const char *path)
         pos += len;
         dev = qbus_find_dev(bus, elem);
         if (!dev) {
-            qerror_report(QERR_DEVICE_NOT_FOUND, elem);
-            qbus_list_dev(bus);
+            if (report_errors) {
+                qerror_report(QERR_DEVICE_NOT_FOUND, elem);
+                qbus_list_dev(bus);
+            }
             return NULL;
         }
         if (dev->num_child_bus == 0) {
-            qerror_report(QERR_DEVICE_NO_BUS, elem);
+            if (report_errors) {
+                qerror_report(QERR_DEVICE_NO_BUS, elem);
+            }
             return NULL;
         }
 
@@ -644,13 +648,55 @@  static BusState *qbus_find(const char *path)
         pos += len;
         bus = qbus_find_bus(dev, elem);
         if (!bus) {
-            qerror_report(QERR_BUS_NOT_FOUND, elem);
-            qbus_list_bus(dev);
+            if (report_errors) {
+                qerror_report(QERR_BUS_NOT_FOUND, elem);
+                qbus_list_bus(dev);
+            }
             return NULL;
         }
     }
 }
 
+static DeviceState *qdev_find(const char *path)
+{
+    const char *dev_name;
+    DeviceState *dev;
+    char *bus_path;
+    BusState *bus;
+
+    /* search for unique ID recursively if path is not absolute */
+    if (path[0] != '/') {
+        dev = qdev_find_id_recursive(main_system_bus, path);
+        if (!dev) {
+            qerror_report(QERR_DEVICE_NOT_FOUND, path);
+        }
+        return dev;
+    }
+
+    dev_name = strrchr(path, '/') + 1;
+
+    bus_path = qemu_strdup(path);
+    bus_path[dev_name - path] = 0;
+
+    bus = qbus_find(bus_path, false);
+    qemu_free(bus_path);
+    if (!bus) {
+        /* retry with full path to generate correct error message */
+        bus = qbus_find(path, true);
+        if (!bus) {
+            return NULL;
+        }
+        dev_name = "";
+    }
+
+    dev = qbus_find_dev(bus, dev_name);
+    if (!dev) {
+        qerror_report(QERR_DEVICE_NOT_FOUND, dev_name);
+        qbus_list_dev(bus);
+    }
+    return dev;
+}
+
 void qbus_create_inplace(BusState *bus, BusInfo *info,
                          DeviceState *parent, const char *name)
 {
@@ -810,12 +856,11 @@  int do_device_add(Monitor *mon, const QDict *qdict, QObject **ret_data)
 
 int do_device_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
-    const char *id = qdict_get_str(qdict, "id");
+    const char *path = qdict_get_str(qdict, "device");
     DeviceState *dev;
 
-    dev = qdev_find_recursive(main_system_bus, id);
-    if (NULL == dev) {
-        qerror_report(QERR_DEVICE_NOT_FOUND, id);
+    dev = qdev_find(path);
+    if (!dev) {
         return -1;
     }
     return qdev_unplug(dev);
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index 9f62b94..0ea0555 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -703,7 +703,7 @@  EQMP
 
     {
         .name       = "device_del",
-        .args_type  = "id:s",
+        .args_type  = "device:s",
         .params     = "device",
         .help       = "remove device",
         .user_print = monitor_user_noop,
@@ -711,10 +711,10 @@  EQMP
     },
 
 STEXI
-@item device_del @var{id}
+@item device_del @var{device}
 @findex device_del
 
-Remove device @var{id}.
+Remove @var{device}, specified via its qtree path or unique ID.
 ETEXI
 SQMP
 device_del
@@ -724,11 +724,11 @@  Remove a device.
 
 Arguments:
 
-- "id": the device's ID (json-string)
+- "device": the device's qtree path or unique ID (json-string)
 
 Example:
 
--> { "execute": "device_del", "arguments": { "id": "net1" } }
+-> { "execute": "device_del", "arguments": { "device": "net1" } }
 <- { "return": {} }
 
 EQMP