Patchwork [RFC,01/16] Visitor: Add methods for migration format use

login
register
mail settings
Submitter Dave Gilbert
Date March 25, 2014, 8:17 p.m.
Message ID <1395778647-30925-2-git-send-email-dgilbert@redhat.com>
Download mbox | patch
Permalink /patch/333708/
State New
Headers show

Comments

Dave Gilbert - March 25, 2014, 8:17 p.m.
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(+)
Markus Armbruster - June 5, 2014, 5 p.m.
"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:

> 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

Naive question: why do you need this when you have arrays?

> buffer type
>     For a blob of data that the caller wants to deliver whole (e.g.
>     a page of RAM or block of disk)

This smells like an array, too.

> Load/save flags to let a user perform pre-save/post-load checking

Odd.  I'd expect separate visitors, one for save, one for load.

> 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.

When the commit message is basically a list, splitting it into one
commit per list item often makes sense.

Some of them (the one introducing destroy, perhaps?) can then be put to
use in the same or the next patch.  Makes review much easier.

But on to the actual code.

> 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)
> +

Comment explaining their meaning and the connection to Visitor member
flags needed.

Are the flags independent, i.e. all four combinations meaningful?

>  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);

Since these "must be set", you need to update all existing visitors to
actually set them, in the same patch, don't you?

I'm not sure the existing visitors completely obey "must be set"
vs. "may be null".  But let's not make it worse.

> @@ -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);
>  
       /* May be NULL */

More context ^^^

Members above must be set, members below may be null.

> @@ -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);

No longer applies (I killed end_handle() in commit cbc955).

> +    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);

Style nit: QEMUFile *(*get_qemufile)(Visitor *v);

The empty line before destroy makes me wonder whether these still belong
to the "May be NULL" heading.

> +
> +    uint64_t flags;

This one can't, because it can't be null :)

>  };
>  
>  void input_type_enum(Visitor *v, int *obj, const char *strings[],

Your patch drags migration-specific stuff into the until now perfectly
generic struct Visitor:

* get_qemufile()

  Looks temporary, thus tolerable.

* Compat sequences
* Load/save flags

  These look permanent :(

  I'd have to review how they're used to convince myself we actually
  need them.

> 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);

I'm afraid your specification of async is tied to a specific kind of
visitor: one that "writes".  Many don't.

>  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;
> +
> +/* 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 */

I'm afraid I don't get this comment.

> +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);

Likewise.

>  
>  #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);
> +    }
> +}
> +

Looks like Visitor member type_buffer must be set.  However, you put it
under the "may be null" heading in visitor-impl.h.

>  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);
> +}

Likewise.

> +
> +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);
> +}

Likewise.

> +
> +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);
> +}

Likewise.

> +
> +QEMUFile *visitor_get_qemufile(Visitor *v)
> +{
> +    return v->get_qemufile(v);
> +}

Likewise.

> +
> +bool visitor_loading(Visitor *v)
> +{
> +    return v->flags & VISITOR_LOADING;
> +}
> +
> +bool visitor_saving(Visitor *v)
> +{
> +    return v->flags & VISITOR_SAVING;
> +}
> +
Dave Gilbert - June 5, 2014, 5:43 p.m.
* Markus Armbruster (armbru@redhat.com) wrote:
> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:
> 

Hi Markus,
  Thanks for the review.  I've reordered your comments to group some of the
replies together.

> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

