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

[Michael Roth <mdroth@linux.vnet.ibm.com>]

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

The main goal is to abstract away data serialization schemes 
(QObject->JSON, C->QEMUFile, etc). In the case of a JSON-based 
serialization, the visitor interface for fixed-with types would end up 
serializing everything as int64_t/double, but QEMUFile requires 
byte-length affinity to remain backward-compatible, so that information 
must be passed on to the Visitor interface when we call it.

And beyond QEMUFile, we'd like to eventually move to a serialization 
scheme that is self-describing in the types of the fields it stores so 
that we can do stricter checking in the deserialization/input visitor 
routines, or just plain be able to make sense of the serialized data 
without any outside information, since those schemes would eventually be 
used directly in implementing a new migration wire protocol and/or 
device state introspection.

>
> We already have such an adaptor: QOM static properties know about names,
> JSON types, C type and struct offset.
>
> 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.

And these are both requirements to implementing a robust, flexible 
serialization/Visitor interface with pluggable back-ends, but if those 
interface throw away the type/field names then the only way to get them 
back is to deserialize, which isn't useful for introspection, and 
volatile for migration (since type errors can be silently missed in a 
lot of cases)

>
> As always, you can implement that in many ways. However, I think the
> point of using Visitors is not to remove QEMUFile. It is to provide a
> backend-independent representation that backends can transform and that
> secondarily can be exposed by QOM.

Agreed, it's just a matter of wanting to maintain that information from 
start to finish.

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

Yup, that's the plan. These patches are a bit lazy in that regard. I 
agree that if we get into the habit of adding interfaces for a specific 
back-end without mapping these to the base-class implementations in 
other backends things will get out of hand quickly. Fortunately we 
haven't yet hit a situation where one backend ends up adding an 
interface that other backends cant' handle in some form.

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

You mean, for instance, put_int32()/get_int32_equal()? If so, I'm not 
sure I follow. In that case we use a Visitor purely to 
serialize/deserialize an int32, vmstate adds the *_equal() interface as 
helper function on top of that, but it's not part of the Visitor interfaces.

>
>>> 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.
>
> (2) Serialize this with QEMUFile, both for backwards-compatibility and
> to ensure that the whole thing works.
>
> 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'll fix up the existing non-QEMUFile Visitor backends with base-class 
implementations for all the new interfaces. Beyond that, is there 
anything else missing to achieve 1)?

>
> Paolo
>