[v2,01/10] qapi: add Visitor interfaces for uint*_t and int*_t

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qapi/qapi-visit-core.c |   78 ++++++++++++++++++++++++++++++++++++++++++++++++
 qapi/qapi-visit-core.h |   30 ++++++++++++++++++
 2 files changed, 108 insertions(+), 0 deletions(-)

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

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

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.

Paolo

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
>

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

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.

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.

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.

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

Paolo

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
>

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
>

On 12/20/2011 09:22 PM, Michael Roth wrote:
> The main goal is to abstract away data serialization schemes
> (QObject->JSON, C->QEMUFile, etc).

Right.  And the simplest way to abstract the scheme is to provide a 
backend-independent representation of device state.

As a convention, I'll call:

- "device state" the fields in the struct

- "QEMUFile" the serialized output that VMState save produces

- "device state representation" a representation of device state that is 
independent of the actual serialization backend.

- "device configuration" is the parameters that are used to create the 
device (coming from compile-time constants or the command-line)

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

When creating the device state representation from either device state 
or QEMUFile, you need the byte length to fetch fields correctly.

When recreating device state from its representation, or saving to 
QEMUFile state, you need the byte length to store fields correctly.

However, you do not need the byte length in the device state 
representation.  In all four cases (from/to device state, from/to 
QEMUFile) the byte length can be fetched from the VMState.

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

I don't see the point of maintaining the type hints from start to 
finish, as long as you can reconstruct it any time you want it.

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

It should be only in QEMUFile, because it's one of its quirks.  It is a 
separate property that is fixed at compile-time or realize-time, so it's 
part of device configuration; it's not part of device state representation.

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

I think almost nothing (you need to integrate into the QOM properties, 
and to handle a few special cases such as VMSTATE_EQUAL and 
VMSTATE_UNUSED).  That's quite good.

However, once you do this, you will still be serializing QEMUFile 
directly from device state rather than from device state representation. 
  This is fine, but not what we should do when working on BER 
serialization or similar.

Paolo

On 12/20/2011 09:56 PM, Anthony Liguori wrote:
>> 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.

The point of using Visitors is to provide a standard representation of 
device state.  QEMUFile is but one consumer of that representation, 
along with any other migration filter.

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

Agreed.  I'm saying that the QEMUFile serialization should be the first 
example of a migration filter.  Mike's patch add a layer of indirection 
in the code, but not in the data.  We can't do a migration filter until 
we have device state represented as introspectable data.

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

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?

It serializes device state directly, without going through a 
backend-independent representation.  It doesn't provide a place in which 
you can plug a filter.

Paolo

On 12/21/2011 06:35 AM, Paolo Bonzini wrote:
> On 12/20/2011 09:56 PM, Anthony Liguori wrote:
>>> 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.
>
> The point of using Visitors is to provide a standard representation of device
> state. QEMUFile is but one consumer of that representation, along with any other
> migration filter.

Can you do a quick code mock up of how you'd expect this to work?  I'm not sure 
if I'm just being dense, but I'm having a really hard time understanding what 
you're suggesting.  How I see this evolving with Mike's series is:

struct DeviceStateClass {
    ObjectClass parent;

    void (*load_state)(DeviceState *obj, Visitor *v, const char *name,
                       Error **errp);
    void (*save_state)(DeviceState *obj, Visitor *v, const char *name,
                       Error **errp);
};

The PCIDevice base class would expose:

/* protected */
void pci_load_state(PCIDevice *obj, Visitor *v, const char *name,
                     Error **errp)
{
     visit_start_struct(v, "PCIDevice", name, errp);
     visit_type_int8(v, &obj->reg[0], "reg[0]", errp);
     ..
     visit_end_struct(v, errp);
}

Or:

static VMStateDescription pci_device_desc = {
   .name = "PCIDevice",
   .fields = {
      VMSTATE_UINT8(PCIDevice, reg[0]),
      ...
   }
};

