[Qemu-devel] Re: [PATCH 4/5] New VMstate save/load infrastructure

[Juan Quintela <quintela@redhat.com>]

Reply-to: quintela@redhat.com
Gerd Hoffmann <kraxel@redhat.com> wrote:
>> +enum VMStateType {
>> +    VMSTATE_TYPE_UNSPEC = 0,
>> +    VMSTATE_TYPE_INT8,
>> +    VMSTATE_TYPE_INT16,
>> +    VMSTATE_TYPE_INT32,
>> +    VMSTATE_TYPE_INT64,
>> +    VMSTATE_TYPE_UINT8,
>> +    VMSTATE_TYPE_UINT16,
>> +    VMSTATE_TYPE_UINT32,
>> +    VMSTATE_TYPE_UINT64,
>> +    VMSTATE_TYPE_TIMER,
>> +};
>> +
>> +typedef struct VMStateInfo VMStateInfo;
>> +
>> +struct VMStateInfo {
>> +    const char *name;
>> +    size_t size;
>> +    enum VMStateType type;
>> +    void (*get)(QEMUFile *f, VMStateInfo *info, void *pv);
>> +    void (*put)(QEMUFile *f, VMStateInfo *info, const void *pv);
>> +};
>> +
>> +typedef struct {
>> +    const char *name;
>> +    size_t num;
>> +    size_t offset;
>> +    VMStateInfo *info;
>> +    int version_id;
>> +} VMStateField;
>
> Hmm, I'm not sure VMStateInfo is that useful here.  All the get/put
> callbacks are simple two-liners, and it is unlikely that the number of
> types ever grows.

Thinking about this.  VMStateType is not needed at all (see that it is
not used in the patch).  But Ilike the VMStateInfo struct approach.
This is the easier way of dealing with structs.  For instance
pci_device_save().  My idea was to just create a new VMStateInfo for it,
and then everything that needs to call pci_device_save() just add a
field like:

VMSTATE_FILED(dev, ThisPCIDevice, verion_id, pci_vmstate_info, PCIDevice)

and here, we go.  The reaso that I don't need VMStateType anymore is
that now load/save are done:

+static void vmstate_save(QEMUFile *f, VMStateDescription *vmsd, void *base)
+{
+    VMStateField *field = vmsd->fields;
+    int i;
+
+    while(field->name) {
+        for (i = 0; i < field->num; i++) {
+            field->info->put(f, field->info, base + field->offset +
+                             field->info->size * i);
+        }
+        field++;
+    }
+}

No need to have a type if we have the propper info struct.  As this
struct is going to grow (at least 2 new pointers for printing into
monitor/read value from monitor), I think it is a good idea to maintain
the sharing.

Later, Juan.

> I think I would stick "enum VMStateType" directly into VMStateField
> and kill one level of indirection.
>
>> +
>> +typedef struct {
>> +    const char *name;
>> +    int version_id;
>> +    int minimum_version_id;
>> +    VMStateField *fields;
>> +} VMStateDescription;
>
> Add compat_load() callback for version_id < minimum_version_id here?

I am not sure where/what to add.

I liked your approach of having the new version clean, and let the old
code to deal with the mess that used to be here.  Will take a look at
implementing soemething like that.

Thanks for the review, Juan.