> Your patch drags migration-specific stuff into the until now perfectly
> generic struct Visitor:
> 
> * get_qemufile()
> 
>   Looks temporary, thus tolerable.
> 
> * Compat sequences
> * Load/save flags
> 
>   These look permanent :(
> 
>   I'd have to review how they're used to convince myself we actually
>   need them.

Right - and this is the one I actually wanted you to look at this series
for; I know it feels weird, and I'm up for better suggestions
I've toyed with the idea of having a MigrationVisitor subclass - ie just like
the visitor except the few additions; but how would I cleanly make it a subclass
in this setup?

> > 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
> 
> Naive question: why do you need this when you have arrays?

> > buffer type
> >     For a blob of data that the caller wants to deliver whole (e.g.
> >     a page of RAM or block of disk)
> 
> This smells like an array, too.

I'd assumed that the point of the 'array' was something where you started
an array and then visited each member (with the appropriate visitor type
for the type of the member).

I use arrays for things that are declared as such in vmstate and which
I know the internal types of;

BER has various string types and it makes sense to me to use that
for readable strings. The other reason in this case is that the
binary-visitor knows to format this in the way compatible with the
existing length+text string formatting.

Where there is a blob of data which the visitor code isn't going to
get to know the inside of, that's a buffer.

> > Load/save flags to let a user perform pre-save/post-load checking
> 
> Odd.  I'd expect separate visitors, one for save, one for load.

Indeed they are - these flags are to allow a caller to know whether
it's being visited for the purpose of saving or loading;
In a non-visitor world you typically have a:
   load_state
   save_state
function; in the visit world you more commonly have a:
   visit_state
function; for a lot of the simpler types this works fine - but when
you want to do some reformatting or checking of your data then
you really want to know if you're saving or loading.
The other way of doing it would still be to have a load_state/save_state
that just happen to call the same function in most cases.

> > +#define VISITOR_SAVING (1<<0)
> > +#define VISITOR_LOADING (1<<1)
> > +
> 
> Comment explaining their meaning and the connection to Visitor member
> flags needed.
> 
> Are the flags independent, i.e. all four combinations meaningful?

They're only two states, and I could have encoded it as one bit in the flag.

> > 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.
> 
> When the commit message is basically a list, splitting it into one
> commit per list item often makes sense.
> 
> Some of them (the one introducing destroy, perhaps?) can then be put to
> use in the same or the next patch.  Makes review much easier.

Yep, happy to split.

> But on to the actual code.
> 
> > 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"
> >  
> 
> >  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);
> 
> Since these "must be set", you need to update all existing visitors to
> actually set them, in the same patch, don't you?
> 
> I'm not sure the existing visitors completely obey "must be set"
> vs. "may be null".  But let's not make it worse.

Yep; you've picked me up on a whole bunch of the 'must be set'/'may be null'
- I'll treat those all as one and go through and tidy them up.

> Members above must be set, members below may be null.
> 
> > @@ -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);
> 
> No longer applies (I killed end_handle() in commit cbc955).

Yep; it's dead in my working tree.

> > +    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);
> 
> Style nit: QEMUFile *(*get_qemufile)(Visitor *v);

Fixed.

> The empty line before destroy makes me wonder whether these still belong
> to the "May be NULL" heading.
> 
> > +
> > +    uint64_t flags;
> 
> This one can't, because it can't be null :)
> 

> > +/* 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);
> 
> I'm afraid your specification of async is tied to a specific kind of
> visitor: one that "writes".  Many don't.

Do you have a suggestion on how to do this better? Async accesses were
put in some time ago apparently for a useful speed increase, so I can't
remove them, and anyway they make sense for the writes; I guess you could
argue that they could be used for read as well, if the time at which
the 'async' had to have completed was defined, but that's needed for the
write anyway.

> >  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;
> > +
> > +/* 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);
> > +
> 
> I'm afraid I don't get this comment.

Quite a lot of the structures I need to keep compatibility with are
lists of some mixture of different things with a terminator on the end
(there's not much consistency in the binary format about how you tell
what's next or what the terminator is).
So the typical use is:

    visit_start_sequence_compat(...)
    while (visit_get_next_type( &flag ), flag != end_flag) {
       switch (flag) {
       case A:
          do whatever you need to do now that we've found an 'A' in the list
          (typically another visit_start_sequence_compat for that type)
       case B:
          do whatever you need to do now that we've found an 'B' in the list
   ....
       }
    }
    visit_end_sequence_compat(...)

An example is a RAM migration section; it's a list of initialisation data,
normal pages, all-zero pages, XBZRLE pages

> > +/* Don't Use! - lets us move forward until we can get rid of all file uses */
> > +QEMUFile *visitor_get_qemufile(Visitor *v);
> 
> Likewise.

