From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1Mdhek-0002Zi-EI for qemu-devel@nongnu.org; Wed, 19 Aug 2009 05:40:34 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1Mdhef-0002Yy-7E for qemu-devel@nongnu.org; Wed, 19 Aug 2009 05:40:33 -0400 Received: from [199.232.76.173] (port=43664 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Mdhef-0002Yv-0K for qemu-devel@nongnu.org; Wed, 19 Aug 2009 05:40:29 -0400 Received: from [66.187.237.31] (port=58499 helo=mx2.redhat.com) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1Mdhee-0000MV-Ft for qemu-devel@nongnu.org; Wed, 19 Aug 2009 05:40:28 -0400 Received: from int-mx2.corp.redhat.com (int-mx2.corp.redhat.com [172.16.27.26]) by mx2.redhat.com (8.13.8/8.13.8) with ESMTP id n7J9eRKa013938 for ; Wed, 19 Aug 2009 05:40:27 -0400 From: Juan Quintela In-Reply-To: <4A8BAE97.8010500@redhat.com> (Gerd Hoffmann's message of "Wed, 19 Aug 2009 09:49:43 +0200") Date: Wed, 19 Aug 2009 11:38:13 +0200 Message-ID: References: <4A8BAE97.8010500@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Subject: [Qemu-devel] Re: [PATCH 4/5] New VMstate save/load infrastructure List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Gerd Hoffmann Cc: qemu-devel@nongnu.org Reply-to: quintela@redhat.com Gerd Hoffmann wrote: >> +enum VMStateType { >> + VMSTATE_TYPE_UNSPEC = 0, >> + VMSTATE_TYPE_INT8, >> + VMSTATE_TYPE_INT16, >> + VMSTATE_TYPE_INT32, >> + VMSTATE_TYPE_INT64, >> + VMSTATE_TYPE_UINT8, >> + VMSTATE_TYPE_UINT16, >> + VMSTATE_TYPE_UINT32, >> + VMSTATE_TYPE_UINT64, >> + VMSTATE_TYPE_TIMER, >> +}; >> + >> +typedef struct VMStateInfo VMStateInfo; >> + >> +struct VMStateInfo { >> + const char *name; >> + size_t size; >> + enum VMStateType type; >> + void (*get)(QEMUFile *f, VMStateInfo *info, void *pv); >> + void (*put)(QEMUFile *f, VMStateInfo *info, const void *pv); >> +}; >> + >> +typedef struct { >> + const char *name; >> + size_t num; >> + size_t offset; >> + VMStateInfo *info; >> + int version_id; >> +} VMStateField; > > Hmm, I'm not sure VMStateInfo is that useful here. All the get/put > callbacks are simple two-liners, and it is unlikely that the number of > types ever grows. Thinking about this. VMStateType is not needed at all (see that it is not used in the patch). But Ilike the VMStateInfo struct approach. This is the easier way of dealing with structs. For instance pci_device_save(). My idea was to just create a new VMStateInfo for it, and then everything that needs to call pci_device_save() just add a field like: VMSTATE_FILED(dev, ThisPCIDevice, verion_id, pci_vmstate_info, PCIDevice) and here, we go. The reaso that I don't need VMStateType anymore is that now load/save are done: +static void vmstate_save(QEMUFile *f, VMStateDescription *vmsd, void *base) +{ + VMStateField *field = vmsd->fields; + int i; + + while(field->name) { + for (i = 0; i < field->num; i++) { + field->info->put(f, field->info, base + field->offset + + field->info->size * i); + } + field++; + } +} No need to have a type if we have the propper info struct. As this struct is going to grow (at least 2 new pointers for printing into monitor/read value from monitor), I think it is a good idea to maintain the sharing. Later, Juan. > I think I would stick "enum VMStateType" directly into VMStateField > and kill one level of indirection. > >> + >> +typedef struct { >> + const char *name; >> + int version_id; >> + int minimum_version_id; >> + VMStateField *fields; >> +} VMStateDescription; > > Add compat_load() callback for version_id < minimum_version_id here? I am not sure where/what to add. I liked your approach of having the new version clean, and let the old code to deal with the mess that used to be here. Will take a look at implementing soemething like that. Thanks for the review, Juan.