Re: [Qemu-devel] [PATCH v4 12/23] monitor: Add completion for qdev paths

[Markus Armbruster <armbru@redhat.com>]

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

> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> Implement monitor command line completion for device tree paths. The
> first user are device_add ('bus' option) and device_del.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  hw/qdev.c       |   19 +++++----
>  hw/qdev.h       |    3 +
>  monitor.c       |  115 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  qemu-monitor.hx |    4 +-
>  4 files changed, 129 insertions(+), 12 deletions(-)
>
> diff --git a/hw/qdev.c b/hw/qdev.c
> index 466d8d5..dbc5b10 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -39,7 +39,6 @@ DeviceInfo *device_info_list;
>  
>  static BusState *qbus_find_recursive(BusState *bus, const char *name,
>                                       const BusInfo *info);
> -static BusState *qbus_find(const char *path, bool report_errors);
>  
>  /* Register a new device type.  */
>  void qdev_register(DeviceInfo *info)
> @@ -514,7 +513,7 @@ static DeviceState *qdev_find_id_recursive(BusState *bus, const char *id)
>      return qdev_iterate_recursive(bus, find_id_callback, (void *)id);
>  }
>  
> -static int qdev_instance_no(DeviceState *dev)
> +int qdev_instance_no(DeviceState *dev)
>  {
>      struct DeviceState *sibling;
>      int instance = 0;
> @@ -611,7 +610,7 @@ static DeviceState *qbus_find_dev(BusState *bus, const char *elem)
>      return NULL;
>  }
>  
> -static BusState *qbus_find(const char *path, bool report_errors)
> +BusState *qbus_find(const char *path, bool report_errors)
>  {
>      DeviceState *dev;
>      BusState *bus = main_system_bus;
> @@ -678,7 +677,7 @@ static BusState *qbus_find(const char *path, bool report_errors)
>      }
>  }
>  
> -static DeviceState *qdev_find(const char *path)
> +DeviceState *qdev_find(const char *path, bool report_errors)
>  {
>      const char *dev_name;
>      DeviceState *dev;
> @@ -688,7 +687,7 @@ static DeviceState *qdev_find(const char *path)
>      /* search for unique ID recursively if path is not absolute */
>      if (path[0] != '/') {
>          dev = qdev_find_id_recursive(main_system_bus, path);
> -        if (!dev) {
> +        if (!dev && report_errors) {
>              qerror_report(QERR_DEVICE_NOT_FOUND, path);
>          }
>          return dev;
> @@ -702,8 +701,10 @@ static DeviceState *qdev_find(const char *path)
>      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 (report_errors) {
> +            /* retry with full path to generate correct error message */
> +            bus = qbus_find(path, report_errors);
> +        }
>          if (!bus) {
>              return NULL;
>          }
> @@ -711,7 +712,7 @@ static DeviceState *qdev_find(const char *path)
>      }
>  
>      dev = qbus_find_dev(bus, dev_name);
> -    if (!dev) {
> +    if (!dev && report_errors) {
>          qerror_report(QERR_DEVICE_NOT_FOUND, dev_name);
>          qbus_list_dev(bus);
>      }
> @@ -880,7 +881,7 @@ int do_device_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
>      const char *path = qdict_get_str(qdict, "device");
>      DeviceState *dev;
>  
> -    dev = qdev_find(path);
> +    dev = qdev_find(path, true);
>      if (!dev) {
>          return -1;
>      }
> diff --git a/hw/qdev.h b/hw/qdev.h
> index 111c876..59159a0 100644
> --- a/hw/qdev.h
> +++ b/hw/qdev.h
> @@ -171,6 +171,8 @@ CharDriverState *qdev_init_chardev(DeviceState *dev);
>  BusState *qdev_get_parent_bus(DeviceState *dev);
>  void *qdev_iterate_recursive(BusState *bus, qdev_iteratefn callback,
>                               void *opaque);
> +DeviceState *qdev_find(const char *path, bool report_errors);
> +int qdev_instance_no(DeviceState *dev);
>  
>  /*** BUS API. ***/
>  
> @@ -178,6 +180,7 @@ void qbus_create_inplace(BusState *bus, BusInfo *info,
>                           DeviceState *parent, const char *name);
>  BusState *qbus_create(BusInfo *info, DeviceState *parent, const char *name);
>  void qbus_free(BusState *bus);
> +BusState *qbus_find(const char *path, bool report_errors);
>  
>  #define FROM_QBUS(type, dev) DO_UPCAST(type, qbus, dev)
>  
> diff --git a/monitor.c b/monitor.c
> index 3e0d862..f535c56 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -64,12 +64,14 @@
>   *
>   * 'F'          filename
>   * 'B'          block device name
> + * 'q'          qdev bus path
> + * 'Q'          qdev device path
>   * 's'          string (accept optional quote)
>   * 'O'          option string of the form NAME=VALUE,...
>   *              parsed according to QemuOptsList given by its name
>   *              Example: 'device:O' uses qemu_device_opts.
>   *              Command completion for specific keys can be requested via
> - *              appending '(NAME:TYPE,...)' with 'F', 'B' as type.
> + *              appending '(NAME:TYPE,...)' with 'F', 'B', 'q', 'Q' as type.
>   *              Example: 'device:O(bus:Q)' to expand 'bus=...' as qtree path.
>   *              Restriction: only lists with empty desc are supported
>   *              TODO lift the restriction

Nitpick: you add "Example: 'device:O(bus:Q)'" in patch 11, before 'Q'
gets defined in patch 12.