I'm hoping this is a temporary shim - my hope eventually is that we won't need to
use this and everywhere that uses Visitors won't also need to get a QEMUFile
except internally to the visitor.

Dave

--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Dave Gilbert - June 5, 2014, 6:59 p.m.
* Dr. David Alan Gilbert (dgilbert@redhat.com) wrote:
> * Markus Armbruster (armbru@redhat.com) wrote:
> > * Dr. David Alan Gilbert (dgilbert@redhat.com) wrote:

<snip>

> > > Load/save flags to let a user perform pre-save/post-load checking
> > 
> > Odd.  I'd expect separate visitors, one for save, one for load.
> 
> Indeed they are - these flags are to allow a caller to know whether

Oops that's a typo - 'Indeed there are separate load and save visitors'

--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Markus Armbruster - June 6, 2014, 8:19 a.m.
"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:

> * Markus Armbruster (armbru@redhat.com) wrote:
>> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:
>> 
>
> Hi Markus,
>   Thanks for the review.  I've reordered your comments to group some of the
> replies together.
>
>> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
>> Your patch drags migration-specific stuff into the until now perfectly
>> generic struct Visitor:
>> 
>> * get_qemufile()
>> 
>>   Looks temporary, thus tolerable.
>> 
>> * Compat sequences
>> * Load/save flags
>> 
>>   These look permanent :(
>> 
>>   I'd have to review how they're used to convince myself we actually
>>   need them.
>
> Right - and this is the one I actually wanted you to look at this series
> for; I know it feels weird, and I'm up for better suggestions
> I've toyed with the idea of having a MigrationVisitor subclass - ie just like
> the visitor except the few additions; but how would I cleanly make it a subclass
> in this setup?

To help with that, I need to understand the problems your additional
functionality solves.  I could try to find out by studying the rest of
this series, but it'll take time.  Perhaps I can find some next week.

>> > 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
>> 
>> Naive question: why do you need this when you have arrays?
>
>> > buffer type
>> >     For a blob of data that the caller wants to deliver whole (e.g.
>> >     a page of RAM or block of disk)
>> 
>> This smells like an array, too.
>
> I'd assumed that the point of the 'array' was something where you started
> an array and then visited each member (with the appropriate visitor type
> for the type of the member).
>
> I use arrays for things that are declared as such in vmstate and which
> I know the internal types of;
>
> BER has various string types and it makes sense to me to use that
> for readable strings. The other reason in this case is that the
> binary-visitor knows to format this in the way compatible with the
> existing length+text string formatting.

Point taken: while (unboxed) strings really are just special arrays, it
can make sense to treat them specially.

But limiting it to arrays of exactly 256 characters is lame :)  Would
generalizing to arbitrary size be hard?

This one's not specific to migration.

> Where there is a blob of data which the visitor code isn't going to
> get to know the inside of, that's a buffer.

Similar argument as for strings: blobs are just special arrays, but it
can make sense to treat them specially.  Not sure I find it convincing,
but examining actual users tends to fix that.

Again, not necessarily specific to migration.

>> > Load/save flags to let a user perform pre-save/post-load checking
>> 
>> Odd.  I'd expect separate visitors, one for save, one for load.
>
> Indeed they are - these flags are to allow a caller to know whether
> it's being visited for the purpose of saving or loading;
> In a non-visitor world you typically have a:
>    load_state
>    save_state
> function; in the visit world you more commonly have a:
>    visit_state
> function; for a lot of the simpler types this works fine - but when
> you want to do some reformatting or checking of your data then
> you really want to know if you're saving or loading.
> The other way of doing it would still be to have a load_state/save_state
> that just happen to call the same function in most cases.

The purpose of visitors is to separate data structure traversal and what
to do with the data.

An implementation of the Visitor type encapsulates "what to do".  The
code passing a Visitor instance v to the visit_FOO(v, ...)  does the
traversal.

In other words, the abstract Visitor type hides "what to do".