void pci_load_state(PCIDevice *obj, Visitor *v, const char *name,
                     Error **errp)
{
     visit_with_vmstate(v, obj, &pci_device_desc, name, errp);
}

A subclass would do:

static void my_device_load_state(DeviceState *obj, Visitor *v, const char *name,
                                  Error **errp)
{
     MyDevice *s = MY_DEVICE(obj);
     visit_start_struct(v, "MyDevice", name, errp);
     pci_load_state(PCI_DEVICE(obj), v, "super", errp);
     visit_end_struct(v, errp);
}

static void my_device_class_init(ObjectClass *klass, void *data)
{
    DeviceClass *dc = DEVICE_CLASS(klass);

    dc->load_state = my_device_load_state;
    ...
}

There's no reference at all to QEMUFile.  The load_state/save_state methods can 
be exposed as a "state" property too.

Once the series 2/4 lands and Mike's series, implementing this should be very 
straight forward.

Regards,

Anthony Liguori

On 12/21/2011 03:45 PM, Anthony Liguori wrote:
> On 12/21/2011 06:35 AM, Paolo Bonzini wrote:
>> On 12/20/2011 09:56 PM, Anthony Liguori wrote:
>>>> 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.
>>
>> The point of using Visitors is to provide a standard representation of
>> device
>> state. QEMUFile is but one consumer of that representation, along with
>> any other
>> migration filter.
>
> Can you do a quick code mock up of how you'd expect this to work?

void
vmstate_get(Device *obj, Visitor *v, void *opaque, const char *name)
{
     /* Use the VMStateDescription in opaque to add fields to Visitor
        from the struct.  */
}

void
vmstate_set(Device *obj, Visitor *v, void *opaque, const char *name)
{
     /* Use the VMStateDescription in opaque to retrieve fields from
        Visitor and store them in the struct.  */
}

void
vmstate_load_state(Device *obj, Visitor *v, QEMUFile )
{
     VMStateDescription *vmsd = ???; /* something from the DeviceClass */

     /* Use the VMStateDescription in opaque to retrieve fields from
        Visitor and store them in the struct.  This is basically the
        VMState interpreter currently in savevm.c, only fetching fields
        from v rather than the file.  */
}

void
vmstate_save_state(Device *obj, Visitor *v, QEMUFile *out)
{
     VMStateDescription *vmsd = ???; /* something from the DeviceClass */

     /* Use the VMStateDescription in opaque to fetch fields from
        Visitor and store them in the file.  This is basically the
        other VMState interpreter currently in savevm.c, but fetching
        fields from v rather than the struct.  */
}

void
qdev_add_vmstate(Device *obj, VMStateDescription *vmsd)
{
     qdev_add_property(obj, "vmstate", vmstate_get, vmstate_set, vmsd);
     qdev_add_interface(obj, "QEMUFileSerializable", vmstate_load_state,
                        vmstate_save_state);
}


savevm.c:

Visitor *v = qmp_output_visitor_new();
qdev_property_get(obj, "vmstate", v);
QObject *qobj = qmp_visitor_get_obj(v);
qmp_visitor_free(v);

Visitor *v = qmp_input_visitor_new(qobj);
QEMUFileSerializable *s = QEMU_FILE_SERIALIZABLE(obj);
s->save_state(obj, v, outfile);
qmp_visitor_free(v);

...

Visitor *v = qmp_output_visitor_new();
QEMUFileSerializable *s = QEMU_FILE_SERIALIZABLE(obj);
s->load_state(obj, v, outfile);
QObject *qobj = qmp_visitor_get_obj(v);
qmp_visitor_free(v);

Visitor *v = qmp_input_visitor_new(qobj);
QEMUFileSerializable *s = QEMU_FILE_SERIALIZABLE(obj);
qdev_property_set(obj, "vmstate", v);
qmp_visitor_free(v);

---------------------

Yes, this makes QEMUFile serialization special.  But that's because it's 
legacy, and it needs to do strange things and store things beyond the 
contents of the vmstate.

Take for example "unused" fields.  In Mike's implementation, if you try 
to pass a QMPOutputVisitor to save_state you might get a "unused" entry 
that is an array of zeros.  I have no idea what happens if a single 
VMStateDescription has more than one VMSTATE_UNUSED field. 
QEMUFileOutputVisitor completely ignores the field names, so it probably 
works but would break with QMPOutputVisitor.

Other serialization backends should not need any hooks in the devices 
beyond save_state and load_state.

Paolo

On 12/21/2011 09:39 AM, Paolo Bonzini wrote:
> On 12/21/2011 03:45 PM, Anthony Liguori wrote:
>> On 12/21/2011 06:35 AM, Paolo Bonzini wrote:
>>> On 12/20/2011 09:56 PM, Anthony Liguori wrote:
>>>>> 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.
>>>
>>> The point of using Visitors is to provide a standard representation of
>>> device
>>> state. QEMUFile is but one consumer of that representation, along with
>>> any other
>>> migration filter.
>>
>> Can you do a quick code mock up of how you'd expect this to work?
>
> void
> vmstate_get(Device *obj, Visitor *v, void *opaque, const char *name)
> {
> /* Use the VMStateDescription in opaque to add fields to Visitor
> from the struct. */
> }
>
> void
> vmstate_set(Device *obj, Visitor *v, void *opaque, const char *name)
> {
> /* Use the VMStateDescription in opaque to retrieve fields from
> Visitor and store them in the struct. */
> }
>
> void
> vmstate_load_state(Device *obj, Visitor *v, QEMUFile )
> {
> VMStateDescription *vmsd = ???; /* something from the DeviceClass */
>
> /* Use the VMStateDescription in opaque to retrieve fields from
> Visitor and store them in the struct. This is basically the
> VMState interpreter currently in savevm.c, only fetching fields
> from v rather than the file. */
> }
>
> void
> vmstate_save_state(Device *obj, Visitor *v, QEMUFile *out)
> {
> VMStateDescription *vmsd = ???; /* something from the DeviceClass */
>
> /* Use the VMStateDescription in opaque to fetch fields from
> Visitor and store them in the file. This is basically the
> other VMState interpreter currently in savevm.c, but fetching
> fields from v rather than the struct. */
> }
>
> void
> qdev_add_vmstate(Device *obj, VMStateDescription *vmsd)
> {
> qdev_add_property(obj, "vmstate", vmstate_get, vmstate_set, vmsd);
> qdev_add_interface(obj, "QEMUFileSerializable", vmstate_load_state,
> vmstate_save_state);

Ok, I get what you're suggesting.  You would like to continue to keep 
VMStateDescription describing both the QEMUFile format and the fields.

I'm suggesting something fundamentally different.  What I see us doing is 
breaking VMStateDescription apart into two different things.  One would be a 
pure description of current fields.  The other one would be the description of 
the old protocol.

The later would be fed to a migration filter and the former would live off of 
DeviceState.

By doing Mike's series, we can do this incrementally by splitting the 
description up, and invoking the filter post_load/pre_save.

One of the reasons for this split up is because I would like to generate the 
first table by IDL and make the second table not dependent on structure members 
(so we don't end up with all the hacks we have with dummy struct fields).

Regards,

Anthony Liguori

> }
>
>
> savevm.c:
>
> Visitor *v = qmp_output_visitor_new();
> qdev_property_get(obj, "vmstate", v);
> QObject *qobj = qmp_visitor_get_obj(v);
> qmp_visitor_free(v);
>
> Visitor *v = qmp_input_visitor_new(qobj);
> QEMUFileSerializable *s = QEMU_FILE_SERIALIZABLE(obj);
> s->save_state(obj, v, outfile);
> qmp_visitor_free(v);
>
> ...
>
> Visitor *v = qmp_output_visitor_new();
> QEMUFileSerializable *s = QEMU_FILE_SERIALIZABLE(obj);
> s->load_state(obj, v, outfile);
> QObject *qobj = qmp_visitor_get_obj(v);
> qmp_visitor_free(v);
>
> Visitor *v = qmp_input_visitor_new(qobj);
> QEMUFileSerializable *s = QEMU_FILE_SERIALIZABLE(obj);
> qdev_property_set(obj, "vmstate", v);
> qmp_visitor_free(v);
>
> ---------------------
>
> Yes, this makes QEMUFile serialization special. But that's because it's legacy,
> and it needs to do strange things and store things beyond the contents of the
> vmstate.
>
> Take for example "unused" fields. In Mike's implementation, if you try to pass a
> QMPOutputVisitor to save_state you might get a "unused" entry that is an array
> of zeros. I have no idea what happens if a single VMStateDescription has more
> than one VMSTATE_UNUSED field. QEMUFileOutputVisitor completely ignores the
> field names, so it probably works but would break with QMPOutputVisitor.
>
> Other serialization backends should not need any hooks in the devices beyond
> save_state and load_state.
>
> Paolo
>

On 12/21/2011 05:24 PM, Anthony Liguori wrote:
> Ok, I get what you're suggesting.  You would like to continue to keep
> VMStateDescription describing both the QEMUFile format and the fields.

I don't "like" to do that, but yes, that's what I'm suggesting. :)

You envision having the front-end (state->introspectable state 
representation) and back-end (state representation->serialization) 
completely decoupled in the future, with migration filters in the middle 
if necessary.

I actually agree this is as the final direction.  For this reason, the 
first step should be to decouple the front-end and backend and actually 
have this introspectable state representation.  Then you can also break 
VMStateDescription apart into a front-end and backend part.

Mike's patches change the way you write to QEMUFile, so the new code 
_does_ go in the right direction.  However, they fail to provide an 
introspectable, backend-independent state representation.  Let's put it 
in a few diagrams:

where we are now:        struct ---> QEMUFile
                                  ^
                                  |
                             VMStateDescription


what Mike's patches do:  struct ---> QEMUFileVisitor ---> QEMUFile
                                  ^
                                  |
                             VMStateDescription


what I proposed:         struct ---> QMPOutputVisitor ---> QObject
                                  ^
                                  |
                             VMStateDescription

                          QObject ---> QEMUFile
                                   ^
                                   |
                            VMStateDescription

where you want to go:    struct ---> QMPOutputVisitor ---> QObject
                                  ^
                                  |
                             DeviceStateDescription

                         QObject ---> QEMUFileOutputVisitor ---> QEMUFile
                                  ^
                                  |
                           VMStateDescription

As Mike's patches stand, I'm worried that they would make further steps 
harder rather than easier.  This is because I'm not convinced that the 
QEMUFileOutputVisitor is actually useful.

However, the new code from Mike's patches is very close to the 
backend-independent representation from the VMStateDescription.  So, 
Mike's patches could be morphed into this pretty easily:

                         struct ---> QMPOutputVisitor ---> QObject
                                 ^
                                 |
                          VMStateDescription

                         struct ---> QEMUFile         \
                                 ^                     \ that's savevm.c,
                                 |                     / unchanged
                            VMStateDescription        /

This is an intermediate state that lets us do the next steps:

- serialize to QEMUFile from a QObject;

- reintroduce Mike's QEMUFileOutputVisitor [I don't really see the 
benefit of this];

- split the DeviceStateDescription and the VMStateDescription;

None of these three steps are a prerequisite for introducing a new 
migration format.

> One of the reasons for this split up is because I would like to generate
> the first table by IDL and make the second table not dependent on
> structure members (so we don't end up with all the hacks we have with
> dummy struct fields).

That would be a few years away.  Let's reason in incremental steps.

Paolo