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

[Halil Pasic]

On 10/03/2016 05:24 PM, Paolo Bonzini wrote:
> 
> 
> 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
> 

Hi Paolo,

clear now. I will come back with a v2 with the suggestions implemented.
Thank you very much for your time.

Cheers,
Halil

signature.asc
Description: OpenPGP digital signature