Re: [Qemu-devel] [PATCH v3 19/22] hmp: handle virtio-pmem when printing memory device infos

[Dr. David Alan Gilbert]

* David Hildenbrand (address@hidden) wrote:
> Print the memory device info just like for PCDIMM/NVDIMM.
> 
> Signed-off-by: David Hildenbrand <address@hidden>
> ---
>  hmp.c | 29 +++++++++++++++++------------
>  1 file changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/hmp.c b/hmp.c
> index 4975fa56b0..a592a4af9d 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -2529,27 +2529,21 @@ void hmp_info_memory_devices(Monitor *mon, const 
> QDict *qdict)
>      MemoryDeviceInfoList *info_list = qmp_query_memory_devices(&err);
>      MemoryDeviceInfoList *info;
>      MemoryDeviceInfo *value;
> -    PCDIMMDeviceInfo *di;
> +    VirtioPMemDeviceInfo *vpi;
>  
>      for (info = info_list; info; info = info->next) {
> +        PCDIMMDeviceInfo *di = NULL;
> +
>          value = info->value;
>  
>          if (value) {
>              switch (value->type) {
>              case MEMORY_DEVICE_INFO_KIND_DIMM:
>                  di = value->u.dimm.data;
> -                break;
> -
>              case MEMORY_DEVICE_INFO_KIND_NVDIMM:
> -                di = value->u.nvdimm.data;
> -                break;
> -
> -            default:
> -                di = NULL;
> -                break;
> -            }
> -
> -            if (di) {
> +                if (!di) {
> +                    di = value->u.nvdimm.data;
> +                }
>                  monitor_printf(mon, "Memory device [%s]: \"%s\"\n",
>                                 MemoryDeviceInfoKind_str(value->type),
>                                 di->id ? di->id : "");

That all works out as something like:
> +        PCDIMMDeviceInfo *di = NULL;
> +
>          value = info->value;
>  
>          if (value) {
>              switch (value->type) {
>              case MEMORY_DEVICE_INFO_KIND_DIMM:
>                  di = value->u.dimm.data;
>              case MEMORY_DEVICE_INFO_KIND_NVDIMM:
> +                if (!di) {
> +                    di = value->u.nvdimm.data;
> +                }
>                  monitor_printf(mon, "Memory device [%s]: \"%s\"\n",
>                                 MemoryDeviceInfoKind_str(value->type),
>                                 di->id ? di->id : "");

I'd prefer to avoid the drop through case statement; something like:
           PCDIMMDeviceInfo *di = NULL;
    
           value = info->value;
    
           if (value) {
               switch (value->type) {
               case MEMORY_DEVICE_INFO_KIND_DIMM:
               case MEMORY_DEVICE_INFO_KIND_NVDIMM:
                   di = (value->type == MEMORY_DEVICE_INFO_KIND_DIMM) ?
                       value->u.dimm.data : value->u.nvdimm.data;
                   monitor_printf(mon, "Memory device [%s]: \"%s\"\n",

> @@ -2562,6 +2556,17 @@ void hmp_info_memory_devices(Monitor *mon, const QDict 
> *qdict)
>                                 di->hotplugged ? "true" : "false");
>                  monitor_printf(mon, "  hotpluggable: %s\n",
>                                 di->hotpluggable ? "true" : "false");
> +                break;
> +            case MEMORY_DEVICE_INFO_KIND_VIRTIO_PMEM:
> +                vpi = value->u.virtio_pmem.data;
> +                monitor_printf(mon, "Memory device [%s]: \"%s\"\n",
> +                               MemoryDeviceInfoKind_str(value->type),
> +                               vpi->id ? vpi->id : "");
> +                monitor_printf(mon, "  memaddr: 0x%" PRIx64 "\n", 
> vpi->memaddr);
> +                monitor_printf(mon, "  size: %" PRIu64 "\n", vpi->size);
> +                monitor_printf(mon, "  memdev: %s\n", vpi->memdev);

Do you not get any equivalent of the 'hotplugged' flag or node?

Dave

> +            default:
> +                break;
>              }
>          }
>      }
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK