[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Michael Roth
Subject: Re: [Qemu-devel] [PATCH v2 01/10] qapi: add Visitor interfaces for uint*_t and int*_t
Date: Tue, 20 Dec 2011 14:22:28 -0600
User-agent: Mozilla/5.0 (X11; Linux i686 on x86_64; rv:8.0) Gecko/20111105 Thunderbird/8.0

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

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

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

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


reply via email to

[Prev in Thread] Current Thread [Next in Thread]