Your flags leak "what to do" bits into the abstract Visitor type
interface.  Unclean.

The orthodox way to provide additional data to a concrete visitor is
extending the abstract Visitor type:

    struct FooVisitor {
        Visitor visitor;
        ... Foo-specific stuff ...
    }

This is how the existing visitors work.  Could this work for you, too?

>> > +#define VISITOR_SAVING (1<<0)
>> > +#define VISITOR_LOADING (1<<1)
>> > +
>> 
>> Comment explaining their meaning and the connection to Visitor member
>> flags needed.
>> 
>> Are the flags independent, i.e. all four combinations meaningful?
>
> They're only two states, and I could have encoded it as one bit in the flag.

I prefer to avoid invalid states.

>> > 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.
>> 
>> When the commit message is basically a list, splitting it into one
>> commit per list item often makes sense.
>> 
>> Some of them (the one introducing destroy, perhaps?) can then be put to
>> use in the same or the next patch.  Makes review much easier.
>
> Yep, happy to split.
>
>> But on to the actual code.
>> 
>> > 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"
>> >  
>> 
>> >  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);
>> 
>> Since these "must be set", you need to update all existing visitors to
>> actually set them, in the same patch, don't you?
>> 
>> I'm not sure the existing visitors completely obey "must be set"
>> vs. "may be null".  But let's not make it worse.
>
> Yep; you've picked me up on a whole bunch of the 'must be set'/'may be null'
> - I'll treat those all as one and go through and tidy them up.
>
>> Members above must be set, members below may be null.
>> 
>> > @@ -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);
>> 
>> No longer applies (I killed end_handle() in commit cbc955).
>
> Yep; it's dead in my working tree.

When I get to studying how you put this stuff to use, I'll want a
current version.  Happy to pull, just tell me where from.

>> > +    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);
>> 
>> Style nit: QEMUFile *(*get_qemufile)(Visitor *v);
>
> Fixed.
>
>> The empty line before destroy makes me wonder whether these still belong
>> to the "May be NULL" heading.
>> 
>> > +
>> > +    uint64_t flags;
>> 
>> This one can't, because it can't be null :)
>> 
>
>> > +/* 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);
>> 
>> I'm afraid your specification of async is tied to a specific kind of
>> visitor: one that "writes".  Many don't.
>
> Do you have a suggestion on how to do this better? Async accesses were
> put in some time ago apparently for a useful speed increase, so I can't
> remove them, and anyway they make sense for the writes; I guess you could
> argue that they could be used for read as well, if the time at which
> the 'async' had to have completed was defined, but that's needed for the
> write anyway.

How do we decide async / sync?

>> >  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;
>> > +
>> > +/* 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);
>> > +
>> 
>> I'm afraid I don't get this comment.
>
> Quite a lot of the structures I need to keep compatibility with are
> lists of some mixture of different things with a terminator on the end
> (there's not much consistency in the binary format about how you tell
> what's next or what the terminator is).
> So the typical use is:
>
>     visit_start_sequence_compat(...)
>     while (visit_get_next_type( &flag ), flag != end_flag) {
>        switch (flag) {
>        case A:
>           do whatever you need to do now that we've found an 'A' in the list
>           (typically another visit_start_sequence_compat for that type)
>        case B:
>           do whatever you need to do now that we've found an 'B' in the list
>    ....
>        }
>     }
>     visit_end_sequence_compat(...)
>
> An example is a RAM migration section; it's a list of initialisation data,
> normal pages, all-zero pages, XBZRLE pages

Doesn't match the existing visit_get_next_type().  I guess I need more
context.  One way to get it is study your patches.

>> > +/* Don't Use! - lets us move forward until we can get rid of all
>> > file uses */
>> > +QEMUFile *visitor_get_qemufile(Visitor *v);
>> 
>> Likewise.
>
> I'm hoping this is a temporary shim - my hope eventually is that we
> won't need to
> use this and everywhere that uses Visitors won't also need to get a QEMUFile
> except internally to the visitor.

