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

[Dr. David Alan Gilbert]

* Markus Armbruster (address@hidden) wrote:
> "Dr. David Alan Gilbert" <address@hidden> writes:
> 
> > * Markus Armbruster (address@hidden) wrote:
> >> "Dr. David Alan Gilbert (git)" <address@hidden> writes:
> >> 
> >
> > Hi Markus,
> >   Thanks for the review.  I've reordered your comments to group some of the
> > replies together.
> >
> >> > From: "Dr. David Alan Gilbert" <address@hidden>
> >
> >> 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 <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"
> >> >  
> >> 
> >> >  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 / address@hidden / Manchester, UK