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

[Markus Armbruster]

"Dr. David Alan Gilbert (git)" <address@hidden> writes:

> From: "Dr. David Alan Gilbert" <address@hidden>
>
> 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 <address@hidden>
> ---
>  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;
> +}
> +