Suggest to put that in the comment.
Dave Gilbert - June 6, 2014, 11:44 a.m.
* Markus Armbruster (armbru@redhat.com) wrote:
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
> 
> > * Markus Armbruster (armbru@redhat.com) wrote:
> >> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:
> >> 
> >
> > Hi Markus,
> >   Thanks for the review.  I've reordered your comments to group some of the
> > replies together.
> >
> >> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >
> >> Your patch drags migration-specific stuff into the until now perfectly
> >> generic struct Visitor:
> >> 
> >> * get_qemufile()
> >> 
> >>   Looks temporary, thus tolerable.
> >> 
> >> * Compat sequences
> >> * Load/save flags
> >> 
> >>   These look permanent :(
> >> 
> >>   I'd have to review how they're used to convince myself we actually
> >>   need them.
> >
> > Right - and this is the one I actually wanted you to look at this series
> > for; I know it feels weird, and I'm up for better suggestions
> > I've toyed with the idea of having a MigrationVisitor subclass - ie just like
> > the visitor except the few additions; but how would I cleanly make it a subclass
> > in this setup?
> 
> To help with that, I need to understand the problems your additional
> functionality solves.  I could try to find out by studying the rest of
> this series, but it'll take time.  Perhaps I can find some next week.

Hmm, that means I need to beef up my comments/commit messages to make that
more obvious.

OK, the quick overview is that I want to abstract out of the devices/ram/etc code
any notion of the format of the migration file, and then support two different formats,
the current binary format and the new BER format.  I end up with 4 visitors
for each of (Binary, BER) x (Input, output).

The tricky bit for the visitor inteface is matching the existing binary format;
ignoring the VMState (which is pretty easy) all the rest of the migration format
is frankly pretty random in the way it deals with lists of items or special formatting,
and each one needs treating slightly specially; although many are similar.

I didn't want to create a separate visitor method for each weird case in the binary format,
and so I created 'visit_start_sequence_compat' which takes an enum telling it which of
the special cases it's dealing with (and a pointer to a data structure for that case).

> >> > 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
> >> 
> >> Naive question: why do you need this when you have arrays?
> >
> >> > buffer type
> >> >     For a blob of data that the caller wants to deliver whole (e.g.
> >> >     a page of RAM or block of disk)
> >> 
> >> This smells like an array, too.
> >
> > I'd assumed that the point of the 'array' was something where you started
> > an array and then visited each member (with the appropriate visitor type
> > for the type of the member).
> >
> > I use arrays for things that are declared as such in vmstate and which
> > I know the internal types of;
> >
> > BER has various string types and it makes sense to me to use that
> > for readable strings. The other reason in this case is that the
> > binary-visitor knows to format this in the way compatible with the
> > existing length+text string formatting.
> 
> Point taken: while (unboxed) strings really are just special arrays, it
> can make sense to treat them specially.
> 
> But limiting it to arrays of exactly 256 characters is lame :)  Would
> generalizing to arbitrary size be hard?
> 
> This one's not specific to migration.

