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

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Paolo Bonzini (pbonzini@redhat.com) 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)

>  However, if you agree with Halil your
> opinion counts more.

Well I think Halil's stuff moves it in the right direction;
with everything in VIRTIO_DEF_DEVICE_VMSD it means we can move things
out of virtio_load/save and into corresponding members in VIRTIO_DEF_DEVICE_VMSD's
 .fields array (before or after VMSTATE_VIRTIO_FIELD) without having
to change any of the devices. Eventually virtio_load/save disappear.

Dave

> 
> Paolo
> 
> > I'd say we take these macros (apart from anything minor) - I'd anticipate
> > they'd evolve slowly as we move more and more chunks to VMState.
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK