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

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* 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