Probably not too hard; the problem I have here is that I know that
the binary visitor must encode that as a single byte length followed
by the string; now I could just take the '256' off the name and
error in the case of someone passing a string longer than 255 bytes
to it for the binary visitor.   The other thing I would have to
do is add a string copy (at the moment the input visitor here
assumes it's passed a pointer to a nice roomy 256 byte buffer).

> > Where there is a blob of data which the visitor code isn't going to
> > get to know the inside of, that's a buffer.
> 
> Similar argument as for strings: blobs are just special arrays, but it
> can make sense to treat them specially.  Not sure I find it convincing,
> but examining actual users tends to fix that.
> 
> Again, not necessarily specific to migration.

Let me give some examples from a decoded BER stream (from the unber tool):

A string:
<P O="105" T="[UNIVERSAL 12]" TL="2" V="22" A="UTF8String">0000:00:03.0/e1000.rom</P>

A buffer:
<P O="289" T="[UNIVERSAL 4]" TL="4" V="4096" A="OCTET STRING">   (Now follows 4k of hex data)</P>

An array:
     <I O="1589207" T="[UNIVERSAL 16]" TL="2" V="Indefinite" A="SEQUENCE">
        <P O="1589209" T="[UNIVERSAL 2]" TL="2" V="2" A="INTEGER" F>4416</P>
        <P O="1589213" T="[UNIVERSAL 2]" TL="2" V="2" A="INTEGER" F>31053</P>
        <P O="1589217" T="[UNIVERSAL 2]" TL="2" V="2" A="INTEGER" F>321</P>
        <P O="1589221" T="[UNIVERSAL 2]" TL="2" V="2" A="INTEGER" F>3104</P>
        <P O="1589311" T="[UNIVERSAL 2]" TL="2" V="1" A="INTEGER" F>0</P>
        <P O="1589314" T="[UNIVERSAL 2]" TL="2" V="1" A="INTEGER" F>0</P>
     </I O="1589317" T="[UNIVERSAL 0]" TL="2" L="112">

> >> > Load/save flags to let a user perform pre-save/post-load checking
> >> 
> >> Odd.  I'd expect separate visitors, one for save, one for load.
> >
> > Indeed they are - these flags are to allow a caller to know whether
> > it's being visited for the purpose of saving or loading;
> > In a non-visitor world you typically have a:
> >    load_state
> >    save_state
> > function; in the visit world you more commonly have a:
> >    visit_state
> > function; for a lot of the simpler types this works fine - but when
> > you want to do some reformatting or checking of your data then
> > you really want to know if you're saving or loading.
> > The other way of doing it would still be to have a load_state/save_state
> > that just happen to call the same function in most cases.
> 
> The purpose of visitors is to separate data structure traversal and what
> to do with the data.
> 
> An implementation of the Visitor type encapsulates "what to do".  The
> code passing a Visitor instance v to the visit_FOO(v, ...)  does the
> traversal.
> 
> In other words, the abstract Visitor type hides "what to do".
> 
> Your flags leak "what to do" bits into the abstract Visitor type
> interface.  Unclean.
> 
> The orthodox way to provide additional data to a concrete visitor is
> extending the abstract Visitor type:
> 
>     struct FooVisitor {
>         Visitor visitor;
>         ... Foo-specific stuff ...
>     }
> 
> This is how the existing visitors work.  Could this work for you, too?

Note that these flags are only accessed by the:

  bool visitor_loading(Visitor *v);
  bool visitor_saving(Visitor *v);

methods; those magic constants are never used by the callers.

an example user is traversing the int32_le types in vmstate:

static int visit_int32_le(Visitor *v, void *pdata, const char *name,
                          size_t size, Error **errp)
{
    if (visitor_loading(v)) {
        int32_t loaded;

        visit_type_int32(v, &loaded, name, errp);
        if (loaded >= 0 && loaded <= *(int32_t *)pdata) {
            *(int32_t *)pdata = loaded;
            return 0;
        }
        error_setg(errp, "Unexpected value for field '%s', expected a value"
                   " no larger than %d but got %d", name, *(int32_t *)pdata,
                   loaded);
        return -EINVAL;
    } else {
        visit_type_int32(v, (int32_t *)pdata, name, errp);
        return 0;
    }

}

all the more basic vmstate wrappers just directly call visit_type_whatever without
careing if they're load/store.

> >> > +#define VISITOR_SAVING (1<<0)
> >> > +#define VISITOR_LOADING (1<<1)
> >> > +
> >> 
> >> Comment explaining their meaning and the connection to Visitor member
> >> flags needed.
> >> 
> >> Are the flags independent, i.e. all four combinations meaningful?
> >
> > They're only two states, and I could have encoded it as one bit in the flag.
> 
> I prefer to avoid invalid states.

Yep, fair enough.

> >> > 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.
> >> 
> >> When the commit message is basically a list, splitting it into one
> >> commit per list item often makes sense.
> >> 
> >> Some of them (the one introducing destroy, perhaps?) can then be put to
> >> use in the same or the next patch.  Makes review much easier.
> >
> > Yep, happy to split.
> >
> >> But on to the actual code.
> >> 
> >> > 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"
> >> >  
> >> 
> >> >  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);
> >> 
> >> Since these "must be set", you need to update all existing visitors to
> >> actually set them, in the same patch, don't you?
> >> 
> >> I'm not sure the existing visitors completely obey "must be set"
> >> vs. "may be null".  But let's not make it worse.
> >
> > Yep; you've picked me up on a whole bunch of the 'must be set'/'may be null'
> > - I'll treat those all as one and go through and tidy them up.
> >
> >> Members above must be set, members below may be null.
> >> 
> >> > @@ -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);
> >> 
> >> No longer applies (I killed end_handle() in commit cbc955).
> >
> > Yep; it's dead in my working tree.
> 
> When I get to studying how you put this stuff to use, I'll want a
> current version.  Happy to pull, just tell me where from.

