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

[Markus Armbruster]

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

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

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