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

[Dr. David Alan Gilbert]

* Paolo Bonzini (address@hidden) 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 / address@hidden / Manchester, UK