Re: [Qemu-devel] [PATCH RFCv2 7/9] hmp: Handle virtio-pmem when printing memory device infos

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Markus Armbruster (armbru@redhat.com) wrote:
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
> 
> > * David Hildenbrand (david@redhat.com) wrote:
> >> Print the memory device info just like for PCDIMM/NVDIMM.
> >> 
> >> Signed-off-by: David Hildenbrand <david@redhat.com>
> >> ---
> >>  hmp.c | 27 +++++++++++++++------------
> >>  1 file changed, 15 insertions(+), 12 deletions(-)
> >> 
> >> diff --git a/hmp.c b/hmp.c
> >> index 8da5fd8760..25c32e0810 100644
> >> --- a/hmp.c
> >> +++ b/hmp.c
> >> @@ -2553,6 +2553,7 @@ void hmp_info_memory_devices(Monitor *mon, const QDict *qdict)
> >>      Error *err = NULL;
> >>      MemoryDeviceInfoList *info_list = qmp_query_memory_devices(&err);
> >>      MemoryDeviceInfoList *info;
> >> +    VirtioPMEMDeviceInfo *vpi;
> >>      MemoryDeviceInfo *value;
> >>      PCDIMMDeviceInfo *di;
> >>  
> >> @@ -2562,19 +2563,9 @@ void hmp_info_memory_devices(Monitor *mon, const QDict *qdict)
> >>          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) {
> >> +                di = value->type == MEMORY_DEVICE_INFO_KIND_DIMM ?
> >> +                     value->u.dimm.data : value->u.nvdimm.data;
> >>                  monitor_printf(mon, "Memory device [%s]: \"%s\"\n",
> >>                                 MemoryDeviceInfoKind_str(value->type),
> >>                                 di->id ? di->id : "");
> >> @@ -2587,6 +2578,18 @@ 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);
> >> +                break;
> >> +            default:
> >> +                g_assert_not_reached();
> >
> >
> > Although I'd prefer if that assert was replaced by a print
> > saying it was an unknown type.
> 
> I would not.  If we reach this, something must have scribbled over
> value->type and who knows what else.  Continuing is unsafe.  Looks like
> a textbook use of assertions to me.

Or it could be that someone added a new type of memory device and forgot
to update the hmp code.

Dave

> > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> >
> >>              }
> >>          }
> >>      }
> >> -- 
> >> 2.17.2
> >> 
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK