On 10/03/2016 08:24 PM, Jianjun Duan wrote: > Current migration code cannot handle some data structures such as > QTAILQ in qemu/queue.h. Here we extend the signatures of put/get > in VMStateInfo so that customized handling is supported. > > Signed-off-by: Jianjun Duan <du...@linux.vnet.ibm.com> > --- > hw/net/vmxnet3.c | 18 ++++++--- > hw/nvram/eeprom93xx.c | 6 ++- > hw/nvram/fw_cfg.c | 6 ++- > hw/pci/msix.c | 6 ++- > hw/pci/pci.c | 12 ++++-- > hw/pci/shpc.c | 5 ++- > hw/scsi/scsi-bus.c | 6 ++- > hw/timer/twl92230.c | 6 ++- > hw/usb/redirect.c | 18 ++++++--- > hw/virtio/virtio-pci.c | 6 ++- > hw/virtio/virtio.c | 6 ++- > include/migration/vmstate.h | 10 +++-- > migration/savevm.c | 5 ++- > migration/vmstate.c | 95 > ++++++++++++++++++++++++++++----------------- > target-alpha/machine.c | 5 ++- > target-arm/machine.c | 12 ++++-- > target-i386/machine.c | 21 ++++++---- > target-mips/machine.c | 10 +++-- > target-ppc/machine.c | 10 +++-- > target-sparc/machine.c | 5 ++- > 20 files changed, 171 insertions(+), 97 deletions(-)
Hi Jianjun! I'm not happy with the intrusive nature of this patch. We end up with a bunch of unused parameters. Have you considered something like: typedef struct { [..] union{ const VMStateInfo *info; const VMStateLinked *linked; }; enum VMStateFlags flags; [..] } VMStateField; /** * Handle linked datastructures. VMStateField.liked s to be used * in conjunction with VMStateField.vmsd which describes a node of * the datastucture without the pointers representing the links. * The links are embedded in the node starting at VMStateField.start. * The on wire representation of the information contained in links * and the head element if the responsibility of a particular VMStateField * instance. VMStateLinked is responsible for saving/loading the whole * sub-tree whose root is the field in question including the allocation of * memory for the nodes. The presence of VMStateField.linked is indicated * by the VMS_LINKED flag in VMStateField.flags. */ struct VMStateLinked { const char *name; void (*save)(QEMUFile *f, void *pv, VMStateField *field, QJSON *vmdesc); int (*load)(QEMUFile *f, void *pv, VMStateField *field); /* Maybe: size_t offset_links; */ }; IMHO this would: * allow us to keep the good old MVStateInfo objects unmodified and the semantic of VMStateInfo unchanged * make clear that VMStateLinked does not care about the calculated size and provide a new anchor for documentation * if overloading the semantic of VMStateField.start is not desired we could put it into VMStateLinked, if needed we could also put more stuff in there * have clearer separation between special handling for (linked/certain) data structures and the usual scenario with the DAG. I would also suggest unit tests in test/test-vmstate.c. Maybe with that the vmstate migration of QTAILQ could be factored out as a separate patch series. Cheers, Halil
signature.asc
Description: OpenPGP digital signature