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

[Paolo Bonzini]

On 03/10/2016 12:36, Halil Pasic wrote:
>> > #define VMSTATE_PCI_DEVICE(_field, _state) {                           \
>> >     .name       = (stringify(_field)),                                 \
>> >     .size       = sizeof(VirtIODevice),                                \
>> >     .vmsd       = &vmstate_virtio_device,                              \
>> >     .flags      = VMS_SINGLE,                                          \
>> >     .offset     = vmstate_offset_value(_state, _field, VirtIODevice),  \
>> > }
>> > 
>> > 
> I'm not sure if I understand where are you coming from, but if I do, I
> think I have to disagree here. I think you are coming from the normal 
> device inheritance direction, where you have the parent storage object
> (that is its instance data( embedded into the child, and the corresponding
> parent state is supposed to get migrated as a (vmstate) field of the child.
> 
> Now if you look at the documentation of virtio migration (or the code) it is
> pretty obvious that the situation is very different for virtio. Virtio 
> migration
> is kind of a template method approach (with virtio_load and virtio_save being
> the template methods), and the parent (core, transport) and the child (device
> specific) data are intermingled (the device data is in the middle). We can
> not change this for compatibility reasons (sadly).

Regarding VMSTATE_VIRTIO_FIELD, it's just a matter of not doing things
differently for no particular reason.  Your macro is already doing
exactly the same as other VMSTATE_* macros, just with different
conventions for the arguments.  I don't see any advantage in changing that.

Regarding VIRTIO_DEF_DEVICE_VMSD, it's true that virtio migration is
currently done differently and we will definitely have to do things
somewhat different due to compatibility, but we can at least evolve
towards having a normal VMStateDescription (virtio-gpu is already more
similar to this).  Having everything hidden behind a magic macro makes
things harder to follow than other vmstate definitions.  It's okay when
you have a very small veneer as in commit 5943124cc, but in my opinion
having field definitions inside macro arguments is already too much.

Paolo