Re: [RFC v4 6/6] hmp: add x-debug-virtio commands

[Laurent Vivier]

On 15/05/2020 17:45, Dr. David Alan Gilbert wrote:
> * Laurent Vivier (address@hidden) wrote:
>> On 13/05/2020 12:51, Dr. David Alan Gilbert wrote:
>>> * Laurent Vivier (address@hidden) wrote:
>>>> This patch implements HMP version of the virtio QMP commands
>>>>
>>>> Signed-off-by: Laurent Vivier <address@hidden>
>>>
>>> Reviewed-by: Dr. David Alan Gilbert <address@hidden>
>>>
>>> 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