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. \ >>>> .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. >>>> .flags = VMS_SINGLE, \ >>>> .offset = vmstate_offset_value(_state, _field, VirtIODevice), \ This is useless if we keep virtio_save and virtio_load. >>>> } >>>> >>>> >> 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. 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. > > 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). Virtio-gpu does not adhere to the virtio-migration document because it saves the specialized device state after the virtio_save is done (that is after the common virtio subsections). This is however more like the normal approach -- first you save the parent, then the child. > 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. > 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. 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. Sorry again for my thick head. Cheers, Halil