OK, will do.

> >> > +    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);
> >> 
> >> Style nit: QEMUFile *(*get_qemufile)(Visitor *v);
> >
> > Fixed.
> >
> >> The empty line before destroy makes me wonder whether these still belong
> >> to the "May be NULL" heading.
> >> 
> >> > +
> >> > +    uint64_t flags;
> >> 
> >> This one can't, because it can't be null :)
> >> 
> >
> >> > +/* 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);
> >> 
> >> I'm afraid your specification of async is tied to a specific kind of
> >> visitor: one that "writes".  Many don't.
> >
> > Do you have a suggestion on how to do this better? Async accesses were
> > put in some time ago apparently for a useful speed increase, so I can't
> > remove them, and anyway they make sense for the writes; I guess you could
> > argue that they could be used for read as well, if the time at which
> > the 'async' had to have completed was defined, but that's needed for the
> > write anyway.
> 
> How do we decide async / sync?

The one current use is for RAM migration; in most cases we just want to throw
the 'current' ram data out on the wire and don't particularly care if we
send a newer version out by the time the write() gets to it and hence set
async; the gotcha is in the xbzrle case where the xbzrle cache has to match
exactly what was sent to the destination and hence we send it sync.
Note that the 'async' in this case is being used to deal with the version/validity
of the data and not really caring about knowing whether it's actually hit the
wire or not.

> >> >  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;
> >> > +
> >> > +/* 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);
> >> > +
> >> 
> >> I'm afraid I don't get this comment.
> >
> > Quite a lot of the structures I need to keep compatibility with are
> > lists of some mixture of different things with a terminator on the end
> > (there's not much consistency in the binary format about how you tell
> > what's next or what the terminator is).
> > So the typical use is:
> >
> >     visit_start_sequence_compat(...)
> >     while (visit_get_next_type( &flag ), flag != end_flag) {
> >        switch (flag) {
> >        case A:
> >           do whatever you need to do now that we've found an 'A' in the list
> >           (typically another visit_start_sequence_compat for that type)
> >        case B:
> >           do whatever you need to do now that we've found an 'B' in the list
> >    ....
> >        }
> >     }
> >     visit_end_sequence_compat(...)
> >
> > An example is a RAM migration section; it's a list of initialisation data,
> > normal pages, all-zero pages, XBZRLE pages
> 
> Doesn't match the existing visit_get_next_type().  I guess I need more
> context.  One way to get it is study your patches.

Probably a good one to look at is the load path change;
http://lists.gnu.org/archive/html/qemu-devel/2014-04/msg03583.html
in particular the changes to ram_load and qemu_loadvm_state  that both have
examples of that use.

> >> > +/* Don't Use! - lets us move forward until we can get rid of all
> >> > file uses */
> >> > +QEMUFile *visitor_get_qemufile(Visitor *v);
> >> 
> >> Likewise.
> >
> > I'm hoping this is a temporary shim - my hope eventually is that we
> > won't need to
> > use this and everywhere that uses Visitors won't also need to get a QEMUFile
> > except internally to the visitor.
> 
> Suggest to put that in the comment.

Will do.

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

Patch

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;
+
+/* 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;
+}
+