* 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.
Dave
> >> +
> >> +static void hmp_virtio_dump_features(Monitor *mon,
> >> + VirtioStatusFeatures *features)
> >> +{
> >> + VirtioTransportFeatureList *transport_list = features->transport;
> >> + while (transport_list) {
> >> + monitor_printf(mon, "%s",
> >> + VirtioTransportFeature_str(transport_list->value));
> >> + transport_list = transport_list->next;
> >> + if (transport_list != NULL) {
> >> + monitor_printf(mon, ", ");
> >> + }
> >> + }
> >> + monitor_printf(mon, "\n");
> >> + switch (features->device->type) {
> >> + case VIRTIO_DEVICE_FEATURES_KIND_VIRTIO_SERIAL:
> >> + DUMP_FEATURES(VirtioSerial, virtio_serial);
> >> + break;
> >> + case VIRTIO_DEVICE_FEATURES_KIND_VIRTIO_BLK:
> >> + DUMP_FEATURES(VirtioBlk, virtio_blk);
> >> + break;
> >> + case VIRTIO_DEVICE_FEATURES_KIND_VIRTIO_GPU:
> >> + DUMP_FEATURES(VirtioGpu, virtio_gpu);
> >> + break;
> >> + case VIRTIO_DEVICE_FEATURES_KIND_VIRTIO_NET:
> >> + DUMP_FEATURES(VirtioNet, virtio_net);
> >> + break;
> >> + case VIRTIO_DEVICE_FEATURES_KIND_VIRTIO_SCSI:
> >> + DUMP_FEATURES(VirtioScsi, virtio_scsi);
> >> + break;
> >> + case VIRTIO_DEVICE_FEATURES_KIND_VIRTIO_BALLOON:
> >> + DUMP_FEATURES(VirtioBalloon, virtio_balloon);
> >> + break;
> >> + case VIRTIO_DEVICE_FEATURES_KIND_VIRTIO_IOMMU:
> >> + DUMP_FEATURES(VirtioIommu, virtio_iommu);
> >> + break;
> >> + default:
> >> + g_assert_not_reached();
> >> + }
> >> + if (features->unknown) {
> >> + monitor_printf(mon, "
> >> unknown(0x%016"PRIx64")\n", \
> >> + features->unknown);
> >> + }
> >> +}
> ...
>
> Thanks,
> Laurent
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
