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
