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:
> 
> > * 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.
> 
> Programming error -> assert.  Nothing catches attention in testing like
> an assertion failure.

I don't want to get back through that; we've had this argument before;
but I'll still say in this case I'd prefer a clean error rather than
abort.

Dave

--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK