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

[Anthony Liguori <anthony@codemonkey.ws>]

On 12/20/2011 08:30 AM, Paolo Bonzini wrote:
> On 12/20/2011 02:50 PM, Anthony Liguori wrote:
>>> 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.
>
> Why does this need to be in Visitor? You can always embed C knowledge in an
> adaptor or decorator. Visitors only need to know about names and JSON types
> (well, they also distinguish int from double).

You are tying Visitors too closely to JSON.  We should be able to write a 
Visitor that can output a serialization format that has more interesting integer 
types and maintains better fidelity with standard C types.

> We already have such an adaptor: QOM static properties know about names, JSON
> types, C type and struct offset.

Yes...  But I don't see the relevance here.

>
> VMState fields know about all that plus QEMUFile encoding. QEMUFile encoding can
> be hidden in the decorator, it does not need to become visible to the concrete
> visitors.

This is mixing up too many concepts.

A visit function -> knows only how to walk a C data structure.  It's just 
saying, I have an int, it's name is X, i have a double, it's name is Y.

The Visitor is the thing that plugs into the visit function and decides what to 
do with this information.

Having a "QEMUFile" decorator just doesn't fit the model.  I'm not even sure 
what it means.

> As always, you can implement that in many ways. However, I think the point of
> using Visitors is not to remove QEMUFile.

Yes, it is.

> It is to provide a backend-independent
> representation that backends can transform and that secondarily can be exposed
> by QOM.

The point of Visitors is to make up for the fact that C lacks introspection. 
It's meant to be a standard way to introspect a C data structure (or type).

>
> This is only half-true in Michael's code, because he relies on primitives that
> QMPInputVisitor and QMPOutputVisitor do not implement. Fixing this is quite
> easy, you only need to add a base-class implementation of the int8/int16/...
> primitives.
>
> On top of this the representation he passes to visitors is somewhat redundant.
> For example, VMState has "equal" fields; they are fields that are serialized but
> are really fixed at compile- or realize-time. Such fields should not be part of
> the backend-independent representation. With Michael's approach they are, and
> that's quite deep in the implementation.

Yes, but there's no way to do this today without breaking the format.  There's 
just too much magic in VMState right now.  We need something like a migration 
filter capability where we can encapsulate this kind of logic such that we can 
ween VMState away from these things (and ultimately switch to an IDL compiler).

We can't do a migration filter until we have something like Michael's series.

>
>>> 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.
>
> Yes, this is accurate, but I see the goals differently. We should:
>
> (1) First and foremost, provide a backend-independent representation of device
> state so that we can add other backends later.

And Mike's series does this, no?

> (2) Serialize this with QEMUFile, both for backwards-compatibility and to ensure
> that the whole thing works.

Mike's series also does this, no?

> Whether you do (2) directly with QEMUFile or, like Michael does, with
> QEMUFile*Visitors is secondary. I don't have big objections to either approach.
> However, the series is missing (1).

I don't see how.

Regards,

Anthony Liguori

>
> Paolo
>