[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 01/12] virtio: add VIRTIO_DEF_DEVICE_VMSD macro
From: |
Dr. David Alan Gilbert |
Subject: |
Re: [Qemu-devel] [PATCH 01/12] virtio: add VIRTIO_DEF_DEVICE_VMSD macro |
Date: |
Wed, 5 Oct 2016 15:29:02 +0100 |
User-agent: |
Mutt/1.7.0 (2016-08-17) |
* 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
- Re: [Qemu-devel] [PATCH 01/12] virtio: add VIRTIO_DEF_DEVICE_VMSD macro, Halil Pasic, 2016/10/03
- Re: [Qemu-devel] [PATCH 01/12] virtio: add VIRTIO_DEF_DEVICE_VMSD macro, Paolo Bonzini, 2016/10/03
- Re: [Qemu-devel] [PATCH 01/12] virtio: add VIRTIO_DEF_DEVICE_VMSD macro, Halil Pasic, 2016/10/03
- Re: [Qemu-devel] [PATCH 01/12] virtio: add VIRTIO_DEF_DEVICE_VMSD macro, Paolo Bonzini, 2016/10/03
- Re: [Qemu-devel] [PATCH 01/12] virtio: add VIRTIO_DEF_DEVICE_VMSD macro, Halil Pasic, 2016/10/04
- Re: [Qemu-devel] [PATCH 01/12] virtio: add VIRTIO_DEF_DEVICE_VMSD macro,
Dr. David Alan Gilbert <=
- Re: [Qemu-devel] [PATCH 01/12] virtio: add VIRTIO_DEF_DEVICE_VMSD macro, Paolo Bonzini, 2016/10/05
- Re: [Qemu-devel] [PATCH 01/12] virtio: add VIRTIO_DEF_DEVICE_VMSD macro, Dr. David Alan Gilbert, 2016/10/05
- Re: [Qemu-devel] [PATCH 01/12] virtio: add VIRTIO_DEF_DEVICE_VMSD macro, Paolo Bonzini, 2016/10/06
- Re: [Qemu-devel] [PATCH 01/12] virtio: add VIRTIO_DEF_DEVICE_VMSD macro, Halil Pasic, 2016/10/06
- Re: [Qemu-devel] [PATCH 01/12] virtio: add VIRTIO_DEF_DEVICE_VMSD macro, Halil Pasic, 2016/10/06