Re: [Qemu-devel] [RFC PATCH v1] monitor: add parameter 'memory-devices' to the 'info' command

[Igor Mammedov <imammedo@redhat.com>]

On Thu, 4 Sep 2014 09:07:23 +0800
Zhu Guihua <zhugh.fnst@cn.fujitsu.com> wrote:

> When you hot remove hotpluggable memory devices, you should know the id of memory devices. But before this, you could not know the id of memory devices unless you remember all infomation about hotpluggable memory devices.
> This patch provides the function, if you input command 'info memory-devices' in monitor, monitor can list all available memory devices and their information.
Maybe followin commit message would be better:

Subj: Add HMP command "info memory-devices"

Provides HMP equivalent of QMP query-memory-devices command.

> 
> Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
> ---
>  hmp-commands.hx |  2 ++
>  hmp.c           | 33 +++++++++++++++++++++++++++++++++
>  hmp.h           |  1 +
>  monitor.c       |  7 +++++++
>  4 files changed, 43 insertions(+)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index d0943b1..1aa353f 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1780,6 +1780,8 @@ show qdev device model list
>  show roms
>  @item info tpm
>  show the TPM device
> +@item info memory-devices
> +show the memory devices
>  @end table
>  ETEXI
>  
> diff --git a/hmp.c b/hmp.c
> index 4d1838e..977d3f4 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1714,3 +1714,36 @@ void hmp_info_memdev(Monitor *mon, const QDict *qdict)
>  
>      monitor_printf(mon, "\n");
>  }
> +
> +void hmp_info_memory_devices(Monitor *mon, const QDict *qdict)
> +{
> +    Error *err = NULL;
> +    MemoryDeviceInfoList *list = qmp_query_memory_devices(&err);
> +    MemoryDeviceInfoList *elem = list;
> +    MemoryDeviceInfo *info;
> +    PCDIMMDeviceInfo *di;
> +    int i = 0;
> +
> +    while (elem) {
> +        info = elem->value;
> +        di = info->dimm;
> +
> +        monitor_printf(mon, "MemoryDevice %d\n", i);
> +        monitor_printf(mon, "  data:\n");
- monitor_printf(mon, "  data:\n");
+ monitor_printf(mon, "  %s\n", info->kind ? "max" : "dimm");

BTW what is "max" here?, there is no such type.
Also please use enum here and lookup type name in
MemoryDeviceInfoKind_lookup[] instead of opencodding it.

Luiz,
  Is there a QMP API to do this lookup or a better way to do it
  instead of accessing MemoryDeviceInfoKind_lookup[] directly?


moreover if MemoryDeviceInfo is not PCDIMMDevice,
accessing info->dimm would be just wrong, please fix it.


> +        monitor_printf(mon, "      id: %s\n", di->id);
> +        monitor_printf(mon, "      addr: %" PRId64 "\n", di->addr);
> +        monitor_printf(mon, "      slot: %" PRId64 "\n", di->slot);
> +        monitor_printf(mon, "      node: %" PRId64 "\n", di->node);
> +        monitor_printf(mon, "      size: %" PRId64 "\n", di->size);
> +        monitor_printf(mon, "      memdev: %s\n", di->memdev);
> +        monitor_printf(mon, "      hotplugged: %s\n", di->hotplugged ? "true" : "false");
> +        monitor_printf(mon, "      hotpluggable: %s\n", di->hotpluggable ? "true" : "false");
> +        monitor_printf(mon, "  type: %s\n", info->kind ? "max" : "dimm");
- monitor_printf(mon, "  type: %s\n", info->kind ? "max" : "dimm");
> +
> +        elem = elem->next;
> +        i++;
> +    }
> +
> +    qapi_free_MemoryDeviceInfoList(elem);
why do you need ^^^ when whole list is freed by the next line?


> +    qapi_free_MemoryDeviceInfoList(list);
> +}
> diff --git a/hmp.h b/hmp.h
> index 4fd3c4a..4bb5dca 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -94,6 +94,7 @@ void hmp_cpu_add(Monitor *mon, const QDict *qdict);
>  void hmp_object_add(Monitor *mon, const QDict *qdict);
>  void hmp_object_del(Monitor *mon, const QDict *qdict);
>  void hmp_info_memdev(Monitor *mon, const QDict *qdict);
> +void hmp_info_memory_devices(Monitor *mon, const QDict *qdict);
>  void object_add_completion(ReadLineState *rs, int nb_args, const char *str);
>  void object_del_completion(ReadLineState *rs, int nb_args, const char *str);
>  void device_add_completion(ReadLineState *rs, int nb_args, const char *str);
> diff --git a/monitor.c b/monitor.c
> index 34cee74..fe88e0d 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -2921,6 +2921,13 @@ static mon_cmd_t info_cmds[] = {
>          .mhandler.cmd = hmp_info_memdev,
>      },
>      {
> +        .name       = "memory-devices",
> +        .args_type  = "",
> +        .params     = "",
> +        .help       = "show memory devices",
> +        .mhandler.cmd = hmp_info_memory_devices,
> +    },
> +    {
>          .name       = NULL,
>      },
>  };