From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60901) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1brnC5-0006nq-SQ for qemu-devel@nongnu.org; Wed, 05 Oct 2016 10:29:15 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1brnBz-0002dS-Rg for qemu-devel@nongnu.org; Wed, 05 Oct 2016 10:29:12 -0400 Received: from mx1.redhat.com ([209.132.183.28]:51218) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1brnBz-0002dK-IC for qemu-devel@nongnu.org; Wed, 05 Oct 2016 10:29:07 -0400 Date: Wed, 5 Oct 2016 15:29:02 +0100 From: "Dr. David Alan Gilbert" Message-ID: <20161005142902.GB11921@work-vm> References: <20160930142003.53232-1-pasic@linux.vnet.ibm.com> <20160930142003.53232-2-pasic@linux.vnet.ibm.com> <2487d687-7fbe-43f4-b9c5-e03b26b2250d@redhat.com> <5b08c569-813b-d805-833a-210e9c8dc436@linux.vnet.ibm.com> <49f37536-0fa7-a38b-a3fa-7b3050001658@redhat.com> <57b04738-7e95-a8c5-d974-0386cbff1419@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <57b04738-7e95-a8c5-d974-0386cbff1419@redhat.com> Subject: Re: [Qemu-devel] [PATCH 01/12] virtio: add VIRTIO_DEF_DEVICE_VMSD macro List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: Halil Pasic , qemu-devel@nongnu.org, "Michael S . Tsirkin" , Gerd Hoffmann , Stefan Hajnoczi , Amit Shah , "Aneesh Kumar K . V" * Paolo Bonzini (pbonzini@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 / dgilbert@redhat.com / Manchester, UK