Re: [Qemu-devel] [PATCH v2 01/10] qapi: add Visitor interfaces for uint*_t and int*_t

[Anthony Liguori]

On 12/20/2011 05:12 AM, Paolo Bonzini wrote:
> > +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_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);
> > +void visit_type_uint8(Visitor *v, uint8_t *obj, const char *name, Error 
> > **errp);
> > +void visit_type_uint16(Visitor *v, uint16_t *obj, const char *name, Error
> > **errp);
> > +void visit_type_uint32(Visitor *v, uint32_t *obj, const char *name, Error
> > **errp);
> > +void visit_type_uint64(Visitor *v, uint64_t *obj, const char *name, Error
> > **errp);
> > +void visit_type_int8(Visitor *v, int8_t *obj, const char *name, Error **errp);
> > +void visit_type_int16(Visitor *v, int16_t *obj, const char *name, Error 
> > **errp);
> > +void visit_type_int32(Visitor *v, int32_t *obj, const char *name, Error 
> > **errp);
> > +void visit_type_int64(Visitor *v, int64_t *obj, const char *name, Error 
> > **errp);
>
> I think this approach is wrong. We're mashing the design of vmstate with that of
> visitors and getting something that is not a visitor and not vmstate.

Thanks for taking a look at this.

> Instead, I think you should have something like this:
>
> struct VMStateInfo {
> const char *name;
> // takes a QMPOutputVisitor and a QEMUFile open for reading
> int (*load)(QEMUFile *f, const char *name, Visitor *v,
> size_t size, Error **err);
>
> // takes a QMPInputVisitor and a QEMUFile open for writing
> void (*save)(QEMUFile *f, const char *name, Visitor *v,
> size_t size, Error **err);
>
> // takes a QMPOutputVisitor and reads from *pv
> int (*get)(Visitor *v, const char *name, void *pv,
> size_t size, Error **err);
>
> // takes a QMPInputVisitor and writes into *pv
> void (*set)(Visitor *v, const char *name, void *pv,
> size_t size, Error **err);
> };
>
> that splits the existing callbacks in two steps.
>
> For saving, you would adapt your visitor-based vmstate "put" routines so that
> they put things in a dictionary with no regard for integer types (a bit ugly for
> uint64, but perfectly fine for everything else).

I don't understand this.  The visitor interface should expose the C level 
primitives so that we can maintain fidelity when visiting something.  The fact 
that it only knows about "ints" today is a short cut.

> You take the dictionary from
> the output visitor and (with an input visitor) you feed it back to the "save"
> routines, which convert the dictionary to a QEMUFile. Both steps keep the types
> internal to vmstate.

That doesn't make effective use of visitors.  Visitors should preserve as much 
type information as possible.  I'm not really sure I understand the whole 
QEMUFile tie in either.  This series:

1) Makes a fully compatible QEMUFile input and output Visitor

2) Makes VMState no longer know about QEMUFile by using (1)

(2) is really the end goal.  If we have an interface that still uses QEMUFile, 
we're doing something wrong IMHO.

> For loading, it's the other way round: you interpret the vmstate with the
> QEMUFile reading routines, and create a dictionary. Then make an input visitor
> and use the vmstate "set" interpreter to fill in the struct fields.
>
> I'm sorry for noticing this just now, I was waiting for Anthony's QOM plans to
> be committed so that I could understand better how vmstate and QOM properties
> would interact. In fact, it would be great and not hard if the struct<->visitor
> step (get/set) was also exposed as a QOM property.

That's exactly why I'm so anxious to get this merged :-)

Regards,

Anthony Liguori

> Paolo