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
