Re: [Qemu-devel] [PATCH 01/12] virtio: add VIRTIO_DEF_DEVICE_VMSD macro

[Paolo Bonzini]

On 05/10/2016 21:00, Dr. David Alan Gilbert wrote:
> * Paolo Bonzini (address@hidden) wrote:
>>
>>
>> On 05/10/2016 16:29, Dr. David Alan Gilbert wrote:
>>> The virtio-input conversion especially is nice and simple.
>>> The only weird case is virtio-gpu, and well virtio-gpu is the one that's
>>> odd here (compared to the rest of virtio).
>>
>> Though virtio-gpu would be the one that could become nicer without the
>> macros. :)
> 
> Yes, it would.
> 
>> What I don't like about the macros is that they don't allow you to
>> extend the VMStateDescription.
> 
> Hmm so would this work:
> 
> add an 'extra_field'  that we normally leave empty:
> 
> #define VIRTIO_DEF_DEVICE_VMSD(devname, v, extra_field, ...) \
>     static const VMStateDescription vmstate_virtio_ ## devname = { \
>         .name = "virtio-" #devname ,          \
>         .minimum_version_id = v,              \
>         .version_id = v,                      \
>         .fields = (VMStateField[]) {          \
>             VMSTATE_VIRTIO_FIELD,             \
>             extra_field                       \
>             VMSTATE_END_OF_LIST()             \
>         },                                    \
>         __VA_ARGS__                           \
>     };
> 
> so the normal case would gain messy commas:
> 
>     VIRTIO_DEF_DEVICE_VMSD(console,3, /* empty */)
> the case with pre/post would be:
>     VIRTIO_DEF_DEVICE_VMSD(vhost_vsock, VHOST_VSOCK_SAVEVM_VERSION, /* empty 
> */,
>     .pre_save = vhost_vsock_pre_save, .post_load = vhost_vsock_post_load)
> 
> .....
> 
> Define a macro for this field so it's passed as one entry:
> 
> #define VMSTATE_VIRTIO_GPU_FIELD \
>          {                                        \
>              .name = "virtio-gpu",                \
>              .info = &(const VMStateInfo) {       \
>                          .name = "virtio-gpu",    \
>                          .get = virtio_gpu_load,  \
>                          .put = virtio_gpu_save,  \
>              },                                   \
>              .flags = VMS_SINGLE,                 \
>          } /* device */,
> 
> VIRTIO_DEF_DEVICE_VMSD(virtio-gpu, VIRTIO_GPU_VM_VERSION, 
> VMSTATE_VIRTIO_GPU_FIELD)

I would just expand the macro in the virtio-gpu case.  For now what
Halil did is fine.

Paolo