Re: [Qemu-devel] [RFC PATCH v2 05/16] Header/constant/types fixes for visitors

Re: [Qemu-devel] [RFC PATCH v2 05/16] Header/constant/types fixes for visitors

[Michael S. Tsirkin]

On Wed, Apr 23, 2014 at 05:37:38PM +0100, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Move constants around and add types to allow file structure to move into
> visitors.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  arch_init.c                   | 12 ------------
>  include/migration/migration.h | 17 +++++++++++++++++
>  include/migration/vmstate.h   | 20 +++++++++++++++++---
>  include/qemu/typedefs.h       |  4 ++--
>  4 files changed, 36 insertions(+), 17 deletions(-)
> 
> diff --git a/arch_init.c b/arch_init.c
> index 60c975d..73b9303 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -110,18 +110,6 @@ static bool mig_throttle_on;
>  static int dirty_rate_high_cnt;
>  static void check_guest_throttling(void);
>  
> -/***********************************************************/
> -/* ram save/restore */
> -
> -#define RAM_SAVE_FLAG_FULL     0x01 /* Obsolete, not used anymore */
> -#define RAM_SAVE_FLAG_COMPRESS 0x02
> -#define RAM_SAVE_FLAG_MEM_SIZE 0x04
> -#define RAM_SAVE_FLAG_PAGE     0x08
> -#define RAM_SAVE_FLAG_EOS      0x10
> -#define RAM_SAVE_FLAG_CONTINUE 0x20
> -#define RAM_SAVE_FLAG_XBZRLE   0x40
> -/* 0x80 is reserved in migration.h start with 0x100 next */
> -
>  static struct defconfig_file {
>      const char *filename;
>      /* Indicates it is an user config file (disabled by -no-user-config) */
> diff --git a/include/migration/migration.h b/include/migration/migration.h
> index 3e1e6c7..8111125 100644
> --- a/include/migration/migration.h
> +++ b/include/migration/migration.h
> @@ -165,7 +165,16 @@ void ram_control_load_hook(QEMUFile *f, uint64_t flags);
>   * side. This lets before_ram_iterate/after_ram_iterate add
>   * transport-specific sections to the RAM migration data.
>   */
> +/* ram save/restore */
> +#define RAM_SAVE_FLAG_FULL     0x01 /* Obsolete, not used anymore */
> +#define RAM_SAVE_FLAG_COMPRESS 0x02
> +#define RAM_SAVE_FLAG_MEM_SIZE 0x04
> +#define RAM_SAVE_FLAG_PAGE     0x08
> +#define RAM_SAVE_FLAG_EOS      0x10
> +#define RAM_SAVE_FLAG_CONTINUE 0x20
> +#define RAM_SAVE_FLAG_XBZRLE   0x40
>  #define RAM_SAVE_FLAG_HOOK     0x80
> +#define RAM_SAVE_FLAG_MASK    0x1ff
>  
>  #define RAM_SAVE_CONTROL_NOT_SUPP -1000
>  #define RAM_SAVE_CONTROL_DELAYED  -2000
> @@ -174,4 +183,12 @@ size_t ram_control_save_page(QEMUFile *f, ram_addr_t block_offset,
>                               ram_addr_t offset, size_t size,
>                               int *bytes_sent);
>  
> +typedef struct {
> +    uint64_t addr;
> +    uint16_t flags;
> +    char     idstr[256];
> +    char     ch;  /* Used for filled pages (normally 0 fill) */
> +    size_t   len; /* Uses include xbzrle's data len */
> +} ramsecentry_header;
> +


RamSecEntryHeader?

and maybe we should make this 256 a named constant too.


>  #endif
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index e7e1705..a5e4b0b 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -26,6 +26,7 @@
>  #ifndef QEMU_VMSTATE_H
>  #define QEMU_VMSTATE_H 1
>  
> +#include "qemu/typedefs.h"
>  #ifndef CONFIG_USER_ONLY
>  #include <migration/qemu-file.h>
>  #endif
> @@ -49,15 +50,27 @@ typedef struct SaveVMHandlers {
>       * use data that is local to the migration thread or protected
>       * by other locks.
>       */
> -    int (*save_live_iterate)(QEMUFile *f, void *opaque);
> +    int (*save_live_iterate)(Visitor *v, void *opaque);
>  
>      /* This runs outside the iothread lock!  */
> -    int (*save_live_setup)(QEMUFile *f, void *opaque);
> -    uint64_t (*save_live_pending)(QEMUFile *f, void *opaque, uint64_t max_size);
> +    int (*save_live_setup)(Visitor *v, void *opaque);
> +    uint64_t (*save_live_pending)(void *opaque, uint64_t max_size);
>  
>      LoadStateHandler *load_state;
>  } SaveVMHandlers;
>  
> +/* This is the data used to identify a section as passed
> + * into the section version of the compat sequence visitor
> + * (TODO: Probably want to move the whole name lookup into there
> + *    and keep the section_id wrapped inside the binary visitor)
> + */
> +typedef struct SectionHeader {
> +    uint32_t section_id;
> +    uint32_t instance_id; /* Below only used for full version */
> +    uint32_t version_id;
> +    char     idstr[256];
> +} SectionHeader;
> +
>  int register_savevm(DeviceState *dev,
>                      const char *idstr,
>                      int instance_id,
> @@ -134,6 +147,7 @@ struct VMStateDescription {
>      void (*pre_save)(void *opaque);
>      VMStateField *fields;
>      const VMStateSubsection *subsections;
> +    uint32_t ber_tag;
>  };
>  
>  #ifdef CONFIG_USER_ONLY
> diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
> index bf8daac..3fea88e 100644
> --- a/include/qemu/typedefs.h
> +++ b/include/qemu/typedefs.h
> @@ -10,8 +10,6 @@ typedef struct QEMUBH QEMUBH;
>  
>  typedef struct AioContext AioContext;
>  
> -typedef struct Visitor Visitor;
> -
>  struct Monitor;
>  typedef struct Monitor Monitor;
>  typedef struct MigrationParams MigrationParams;
> @@ -39,6 +37,7 @@ typedef struct DriveInfo DriveInfo;
>  typedef struct DisplayState DisplayState;
>  typedef struct DisplayChangeListener DisplayChangeListener;
>  typedef struct DisplaySurface DisplaySurface;
> +typedef struct Error Error;
>  typedef struct PixelFormat PixelFormat;
>  typedef struct QemuConsole QemuConsole;
>  typedef struct CharDriverState CharDriverState;
> @@ -73,5 +72,6 @@ typedef struct SHPCDevice SHPCDevice;
>  typedef struct FWCfgState FWCfgState;
>  typedef struct PcGuestInfo PcGuestInfo;
>  typedef struct Range Range;
> +typedef struct Visitor Visitor;
>  
>  #endif /* QEMU_TYPEDEFS_H */
> -- 
> 1.9.0

Re: [Qemu-devel] [RFC PATCH v2 05/16] Header/constant/types fixes for visitors

[Dr. David Alan Gilbert]

* Michael S. Tsirkin (mst@redhat.com) wrote:
> On Wed, Apr 23, 2014 at 05:37:38PM +0100, Dr. David Alan Gilbert (git) wrote:

<snip>

> > +typedef struct {
> > +    uint64_t addr;
> > +    uint16_t flags;
> > +    char     idstr[256];
> > +    char     ch;  /* Used for filled pages (normally 0 fill) */
> > +    size_t   len; /* Uses include xbzrle's data len */
> > +} ramsecentry_header;
> > +
> 
> 
> RamSecEntryHeader?

Fixed, thanks.

> and maybe we should make this 256 a named constant too.

It's 256 because the old format uses a byte as a length; I've commented
it.

Dave

--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [RFC PATCH v2 01/16] Visitor: Add methods for migration format use

[Michael S. Tsirkin]

On Wed, Apr 23, 2014 at 05:37:34PM +0100, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> array types
>     From  https://lists.gnu.org/archive/html/qemu-devel/2011-09/msg02465.html
> 
> str256 type
>     For the upto 256byte strings QEMU commonly uses for IDs
> 
> buffer type
>     For a blob of data that the caller wants to deliver whole (e.g.
>     a page of RAM or block of disk)
> 
> Load/save flags to let a user perform pre-save/post-load checking
> 
> An accessor to get the underlying QEMUFile* (for compatibility)
> 
> compat-sequences
>     Provide enough information for a visitor providing compatibility
> with the old format to generate it's byte stream, while allowing a new
> visitor to do something sensible.
> 
> destroy
>     Allows the caller to destroy a visitor without knowing what type of
>     visitor it is.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  include/qapi/visitor-impl.h | 23 +++++++++++++
>  include/qapi/visitor.h      | 51 +++++++++++++++++++++++++++++
>  qapi/qapi-visit-core.c      | 80 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 154 insertions(+)
> 
> diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
> index f3fa420..10cdbf7 100644
> --- a/include/qapi/visitor-impl.h
> +++ b/include/qapi/visitor-impl.h
> @@ -15,6 +15,9 @@
>  #include "qapi/error.h"
>  #include "qapi/visitor.h"
>  
> +#define VISITOR_SAVING (1<<0)
> +#define VISITOR_LOADING (1<<1)
> +
>  struct Visitor
>  {
>      /* Must be set */
> @@ -29,6 +32,10 @@ struct Visitor
>      void (*start_list)(Visitor *v, const char *name, Error **errp);
>      GenericList *(*next_list)(Visitor *v, GenericList **list, Error **errp);
>      void (*end_list)(Visitor *v, Error **errp);
> +    void (*start_array)(Visitor *v, void **obj, const char *name,
> +                        size_t elem_count, size_t elem_size, Error **errp);
> +    void (*next_array)(Visitor *v, Error **errp);
> +    void (*end_array)(Visitor *v, Error **errp);
>  
>      void (*type_enum)(Visitor *v, int *obj, const char *strings[],
>                        const char *kind, const char *name, Error **errp);
> @@ -38,6 +45,7 @@ struct Visitor
>      void (*type_int)(Visitor *v, int64_t *obj, const char *name, Error **errp);
>      void (*type_bool)(Visitor *v, bool *obj, const char *name, Error **errp);
>      void (*type_str)(Visitor *v, char **obj, const char *name, Error **errp);
> +    void (*type_str256)(Visitor *v, char *obj, const char *name, Error **errp);
>      void (*type_number)(Visitor *v, double *obj, const char *name,
>                          Error **errp);
>  
> @@ -49,6 +57,8 @@ struct Visitor
>      void (*start_handle)(Visitor *v, void **obj, const char *kind,
>                           const char *name, Error **errp);
>      void (*end_handle)(Visitor *v, Error **errp);
> +    void (*type_buffer)(Visitor *v, void *data, size_t len, bool async,
> +                        const char *name, Error **errp);
>      void (*type_uint8)(Visitor *v, uint8_t *obj, const char *name, Error **errp);
>      void (*type_uint16)(Visitor *v, uint16_t *obj, const char *name, Error **errp);
>      void (*type_uint32)(Visitor *v, uint32_t *obj, const char *name, Error **errp);
> @@ -59,6 +69,19 @@ struct Visitor
>      void (*type_int64)(Visitor *v, int64_t *obj, const char *name, Error **errp);
>      /* visit_type_size() falls back to (*type_uint64)() if type_size is unset */
>      void (*type_size)(Visitor *v, uint64_t *obj, const char *name, Error **errp);
> +
> +    void (*destroy)(Visitor *v, Error **errp);
> +
> +    void (*start_sequence_compat)(Visitor *v, const char *name,
> +                                  Visit_seq_compat_mode compat_mode,
> +                                  void *opaque, Error **errp);
> +    void (*end_sequence_compat)(Visitor *v, const char *name,
> +                                Visit_seq_compat_mode compat_mode,
> +                                Error **errp);
> +
> +    QEMUFile* (*get_qemufile)(Visitor *v);
> +
> +    uint64_t flags;
>  };
>  
>  void input_type_enum(Visitor *v, int *obj, const char *strings[],
> diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
> index 29da211..70c20df 100644
> --- a/include/qapi/visitor.h
> +++ b/include/qapi/visitor.h
> @@ -39,11 +39,22 @@ void visit_end_implicit_struct(Visitor *v, Error **errp);
>  void visit_start_list(Visitor *v, const char *name, Error **errp);
>  GenericList *visit_next_list(Visitor *v, GenericList **list, Error **errp);
>  void visit_end_list(Visitor *v, Error **errp);
> +void visit_start_array(Visitor *v, void **obj, const char *name,
> +                       size_t elem_count, size_t elem_size, Error **errp);
> +void visit_next_array(Visitor *v, Error **errp);
> +void visit_end_array(Visitor *v, Error **errp);
> +
>  void visit_start_optional(Visitor *v, bool *present, const char *name,
>                            Error **errp);
>  void visit_end_optional(Visitor *v, Error **errp);
>  void visit_get_next_type(Visitor *v, int *obj, const int *qtypes,
>                           const char *name, Error **errp);
> +/* Blocks of guest memory,disk or otherwise opaque data that there is a lot
> + * of and must be handled efficiently. 'async' true if the write can happen
> + * 'later'
> + */
> +void visit_type_buffer(Visitor *v, void *data, size_t len, bool async,
> +                       const char *name, Error **errp);
>  void visit_type_enum(Visitor *v, int *obj, const char *strings[],
>                       const char *kind, const char *name, Error **errp);
>  void visit_type_int(Visitor *v, int64_t *obj, const char *name, Error **errp);
> @@ -58,6 +69,46 @@ void visit_type_int64(Visitor *v, int64_t *obj, const char *name, Error **errp);
>  void visit_type_size(Visitor *v, uint64_t *obj, const char *name, Error **errp);
>  void visit_type_bool(Visitor *v, bool *obj, const char *name, Error **errp);
>  void visit_type_str(Visitor *v, char **obj, const char *name, Error **errp);
> +/* A string no more than 256 (including term) characters in length */
> +void visit_type_str256(Visitor *v, char *obj, const char *name, Error **errp);
>  void visit_type_number(Visitor *v, double *obj, const char *name, Error **errp);
> +void visit_destroy(Visitor *v, Error **errp);
> +
> +/* Type of visitor, used where actions have to be taken when (de)serializing */
> +bool visitor_loading(Visitor *v);
> +bool visitor_saving(Visitor *v);
> +
> +typedef enum {
> +    VISIT_SEQ_COMPAT_BYTE0TERM,      /* list terminated with a 0 byte */
> +    VISIT_SEQ_COMPAT_FILE,           /* The top level file object */
> +    VISIT_SEQ_COMPAT_SUBSECLIST,     /* list terminated by
> +                                        historical complexity */
> +    VISIT_SEQ_COMPAT_SUBSECTION,     /* One subsection */
> +    VISIT_SEQ_COMPAT_SECTION_HEADER, /* SECTION_FULL/START, header + name */
> +    VISIT_SEQ_COMPAT_SECTION_MIN,    /* SECTION_PART/END - minimal header */
> +    VISIT_SEQ_COMPAT_RAMSECLIST,     /* list terminated by int64 bit
> +                                        RAM_SAVE_FLAG_EOS */
> +    VISIT_SEQ_COMPAT_RAMSECENTRY,    /* Entry in RAM Sec list */
> +    VISIT_SEQ_COMPAT_VMSTATE,        /* One VMState */
> +    VISIT_SEQ_COMPAT_BLOB            /* A binary old-format blob */
> +} Visit_seq_compat_mode;

VisitorSeqCompatMode

We have visit_ for functions but Visitor for types ...

> +
> +/* Start a sequence of items (which may be of unknown length and unknown
> + * mix of some subset of types), specify a compatibility mode that's only
> + * used by an implementation trying to match the existing binary migration
> + * format.
> + * opaque is compat_mode specific
> + */
> +void visit_start_sequence_compat(Visitor *v, const char *name,
> +                                 Visit_seq_compat_mode compat_mode,
> +                                 void *opaque,
> +                                 Error **errp);
> +/* Use visit_get_next_type for each entry including the first */
> +void visit_end_sequence_compat(Visitor *v, const char *name,
> +                                 Visit_seq_compat_mode compat_mode,
> +                                 Error **errp);
> +
> +/* Don't Use! - lets us move forward until we can get rid of all file uses */
> +QEMUFile *visitor_get_qemufile(Visitor *v);
>  
>  #endif
> diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
> index 6451a21..2d20fde 100644
> --- a/qapi/qapi-visit-core.c
> +++ b/qapi/qapi-visit-core.c
> @@ -84,6 +84,28 @@ void visit_end_list(Visitor *v, Error **errp)
>      v->end_list(v, errp);
>  }
>  
> +void visit_start_array(Visitor *v, void **obj, const char *name,
> +                       size_t elem_count, size_t elem_size, Error **errp)
> +{
> +    if (!error_is_set(errp)) {
> +        v->start_array(v, obj, name, elem_count, elem_size, errp);
> +    }
> +}
> +
> +void visit_next_array(Visitor *v, Error **errp)
> +{
> +    if (!error_is_set(errp)) {
> +        v->next_array(v, errp);
> +    }
> +}
> +
> +void visit_end_array(Visitor *v, Error **errp)
> +{
> +    if (!error_is_set(errp)) {
> +        v->end_array(v, errp);
> +    }
> +}
> +
>  void visit_start_optional(Visitor *v, bool *present, const char *name,
>                            Error **errp)
>  {
> @@ -107,6 +129,14 @@ void visit_get_next_type(Visitor *v, int *obj, const int *qtypes,
>      }
>  }
>  
> +void visit_type_buffer(Visitor *v, void *data, size_t len, bool async,
> +                       const char *name, Error **errp)
> +{
> +    if (!error_is_set(errp)) {
> +        v->type_buffer(v, data, len, async, name, errp);
> +    }
> +}
> +
>  void visit_type_enum(Visitor *v, int *obj, const char *strings[],
>                       const char *kind, const char *name, Error **errp)
>  {
> @@ -291,6 +321,13 @@ void visit_type_str(Visitor *v, char **obj, const char *name, Error **errp)
>      }
>  }
>  
> +void visit_type_str256(Visitor *v, char *obj, const char *name, Error **errp)
> +{
> +    if (!error_is_set(errp)) {
> +        v->type_str256(v, obj, name, errp);
> +    }
> +}
> +
>  void visit_type_number(Visitor *v, double *obj, const char *name, Error **errp)
>  {
>      if (!error_is_set(errp)) {
> @@ -347,3 +384,46 @@ void input_type_enum(Visitor *v, int *obj, const char *strings[],
>      g_free(enum_str);
>      *obj = value;
>  }
> +
> +void visit_destroy(Visitor *v, Error **errp)
> +{
> +    v->destroy(v, errp);
> +}
> +
> +void visit_start_sequence_compat(Visitor *v, const char *name,
> +                                 Visit_seq_compat_mode compat_mode,
> +                                 void *opaque, Error **errp)
> +{
> +    if (error_is_set(errp)) {
> +        return;
> +    }
> +
> +    v->start_sequence_compat(v, name, compat_mode, opaque, errp);
> +}
> +
> +void visit_end_sequence_compat(Visitor *v, const char *name,
> +                                 Visit_seq_compat_mode compat_mode,
> +                               Error **errp)
> +{
> +    if (error_is_set(errp)) {
> +        return;
> +    }
> +
> +    v->end_sequence_compat(v, name, compat_mode, errp);
> +}
> +
> +QEMUFile *visitor_get_qemufile(Visitor *v)
> +{
> +    return v->get_qemufile(v);
> +}
> +
> +bool visitor_loading(Visitor *v)
> +{
> +    return v->flags & VISITOR_LOADING;
> +}
> +
> +bool visitor_saving(Visitor *v)
> +{
> +    return v->flags & VISITOR_SAVING;
> +}
> +
> -- 
> 1.9.0

Re: [Qemu-devel] [RFC PATCH v2 01/16] Visitor: Add methods for migration format use

[Dr. David Alan Gilbert]

* Michael S. Tsirkin (mst@redhat.com) wrote:
> On Wed, Apr 23, 2014 at 05:37:34PM +0100, Dr. David Alan Gilbert (git) wrote:

<snip>

> > +/* Type of visitor, used where actions have to be taken when (de)serializing */
> > +bool visitor_loading(Visitor *v);
> > +bool visitor_saving(Visitor *v);
> > +
> > +typedef enum {
> > +    VISIT_SEQ_COMPAT_BYTE0TERM,      /* list terminated with a 0 byte */
> > +    VISIT_SEQ_COMPAT_FILE,           /* The top level file object */
> > +    VISIT_SEQ_COMPAT_SUBSECLIST,     /* list terminated by
> > +                                        historical complexity */
> > +    VISIT_SEQ_COMPAT_SUBSECTION,     /* One subsection */
> > +    VISIT_SEQ_COMPAT_SECTION_HEADER, /* SECTION_FULL/START, header + name */
> > +    VISIT_SEQ_COMPAT_SECTION_MIN,    /* SECTION_PART/END - minimal header */
> > +    VISIT_SEQ_COMPAT_RAMSECLIST,     /* list terminated by int64 bit
> > +                                        RAM_SAVE_FLAG_EOS */
> > +    VISIT_SEQ_COMPAT_RAMSECENTRY,    /* Entry in RAM Sec list */
> > +    VISIT_SEQ_COMPAT_VMSTATE,        /* One VMState */
> > +    VISIT_SEQ_COMPAT_BLOB            /* A binary old-format blob */
> > +} Visit_seq_compat_mode;
> 
> VisitorSeqCompatMode

Fixed.

> We have visit_ for functions but Visitor for types ...

So should I be changing those visitor_loading/saving's to visit_ ?

Dave

> 
> > +
> > +/* Start a sequence of items (which may be of unknown length and unknown
> > + * mix of some subset of types), specify a compatibility mode that's only
> > + * used by an implementation trying to match the existing binary migration
> > + * format.
> > + * opaque is compat_mode specific
> > + */
> > +void visit_start_sequence_compat(Visitor *v, const char *name,
> > +                                 Visit_seq_compat_mode compat_mode,
> > +                                 void *opaque,
> > +                                 Error **errp);
> > +/* Use visit_get_next_type for each entry including the first */
> > +void visit_end_sequence_compat(Visitor *v, const char *name,
> > +                                 Visit_seq_compat_mode compat_mode,
> > +                                 Error **errp);
> > +
> > +/* Don't Use! - lets us move forward until we can get rid of all file uses */
> > +QEMUFile *visitor_get_qemufile(Visitor *v);
> >  
> >  #endif
> > diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
> > index 6451a21..2d20fde 100644
> > --- a/qapi/qapi-visit-core.c
> > +++ b/qapi/qapi-visit-core.c
> > @@ -84,6 +84,28 @@ void visit_end_list(Visitor *v, Error **errp)
> >      v->end_list(v, errp);
> >  }
> >  
> > +void visit_start_array(Visitor *v, void **obj, const char *name,
> > +                       size_t elem_count, size_t elem_size, Error **errp)
> > +{
> > +    if (!error_is_set(errp)) {
> > +        v->start_array(v, obj, name, elem_count, elem_size, errp);
> > +    }
> > +}
> > +
> > +void visit_next_array(Visitor *v, Error **errp)
> > +{
> > +    if (!error_is_set(errp)) {
> > +        v->next_array(v, errp);
> > +    }
> > +}
> > +
> > +void visit_end_array(Visitor *v, Error **errp)
> > +{
> > +    if (!error_is_set(errp)) {
> > +        v->end_array(v, errp);
> > +    }
> > +}
> > +
> >  void visit_start_optional(Visitor *v, bool *present, const char *name,
> >                            Error **errp)
> >  {
> > @@ -107,6 +129,14 @@ void visit_get_next_type(Visitor *v, int *obj, const int *qtypes,
> >      }
> >  }
> >  
> > +void visit_type_buffer(Visitor *v, void *data, size_t len, bool async,
> > +                       const char *name, Error **errp)
> > +{
> > +    if (!error_is_set(errp)) {
> > +        v->type_buffer(v, data, len, async, name, errp);
> > +    }
> > +}
> > +
> >  void visit_type_enum(Visitor *v, int *obj, const char *strings[],
> >                       const char *kind, const char *name, Error **errp)
> >  {
> > @@ -291,6 +321,13 @@ void visit_type_str(Visitor *v, char **obj, const char *name, Error **errp)
> >      }
> >  }
> >  
> > +void visit_type_str256(Visitor *v, char *obj, const char *name, Error **errp)
> > +{
> > +    if (!error_is_set(errp)) {
> > +        v->type_str256(v, obj, name, errp);
> > +    }
> > +}
> > +
> >  void visit_type_number(Visitor *v, double *obj, const char *name, Error **errp)
> >  {
> >      if (!error_is_set(errp)) {
> > @@ -347,3 +384,46 @@ void input_type_enum(Visitor *v, int *obj, const char *strings[],
> >      g_free(enum_str);
> >      *obj = value;
> >  }
> > +
> > +void visit_destroy(Visitor *v, Error **errp)
> > +{
> > +    v->destroy(v, errp);
> > +}
> > +
> > +void visit_start_sequence_compat(Visitor *v, const char *name,
> > +                                 Visit_seq_compat_mode compat_mode,
> > +                                 void *opaque, Error **errp)
> > +{
> > +    if (error_is_set(errp)) {
> > +        return;
> > +    }
> > +
> > +    v->start_sequence_compat(v, name, compat_mode, opaque, errp);
> > +}
> > +
> > +void visit_end_sequence_compat(Visitor *v, const char *name,
> > +                                 Visit_seq_compat_mode compat_mode,
> > +                               Error **errp)
> > +{
> > +    if (error_is_set(errp)) {
> > +        return;
> > +    }
> > +
> > +    v->end_sequence_compat(v, name, compat_mode, errp);
> > +}
> > +
> > +QEMUFile *visitor_get_qemufile(Visitor *v)
> > +{
> > +    return v->get_qemufile(v);
> > +}
> > +
> > +bool visitor_loading(Visitor *v)
> > +{
> > +    return v->flags & VISITOR_LOADING;
> > +}
> > +
> > +bool visitor_saving(Visitor *v)
> > +{
> > +    return v->flags & VISITOR_SAVING;
> > +}
> > +
> > -- 
> > 1.9.0
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [RFC PATCH v2 01/16] Visitor: Add methods for migration format use

[Michael S. Tsirkin]

On Wed, May 07, 2014 at 11:23:05AM +0100, Dr. David Alan Gilbert wrote:
> * Michael S. Tsirkin (mst@redhat.com) wrote:
> > On Wed, Apr 23, 2014 at 05:37:34PM +0100, Dr. David Alan Gilbert (git) wrote:
> 
> <snip>
> 
> > > +/* Type of visitor, used where actions have to be taken when (de)serializing */
> > > +bool visitor_loading(Visitor *v);
> > > +bool visitor_saving(Visitor *v);
> > > +
> > > +typedef enum {
> > > +    VISIT_SEQ_COMPAT_BYTE0TERM,      /* list terminated with a 0 byte */
> > > +    VISIT_SEQ_COMPAT_FILE,           /* The top level file object */
> > > +    VISIT_SEQ_COMPAT_SUBSECLIST,     /* list terminated by
> > > +                                        historical complexity */
> > > +    VISIT_SEQ_COMPAT_SUBSECTION,     /* One subsection */
> > > +    VISIT_SEQ_COMPAT_SECTION_HEADER, /* SECTION_FULL/START, header + name */
> > > +    VISIT_SEQ_COMPAT_SECTION_MIN,    /* SECTION_PART/END - minimal header */
> > > +    VISIT_SEQ_COMPAT_RAMSECLIST,     /* list terminated by int64 bit
> > > +                                        RAM_SAVE_FLAG_EOS */
> > > +    VISIT_SEQ_COMPAT_RAMSECENTRY,    /* Entry in RAM Sec list */
> > > +    VISIT_SEQ_COMPAT_VMSTATE,        /* One VMState */
> > > +    VISIT_SEQ_COMPAT_BLOB            /* A binary old-format blob */
> > > +} Visit_seq_compat_mode;
> > 
> > VisitorSeqCompatMode
> 
> Fixed.
> 
> > We have visit_ for functions but Visitor for types ...
> 
> So should I be changing those visitor_loading/saving's to visit_ ?
> 
> Dave

I don't think so: visit_ seems to be the function that actually visits something
:)


> > 
> > > +
> > > +/* Start a sequence of items (which may be of unknown length and unknown
> > > + * mix of some subset of types), specify a compatibility mode that's only
> > > + * used by an implementation trying to match the existing binary migration
> > > + * format.
> > > + * opaque is compat_mode specific
> > > + */
> > > +void visit_start_sequence_compat(Visitor *v, const char *name,
> > > +                                 Visit_seq_compat_mode compat_mode,
> > > +                                 void *opaque,
> > > +                                 Error **errp);
> > > +/* Use visit_get_next_type for each entry including the first */
> > > +void visit_end_sequence_compat(Visitor *v, const char *name,
> > > +                                 Visit_seq_compat_mode compat_mode,
> > > +                                 Error **errp);
> > > +
> > > +/* Don't Use! - lets us move forward until we can get rid of all file uses */
> > > +QEMUFile *visitor_get_qemufile(Visitor *v);
> > >  
> > >  #endif
> > > diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
> > > index 6451a21..2d20fde 100644
> > > --- a/qapi/qapi-visit-core.c
> > > +++ b/qapi/qapi-visit-core.c
> > > @@ -84,6 +84,28 @@ void visit_end_list(Visitor *v, Error **errp)
> > >      v->end_list(v, errp);
> > >  }
> > >  
> > > +void visit_start_array(Visitor *v, void **obj, const char *name,
> > > +                       size_t elem_count, size_t elem_size, Error **errp)
> > > +{
> > > +    if (!error_is_set(errp)) {
> > > +        v->start_array(v, obj, name, elem_count, elem_size, errp);
> > > +    }
> > > +}
> > > +
> > > +void visit_next_array(Visitor *v, Error **errp)
> > > +{
> > > +    if (!error_is_set(errp)) {
> > > +        v->next_array(v, errp);
> > > +    }
> > > +}
> > > +
> > > +void visit_end_array(Visitor *v, Error **errp)
> > > +{
> > > +    if (!error_is_set(errp)) {
> > > +        v->end_array(v, errp);
> > > +    }
> > > +}
> > > +
> > >  void visit_start_optional(Visitor *v, bool *present, const char *name,
> > >                            Error **errp)
> > >  {
> > > @@ -107,6 +129,14 @@ void visit_get_next_type(Visitor *v, int *obj, const int *qtypes,
> > >      }
> > >  }
> > >  
> > > +void visit_type_buffer(Visitor *v, void *data, size_t len, bool async,
> > > +                       const char *name, Error **errp)
> > > +{
> > > +    if (!error_is_set(errp)) {
> > > +        v->type_buffer(v, data, len, async, name, errp);
> > > +    }
> > > +}
> > > +
> > >  void visit_type_enum(Visitor *v, int *obj, const char *strings[],
> > >                       const char *kind, const char *name, Error **errp)
> > >  {
> > > @@ -291,6 +321,13 @@ void visit_type_str(Visitor *v, char **obj, const char *name, Error **errp)
> > >      }
> > >  }
> > >  
> > > +void visit_type_str256(Visitor *v, char *obj, const char *name, Error **errp)
> > > +{
> > > +    if (!error_is_set(errp)) {
> > > +        v->type_str256(v, obj, name, errp);
> > > +    }
> > > +}
> > > +
> > >  void visit_type_number(Visitor *v, double *obj, const char *name, Error **errp)
> > >  {
> > >      if (!error_is_set(errp)) {
> > > @@ -347,3 +384,46 @@ void input_type_enum(Visitor *v, int *obj, const char *strings[],
> > >      g_free(enum_str);
> > >      *obj = value;
> > >  }
> > > +
> > > +void visit_destroy(Visitor *v, Error **errp)
> > > +{
> > > +    v->destroy(v, errp);
> > > +}
> > > +
> > > +void visit_start_sequence_compat(Visitor *v, const char *name,
> > > +                                 Visit_seq_compat_mode compat_mode,
> > > +                                 void *opaque, Error **errp)
> > > +{
> > > +    if (error_is_set(errp)) {
> > > +        return;
> > > +    }
> > > +
> > > +    v->start_sequence_compat(v, name, compat_mode, opaque, errp);
> > > +}
> > > +
> > > +void visit_end_sequence_compat(Visitor *v, const char *name,
> > > +                                 Visit_seq_compat_mode compat_mode,
> > > +                               Error **errp)
> > > +{
> > > +    if (error_is_set(errp)) {
> > > +        return;
> > > +    }
> > > +
> > > +    v->end_sequence_compat(v, name, compat_mode, errp);
> > > +}
> > > +
> > > +QEMUFile *visitor_get_qemufile(Visitor *v)
> > > +{
> > > +    return v->get_qemufile(v);
> > > +}
> > > +
> > > +bool visitor_loading(Visitor *v)
> > > +{
> > > +    return v->flags & VISITOR_LOADING;
> > > +}
> > > +
> > > +bool visitor_saving(Visitor *v)
> > > +{
> > > +    return v->flags & VISITOR_SAVING;
> > > +}
> > > +
> > > -- 
> > > 1.9.0
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [RFC PATCH v2 00/16] visitor+BER migration format

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:

<snip>

> > OK but for a new machine type, let's default to BER, right?
> > I see no reason to keep supporting when non-BER when -M specifies 2.1
> > compatibility, do you?
> 
> I fail to see the relation between machine type and migration's wire
> encoding.

New machine types are a useful but not definitive line in the sand.  If
you enable something/change the default on a new machine type you know
it won't break any existing users since there aren't any.

Dve
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [RFC PATCH v2 00/16] visitor+BER migration format

[Michael S. Tsirkin]

On Tue, May 06, 2014 at 07:58:07PM +0100, Dr. David Alan Gilbert wrote:
> * Markus Armbruster (armbru@redhat.com) wrote:
> > "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> <snip>
> 
> > > OK but for a new machine type, let's default to BER, right?
> > > I see no reason to keep supporting when non-BER when -M specifies 2.1
> > > compatibility, do you?
> > 
> > I fail to see the relation between machine type and migration's wire
> > encoding.
> 
> New machine types are a useful but not definitive line in the sand.  If
> you enable something/change the default on a new machine type you know
> it won't break any existing users since there aren't any.
> 
> Dve

Exactly. And on the other hand, someone enabling old machine type
and doing live migration is likely to want to be compatible with old
qemu wrt migration.

> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [RFC PATCH v2 00/16] visitor+BER migration format

[Markus Armbruster]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Tue, May 06, 2014 at 07:58:07PM +0100, Dr. David Alan Gilbert wrote:
>> * Markus Armbruster (armbru@redhat.com) wrote:
>> > "Michael S. Tsirkin" <mst@redhat.com> writes:
>> 
>> <snip>
>> 
>> > > OK but for a new machine type, let's default to BER, right?
>> > > I see no reason to keep supporting when non-BER when -M specifies 2.1
>> > > compatibility, do you?
>> > 
>> > I fail to see the relation between machine type and migration's wire
>> > encoding.
>> 
>> New machine types are a useful but not definitive line in the sand.  If
>> you enable something/change the default on a new machine type you know
>> it won't break any existing users since there aren't any.
>> 
>> Dve

The purpose of machine types is to keep the guest ABI stable.  I don't
like tacking random crap unrelated to guest ABI to machine types.
They're hard enough to grasp for users as they are.

> Exactly. And on the other hand, someone enabling old machine type
> and doing live migration is likely to want to be compatible with old
> qemu wrt migration.

Machine types let you migrate to a newer QEMU (forward migration)
without messing up the guest ABI.  Migrating to an older QEMU (backward
migration) basically doesn't work, and as long as that's the case,
picking the older wire format by default is worthless.

Re: [Qemu-devel] [RFC PATCH v2 00/16] visitor+BER migration format

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Tue, May 06, 2014 at 07:58:07PM +0100, Dr. David Alan Gilbert wrote:
> >> * Markus Armbruster (armbru@redhat.com) wrote:
> >> > "Michael S. Tsirkin" <mst@redhat.com> writes:
> >> 
> >> <snip>
> >> 
> >> > > OK but for a new machine type, let's default to BER, right?
> >> > > I see no reason to keep supporting when non-BER when -M specifies 2.1
> >> > > compatibility, do you?
> >> > 
> >> > I fail to see the relation between machine type and migration's wire
> >> > encoding.
> >> 
> >> New machine types are a useful but not definitive line in the sand.  If
> >> you enable something/change the default on a new machine type you know
> >> it won't break any existing users since there aren't any.
> >> 
> >> Dve
> 
> The purpose of machine types is to keep the guest ABI stable.  I don't
> like tacking random crap unrelated to guest ABI to machine types.
> They're hard enough to grasp for users as they are.
> 
> > Exactly. And on the other hand, someone enabling old machine type
> > and doing live migration is likely to want to be compatible with old
> > qemu wrt migration.
> 
> Machine types let you migrate to a newer QEMU (forward migration)
> without messing up the guest ABI.  Migrating to an older QEMU (backward
> migration) basically doesn't work, and as long as that's the case,
> picking the older wire format by default is worthless.

Anyway, we seem to have had a long conversation about the least complicated
part of this patch set.

I'd love some thoughts on the actual visitor interface, which IMHO is the
bit that's actually messy and needs some rethinking).

Dave
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [RFC PATCH v2 00/16] visitor+BER migration format

[Michael S. Tsirkin]

On Wed, Apr 23, 2014 at 12:57:43PM -0600, Eric Blake wrote:
> On 04/23/2014 11:54 AM, Michael S. Tsirkin wrote:
> > On Wed, Apr 23, 2014 at 11:47:21AM -0600, Eric Blake wrote:
> >> On 04/23/2014 11:16 AM, Dr. David Alan Gilbert wrote:
> >>> The one thing that the environment variable does make nice and easy,
> >>> for dev, is using it with existing test setups - e.g. running virt-test
> >>> in BER mode or existing mode.
> >> Hmm. Maybe a compromise - an environment variable determines the default
> >> state of the QMP capability (for use in running the testsuite in one
> >> mode or the other for all tests that don't explicitly set a mode), but
> >> the QMP command still allows toggling the state at runtime (the
> >> environment variable doesn't lock us out from changing away from the
> >> startup default).
> > 
> > Eric, could you explain the need for the runtime thing?
> 
> I thought I did - when migrating across machines to what we know is an
> older qemu, we want the old migration format; but when migrating to a
> file to be reloaded by the current (or newer) qemu, we want the new
> format since it is more robust.

It's more robust for adding new functionality/new machine types but for
old one I think we can assume it describes it sufficiently.

> But libvirt can't know up front whether
> the user starting a qemu instance has plans down the road of calling
> 'virsh save' (migrate to file) or 'virsh migrate' (migrate across
> machines) first.
> 
> On the other hand, you have a point that upstream tends to not care
> about migration from new qemu to old (that's one of the value-added
> tasks of downstream distros), and that we could merely get away with
> using the -m machine name as the witness of which form to use, with no
> need for an environment variable or any other runtime switch.
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 

Re: [Qemu-devel] [RFC PATCH v2 15/16] Wire in BER visitors

[Michael S. Tsirkin]

On Wed, Apr 23, 2014 at 05:37:48PM +0100, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> BER output visitor to be selected with env
> BER input visitor recognized by file header
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  include/migration/migration.h |   2 +
>  savevm.c                      | 105 +++++++++++++++++++++++++++++++-----------
>  2 files changed, 80 insertions(+), 27 deletions(-)
> 
> diff --git a/include/migration/migration.h b/include/migration/migration.h
> index 8111125..5d500ec 100644
> --- a/include/migration/migration.h
> +++ b/include/migration/migration.h
> @@ -27,6 +27,8 @@
>  #define QEMU_VM_FILE_VERSION_COMPAT  0x00000002
>  #define QEMU_VM_FILE_VERSION         0x00000003
>  
> +#define QEMU_VM_BER_FILE_MAGIC       0x7fcdc551
> +

Where does this come from? I presume this matches
something in BER format?

>  #define QEMU_VM_EOF                  0x00
>  #define QEMU_VM_SECTION_START        0x01
>  #define QEMU_VM_SECTION_PART         0x02
> diff --git a/savevm.c b/savevm.c
> index 515dd77..2bbbcdc 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -24,6 +24,8 @@
>  
>  #include "config-host.h"
>  #include "qemu-common.h"
> +#include "qapi/qemu-file-ber-output-visitor.h"
> +#include "qapi/qemu-file-ber-input-visitor.h"
>  #include "qapi/qemu-file-binary-input-visitor.h"
>  #include "qapi/qemu-file-binary-output-visitor.h"
>  #include "hw/hw.h"
> @@ -527,6 +529,34 @@ bool qemu_savevm_state_blocked(Error **errp)
>      return false;
>  }
>  
> +/* Return a visitor for use on the QEMUFile; visit_destroy should be
> + * called on it to clean it up.
> + */
> +static Visitor *qemu_savevm_get_visitor(QEMUFile *f)
> +{
> +    char *formatvar = getenv("QEMUMIGFORMAT");
> +
> +    if (formatvar && (!strcmp(formatvar, "BER"))) {
> +        QemuFileBEROutputVisitor *qfberov = qemu_file_ber_output_visitor_new(f);
> +        Visitor *v = qemu_file_ber_output_get_visitor(qfberov);
> +
> +        qemu_file_set_tmp_visitor(f, v);
> +        return v;
> +    }
> +
> +    if (formatvar) {
> +        error_report("QEMUMIGFORMAT set to unknown value '%s'", formatvar);
> +        assert(0);
> +    }
> +
> +    QemuFileBinOutputVisitor *qfbov = qemu_file_bin_output_visitor_new(f);
> +    Visitor *v = qemu_file_bin_output_get_visitor(qfbov);
> +
> +    qemu_file_set_tmp_visitor(f, v);
> +
> +    return v;
> +}
> +
>  void qemu_savevm_state_begin(QEMUFile *f,
>                               const MigrationParams *params)
>  {
> @@ -534,10 +564,7 @@ void qemu_savevm_state_begin(QEMUFile *f,
>      int ret;
>      Error *local_err = NULL;
>      SectionHeader sh;
> -
> -    QemuFileBinOutputVisitor *qfbov = qemu_file_bin_output_visitor_new(f);
> -    Visitor *v = qemu_file_bin_output_get_visitor(qfbov);
> -    qemu_file_set_tmp_visitor(f, v);
> +    Visitor *v = qemu_savevm_get_visitor(f);
>  
>      trace_savevm_state_begin();
>      QTAILQ_FOREACH(se, &savevm_handlers, entry) {
> @@ -789,13 +816,11 @@ static int qemu_savevm_state(QEMUFile *f)
>  
>  static int qemu_save_device_state(QEMUFile *f)
>  {
> +    Visitor *v = qemu_savevm_get_visitor(f);
>      SaveStateEntry *se;
>      Error *local_err;
>      SectionHeader sh;
>  
> -    QemuFileBinOutputVisitor *qfbov = qemu_file_bin_output_visitor_new(f);
> -    qemu_file_set_tmp_visitor(f, qemu_file_bin_output_get_visitor(qfbov));
> -    Visitor *v = qemu_file_bin_output_get_visitor(qfbov);
>      int32_t section_type;
>  
>      cpu_synchronize_all_states();
> @@ -861,41 +886,65 @@ typedef struct LoadStateEntry {
>      int version_id;
>  } LoadStateEntry;
>  
> +/* Given a just opened input QEMUFile, peak at the header and figure
> + * out which visitor should be used for it.
> + * Return the visitor ready for use.
> + */
> +static Visitor *pick_input_visitor(QEMUFile *f)
> +{
> +    uint32_t tmp32;
> +
> +    if (qemu_peek_buffer(f, (uint8_t *)&tmp32, 4, 0) != 4) {
> +        fprintf(stderr, "Failed to read SaveVM header word\n");
> +        return NULL;
> +    }
> +
> +    tmp32 = be32_to_cpu(tmp32);
> +
> +    switch (tmp32) {
> +    case QEMU_VM_FILE_MAGIC: {
> +        QemuFileBinInputVisitor *qfbiv = qemu_file_bin_input_visitor_new(f);
> +        Visitor *v = qemu_file_bin_input_get_visitor(qfbiv);
> +        qemu_file_set_tmp_visitor(f, v);
> +
> +        return v;
> +        }
> +
> +    case QEMU_VM_BER_FILE_MAGIC: {
> +        QemuFileBERInputVisitor *qfberiv = qemu_file_ber_input_visitor_new(f);
> +        Visitor *v = qemu_file_ber_input_get_visitor(qfberiv);
> +        qemu_file_set_tmp_visitor(f, v);
> +
> +        return v;
> +        }
> +
> +    default:
> +        fprintf(stderr, "Invalid SaveVM header (0x%x)\n", tmp32);
> +        return NULL;
> +    }
> +}
> +
>  int qemu_loadvm_state(QEMUFile *f)
>  {
>      QLIST_HEAD(, LoadStateEntry) loadvm_handlers =
>          QLIST_HEAD_INITIALIZER(loadvm_handlers);
>      LoadStateEntry *le, *new_le;
> +    Visitor *v;
>      Error *local_err = NULL;
>      int32_t section_type;
> -    unsigned int tmp;
>      int ret;
>  
>      if (qemu_savevm_state_blocked(NULL)) {
>          return -EINVAL;
>      }
>  
> -    tmp = qemu_get_be32(f);
> -    if (tmp != QEMU_VM_FILE_MAGIC) {
> -        return -EINVAL;
> -    }
> -
> -    tmp = qemu_get_be32(f);
> -    if (tmp == QEMU_VM_FILE_VERSION_COMPAT) {
> -        error_report("SaveVM v2 format is obsolete and don't work anymore");
> +    v = pick_input_visitor(f);
> +    if (!v) {
>          return -ENOTSUP;
>      }
> -    if (tmp != QEMU_VM_FILE_VERSION) {
> -        return -ENOTSUP;
> -    }
> -
> -    /* TODO: Here we should be able to figure out if it's a binary file
> -     * or what and open the right type of visitor
> -     */
> -    QemuFileBinInputVisitor *qfbiv = qemu_file_bin_input_visitor_new(f);
> -    Visitor *v = qemu_file_bin_input_get_visitor(qfbiv);
> -    qemu_file_set_tmp_visitor(f, v);
>  
> +    visit_start_sequence_compat(v, "file", VISIT_SEQ_COMPAT_FILE, NULL,
> +                                &local_err);
>      visit_start_sequence_compat(v, "top", VISIT_SEQ_COMPAT_BYTE0TERM,
>                                  NULL, &local_err);
>      while (visit_get_next_type(v, &section_type, NULL, "section", &local_err),
> @@ -912,6 +961,7 @@ int qemu_loadvm_state(QEMUFile *f)
>                        "secstart" : "secfull",
>                    VISIT_SEQ_COMPAT_SECTION_HEADER, &sh, &local_err);
>              if (local_err) {
> +                error_report("%s", error_get_pretty(local_err));
>                  ret = -EINVAL;
>                  goto out;
>              }
> @@ -1001,6 +1051,7 @@ out:
>      }
>  
>      visit_end_sequence_compat(v, "top", VISIT_SEQ_COMPAT_BYTE0TERM, &local_err);
> +    visit_end_sequence_compat(v, "file", VISIT_SEQ_COMPAT_FILE, &local_err);
>      if (local_err) {
>          error_report("%s", error_get_pretty(local_err));
>          ret = -EINVAL;
> @@ -1010,7 +1061,7 @@ out:
>          ret = qemu_file_get_error(f);
>      }
>  
> -    qemu_file_bin_input_visitor_cleanup(qfbiv);
> +    visit_destroy(v, &local_err);
>      return ret;
>  }
>  
> -- 
> 1.9.0

Re: [Qemu-devel] [RFC PATCH v2 15/16] Wire in BER visitors

[Dr. David Alan Gilbert]

* Michael S. Tsirkin (mst@redhat.com) wrote:
> On Wed, Apr 23, 2014 at 05:37:48PM +0100, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > BER output visitor to be selected with env
> > BER input visitor recognized by file header
> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
> >  include/migration/migration.h |   2 +
> >  savevm.c                      | 105 +++++++++++++++++++++++++++++++-----------
> >  2 files changed, 80 insertions(+), 27 deletions(-)
> > 
> > diff --git a/include/migration/migration.h b/include/migration/migration.h
> > index 8111125..5d500ec 100644
> > --- a/include/migration/migration.h
> > +++ b/include/migration/migration.h
> > @@ -27,6 +27,8 @@
> >  #define QEMU_VM_FILE_VERSION_COMPAT  0x00000002
> >  #define QEMU_VM_FILE_VERSION         0x00000003
> >  
> > +#define QEMU_VM_BER_FILE_MAGIC       0x7fcdc551
> > +
> 
> Where does this come from? I presume this matches
> something in BER format?

See ber.h, this corresponds to an Application class tag with
the BER_TYPE_QEMU_FILE; from ber.h:

    BER_TYPE_QEMU_FILE = 1270481, /* come out as 7f cd c5 51  - the 51 is Q
                                   * the c5 and cd being E,M but with the top
                                   * bit set which BER requires.

I'll add a comment pointing to that.

Dave

> 
> >  #define QEMU_VM_EOF                  0x00
> >  #define QEMU_VM_SECTION_START        0x01
> >  #define QEMU_VM_SECTION_PART         0x02
> > diff --git a/savevm.c b/savevm.c
> > index 515dd77..2bbbcdc 100644
> > --- a/savevm.c
> > +++ b/savevm.c
> > @@ -24,6 +24,8 @@
> >  
> >  #include "config-host.h"
> >  #include "qemu-common.h"
> > +#include "qapi/qemu-file-ber-output-visitor.h"
> > +#include "qapi/qemu-file-ber-input-visitor.h"
> >  #include "qapi/qemu-file-binary-input-visitor.h"
> >  #include "qapi/qemu-file-binary-output-visitor.h"
> >  #include "hw/hw.h"
> > @@ -527,6 +529,34 @@ bool qemu_savevm_state_blocked(Error **errp)
> >      return false;
> >  }
> >  
> > +/* Return a visitor for use on the QEMUFile; visit_destroy should be
> > + * called on it to clean it up.
> > + */
> > +static Visitor *qemu_savevm_get_visitor(QEMUFile *f)
> > +{
> > +    char *formatvar = getenv("QEMUMIGFORMAT");
> > +
> > +    if (formatvar && (!strcmp(formatvar, "BER"))) {
> > +        QemuFileBEROutputVisitor *qfberov = qemu_file_ber_output_visitor_new(f);
> > +        Visitor *v = qemu_file_ber_output_get_visitor(qfberov);
> > +
> > +        qemu_file_set_tmp_visitor(f, v);
> > +        return v;
> > +    }
> > +
> > +    if (formatvar) {
> > +        error_report("QEMUMIGFORMAT set to unknown value '%s'", formatvar);
> > +        assert(0);
> > +    }
> > +
> > +    QemuFileBinOutputVisitor *qfbov = qemu_file_bin_output_visitor_new(f);
> > +    Visitor *v = qemu_file_bin_output_get_visitor(qfbov);
> > +
> > +    qemu_file_set_tmp_visitor(f, v);
> > +
> > +    return v;
> > +}
> > +
> >  void qemu_savevm_state_begin(QEMUFile *f,
> >                               const MigrationParams *params)
> >  {
> > @@ -534,10 +564,7 @@ void qemu_savevm_state_begin(QEMUFile *f,
> >      int ret;
> >      Error *local_err = NULL;
> >      SectionHeader sh;
> > -
> > -    QemuFileBinOutputVisitor *qfbov = qemu_file_bin_output_visitor_new(f);
> > -    Visitor *v = qemu_file_bin_output_get_visitor(qfbov);
> > -    qemu_file_set_tmp_visitor(f, v);
> > +    Visitor *v = qemu_savevm_get_visitor(f);
> >  
> >      trace_savevm_state_begin();
> >      QTAILQ_FOREACH(se, &savevm_handlers, entry) {
> > @@ -789,13 +816,11 @@ static int qemu_savevm_state(QEMUFile *f)
> >  
> >  static int qemu_save_device_state(QEMUFile *f)
> >  {
> > +    Visitor *v = qemu_savevm_get_visitor(f);
> >      SaveStateEntry *se;
> >      Error *local_err;
> >      SectionHeader sh;
> >  
> > -    QemuFileBinOutputVisitor *qfbov = qemu_file_bin_output_visitor_new(f);
> > -    qemu_file_set_tmp_visitor(f, qemu_file_bin_output_get_visitor(qfbov));
> > -    Visitor *v = qemu_file_bin_output_get_visitor(qfbov);
> >      int32_t section_type;
> >  
> >      cpu_synchronize_all_states();
> > @@ -861,41 +886,65 @@ typedef struct LoadStateEntry {
> >      int version_id;
> >  } LoadStateEntry;
> >  
> > +/* Given a just opened input QEMUFile, peak at the header and figure
> > + * out which visitor should be used for it.
> > + * Return the visitor ready for use.
> > + */
> > +static Visitor *pick_input_visitor(QEMUFile *f)
> > +{
> > +    uint32_t tmp32;
> > +
> > +    if (qemu_peek_buffer(f, (uint8_t *)&tmp32, 4, 0) != 4) {
> > +        fprintf(stderr, "Failed to read SaveVM header word\n");
> > +        return NULL;
> > +    }
> > +
> > +    tmp32 = be32_to_cpu(tmp32);
> > +
> > +    switch (tmp32) {
> > +    case QEMU_VM_FILE_MAGIC: {
> > +        QemuFileBinInputVisitor *qfbiv = qemu_file_bin_input_visitor_new(f);
> > +        Visitor *v = qemu_file_bin_input_get_visitor(qfbiv);
> > +        qemu_file_set_tmp_visitor(f, v);
> > +
> > +        return v;
> > +        }
> > +
> > +    case QEMU_VM_BER_FILE_MAGIC: {
> > +        QemuFileBERInputVisitor *qfberiv = qemu_file_ber_input_visitor_new(f);
> > +        Visitor *v = qemu_file_ber_input_get_visitor(qfberiv);
> > +        qemu_file_set_tmp_visitor(f, v);
> > +
> > +        return v;
> > +        }
> > +
> > +    default:
> > +        fprintf(stderr, "Invalid SaveVM header (0x%x)\n", tmp32);
> > +        return NULL;
> > +    }
> > +}
> > +
> >  int qemu_loadvm_state(QEMUFile *f)
> >  {
> >      QLIST_HEAD(, LoadStateEntry) loadvm_handlers =
> >          QLIST_HEAD_INITIALIZER(loadvm_handlers);
> >      LoadStateEntry *le, *new_le;
> > +    Visitor *v;
> >      Error *local_err = NULL;
> >      int32_t section_type;
> > -    unsigned int tmp;
> >      int ret;
> >  
> >      if (qemu_savevm_state_blocked(NULL)) {
> >          return -EINVAL;
> >      }
> >  
> > -    tmp = qemu_get_be32(f);
> > -    if (tmp != QEMU_VM_FILE_MAGIC) {
> > -        return -EINVAL;
> > -    }
> > -
> > -    tmp = qemu_get_be32(f);
> > -    if (tmp == QEMU_VM_FILE_VERSION_COMPAT) {
> > -        error_report("SaveVM v2 format is obsolete and don't work anymore");
> > +    v = pick_input_visitor(f);
> > +    if (!v) {
> >          return -ENOTSUP;
> >      }
> > -    if (tmp != QEMU_VM_FILE_VERSION) {
> > -        return -ENOTSUP;
> > -    }
> > -
> > -    /* TODO: Here we should be able to figure out if it's a binary file
> > -     * or what and open the right type of visitor
> > -     */
> > -    QemuFileBinInputVisitor *qfbiv = qemu_file_bin_input_visitor_new(f);
> > -    Visitor *v = qemu_file_bin_input_get_visitor(qfbiv);
> > -    qemu_file_set_tmp_visitor(f, v);
> >  
> > +    visit_start_sequence_compat(v, "file", VISIT_SEQ_COMPAT_FILE, NULL,
> > +                                &local_err);
> >      visit_start_sequence_compat(v, "top", VISIT_SEQ_COMPAT_BYTE0TERM,
> >                                  NULL, &local_err);
> >      while (visit_get_next_type(v, &section_type, NULL, "section", &local_err),
> > @@ -912,6 +961,7 @@ int qemu_loadvm_state(QEMUFile *f)
> >                        "secstart" : "secfull",
> >                    VISIT_SEQ_COMPAT_SECTION_HEADER, &sh, &local_err);
> >              if (local_err) {
> > +                error_report("%s", error_get_pretty(local_err));
> >                  ret = -EINVAL;
> >                  goto out;
> >              }
> > @@ -1001,6 +1051,7 @@ out:
> >      }
> >  
> >      visit_end_sequence_compat(v, "top", VISIT_SEQ_COMPAT_BYTE0TERM, &local_err);
> > +    visit_end_sequence_compat(v, "file", VISIT_SEQ_COMPAT_FILE, &local_err);
> >      if (local_err) {
> >          error_report("%s", error_get_pretty(local_err));
> >          ret = -EINVAL;
> > @@ -1010,7 +1061,7 @@ out:
> >          ret = qemu_file_get_error(f);
> >      }
> >  
> > -    qemu_file_bin_input_visitor_cleanup(qfbiv);
> > +    visit_destroy(v, &local_err);
> >      return ret;
> >  }
> >  
> > -- 
> > 1.9.0
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK