* Paolo Bonzini (pbonz...@redhat.com) 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.
Hmm, I actually disagree here; I like the macros that Halil has got; to me the important thing is that in most cases they're trivially short and in the slightly weirder cases they're not much bigger. The virtio-input conversion especially is nice and simple. The only weird case is virtio-gpu, and well virtio-gpu is the one that's odd here (compared to the rest of virtio). I'd say we take these macros (apart from anything minor) - I'd anticipate they'd evolve slowly as we move more and more chunks to VMState. Dave > Paolo -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK