Re: [Qemu-ppc] [Qemu-devel] [QEMU PATCH v5 3/6] migration: extend VMStateInfo

[Halil Pasic]

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 <address@hidden>
> ---
>  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