On 15/05/2020 17:45, Dr. David Alan Gilbert wrote: > * Laurent Vivier (lviv...@redhat.com) wrote: >> On 13/05/2020 12:51, Dr. David Alan Gilbert wrote: >>> * Laurent Vivier (lviv...@redhat.com) wrote: >>>> This patch implements HMP version of the virtio QMP commands >>>> >>>> Signed-off-by: Laurent Vivier <lviv...@redhat.com> >>> >>> Reviewed-by: Dr. David Alan Gilbert <dgilb...@redhat.com> >>> >>> With a thought below.... >>> >>>> --- >>>> Makefile | 2 +- >>>> Makefile.target | 7 +- >>>> docs/system/monitor.rst | 2 + >>>> hmp-commands-virtio.hx | 160 +++++++++++++++++++++++++++++++++ >>>> hmp-commands.hx | 10 +++ >>>> hw/virtio/virtio.c | 193 +++++++++++++++++++++++++++++++++++++++- >>>> include/monitor/hmp.h | 4 + >>>> monitor/misc.c | 17 ++++ >>>> 8 files changed, 391 insertions(+), 4 deletions(-) >>>> create mode 100644 hmp-commands-virtio.hx >>>> >> ... >>>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c >>>> index 66dc2cef1b39..c3d6b783417e 100644 >>>> --- a/hw/virtio/virtio.c >>>> +++ b/hw/virtio/virtio.c >> ... >>>> @@ -4033,6 +4092,92 @@ VirtioStatus *qmp_x_debug_virtio_status(const char* >>>> path, Error **errp) >>>> return status; >>>> } >>>> >>>> +#define DUMP_FEATURES(type, field) >>>> \ >>>> + do { >>>> \ >>>> + type##FeatureList *list = features->device->u.field.data; >>>> \ >>>> + if (list) { >>>> \ >>>> + monitor_printf(mon, " "); >>>> \ >>>> + while (list) { >>>> \ >>>> + monitor_printf(mon, "%s", >>>> type##Feature_str(list->value)); \ >>>> + list = list->next; >>>> \ >>>> + if (list != NULL) { >>>> \ >>>> + monitor_printf(mon, ", "); >>>> \ >>>> + } >>>> \ >>>> + } >>>> \ >>>> + monitor_printf(mon, "\n"); >>>> \ >>>> + } >>>> \ >>>> + } while (0) >>> >>> It feels like you should be able to have an array of Feature_str's >>> indexed by VIRTIO_DEVICE_FEATURE_KIND_ enum, so that when a new >>> VIRTIO_DEVICE_FEATURE_KIND is added you don't need to fix this up. >> >> I don't understand what you mean here. > > Instead of the switch below, I'm thinking you could have something like: > > if (features->device->type < something_MAX) { > features_str = anarray[features->device->type]; > > .... > monitor_printf(mon, "%s", features_str(list->value)); > .... > } > > with 'anarray' somewhere more central, so we don't have to keep > these switch structures and macros spread around.
OK, I tried that, but in fact we need to know the type of the list to be able to scan it (the "type##FeatureList": VirtoSerialFeatureList, VirtioBlkFeatureList, ...), except if we introduce a generic feature list node (for "next " and "value"). But it becomes more complicated, because we also need to generate the "anarray" somewhere. [Note: I've changed the legacy enum to a flat enum as proposed by Eric in v3] Thanks, Laurent