Re: [Qemu-devel] [PATCH v4 07/23] qdev: Allow device specification by qtree path for device_del

[Markus Armbruster]

[cc: kraxel]
Jan Kiszka <address@hidden> writes:

> From: Jan Kiszka <address@hidden>
>
> 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 <address@hidden>

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)
>  {
[...]