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

[Paolo Bonzini]

On 03/10/2016 15:34, Halil Pasic wrote:
> Hi Paolo,
> 
> I'm sorry, but I do not get it quite yet, or more exactly I have the
> feeling I did not manage to bring my point over. So I will try with
> more details.
> 
> On 10/03/2016 01:29 PM, Paolo Bonzini wrote:
>>
>>
>> On 03/10/2016 12:36, Halil Pasic wrote:
>>>>> #define VMSTATE_PCI_DEVICE(_field, _state) {
> 
> This was probably supposed to be VMSTATE_VIRTIO_DEVICE.

Yes.

>>>>>     .name       = (stringify(_field)),                                 \
>>>>>     .size       = sizeof(VirtIODevice),                                \
>>>>>     .vmsd       = &vmstate_virtio_device,                              \
> 
> This one does not exist at least very tricky to write because of the 
> peculiarities
> of virtio migration. This one would need to migrate the transport stuff too. 
> And
> the specialized device to, which is not normal.

This is my own typo - this should be .info.  Sorry.

>> 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.
> 
> In my opinion it is not the same. In the case of VMSTATE_PCI_DEVICE we have
> (a self contained) parent (in sense of inheritance) device, which is embedded
> as _field in the specialized device and is migrated by the vmstatedescription
> of the parent. The rest of the specialized devices state is represented by
> the other fields.
> 
> VMSTATE_VIRTIO_FIELD is however just there to make sure virtio_load and
> virtio_save are called at the right time with the right arguments. The 
> specialized
> device is then migrated by the save/load callbacks of the device class, or
> the vmsd residing in the device class. VMSTATE_VIRTIO_FIELD is supposed
> to be the only field, if the virtio device adheres to the virtio-migration
> document. VMSTATE_VIRTIO_FIELD has no arguments because it is
> a virtual field and does not depend on the offset stuff.
> 
> To summarize currently I have no idea how to write up the vmstate
> field definition macro VMSTATE_VIRTIO_DEVICE so that it meets your
> expectations. 

As above.

>> Having everything hidden behind a magic macro makes
>> things harder to follow than other vmstate definitions.  
> 
> Again in my opinion the virtio migration is different that the rest of the
> vmstate migration stuff, and it's ugly to some extent. So the idea was
> to make it look different and hide the ugliness behind the macro. 
> If you insist i will create a version where the macros are expanded so
> it's easier to say if this improves or worsens the readability.

I agree it is not exactly the same as the other devices.  But in my
opinion it's not different-enough to do everything completely more, and
in the future we should aim at making it less different.

> I think this is matter of taste, and your taste matters more ;). I do 
> agree that the variadic for the massaging functions is more complicated
> that the two function pointers taken by VMSTATE_VIRTIO_DEVICE. My idea
> was that we end up with more readable code on the caller-side, but if you
> prefer function pointers and NULLs if no callback is needed needed 
> (most cases), I can live with that.

Well, the third possibility would be expanding the VMStateDescription
definition, :) where .post_load becomes just yet another initializer.

Paolo