Re: [Qemu-devel] The State of the SaveVM format

[Markus Armbruster]

Anthony Liguori <address@hidden> writes:

> Hi Juan,
>
> Juan Quintela wrote:
>> The problems is what to do from here:
>> - We can have a very simple VMState format that only allows storing
>>   simple types (int32_t, uint64_t, timers, buffers of uint8_t, ...)
>>   Arrays of valid types
>>   Structs of valid types
>>   And that is it.  Advantage of this approach, it is very simple to
>>   create/test/whatever.  Disadvantage: it can't express all the things
>>   that were done in plain C.  Everybody agrees that we don't want to
>>   support everything that was done in plain C in the old way.  What we
>>   are discussing is "how many" things do we want to support.  Notice
>>   that  we can support _everything_ that we were doing with plain C.
>>   Anytime that you want to do something strange, you just need to write
>>   your own marshaling functions and you are done.  You do there
>>   anything that you want.
>>
>>   We are here at how we want to develop the format.  People that has
>>   expressed opinions so far are:
>>   - Gerd: You do a very simple format, and if the old state can't be
>>           expressed in simple VMState, you just use the old load
>>           function.  This maintains VMState clean, and you can load
>>           everything that was done before. Eventually, we remove the
>>           old load state functions when we don't support so old format.
>>   - Anthony: If we leave the old load state functions, they will be
>>           around forever.  He wants to complicate^Wimprove VMState
>>           to be able to express everything that was done in plain C.
>>           Reason: It is better to only have _one_ set of functions.
>>   
>
> This is not quite an accurate representation.
>
> Today, you make no attempt to support older versions even if their
> format is quite sane.  Take ps2_kbd as an example.
>
> The new format (v3) is:
>
>        VMSTATE_STRUCT(common, PS2KbdState, 0, vmstate_ps2_common,
> PS2State),
>        VMSTATE_INT32(scan_enabled, PS2KbdState),
>        VMSTATE_INT32(translate, PS2KbdState),
>        VMSTATE_INT32_V(scancode_set, PS2KbdState,3),
>
> This is nice and should support v2 and v3.  However, you still point
> to the old savevm function which is:
>
>
> static int ps2_kbd_load_old(QEMUFile* f, void* opaque, int version_id)
> {
>    PS2KbdState *s = (PS2KbdState*)opaque;
>
>    if (version_id != 2 && version_id != 3)
>        return -EINVAL;
>
>    vmstate_load_state(f, &vmstate_ps2_common, &s->common, version_id);
>    s->scan_enabled=qemu_get_be32(f);
>    s->translate=qemu_get_be32(f);
>    if (version_id == 3)
>        s->scancode_set=qemu_get_be32(f);
>    else
>        s->scancode_set=2;
>    return 0;
> }
>
> Which has to be an error.  But this is the real problem with leaving
> the old functions.  It encourages sloppiness.
>
> Let's look at a more complex example (piix_pci):
>
>        VMSTATE_PCI_DEVICE(dev, PCII440FXState),
>        VMSTATE_UINT8(smm_enabled, PCII440FXState),
>
> This is v3.  You have an old load function that duplicates this
> functionality but has an additional field:
>
>    if (version_id == 2)
>        for (i = 0; i < 4; i++)
>            d->irq_state->piix3->pci_irq_levels[i] = qemu_get_be32(f);
>
> All I'm suggesting, is that instead of leaving that old function, you
> introduce:
>
> static void marshal_pci_irq_levels(void *opaque, const char *name,
> size_t offset, int load, int version)
> {
>    if (version == 2) {
>        for (i = 0; i < 4; i++)
>            d->irq_state->piix3->pci_irq_levels[i] = qemu_get_be32(f);
>   }
> }
>
>        VMSTATE_PCI_DEVICE(dev, PCII440FXState),
>        VMSTATE_UINT8(smm_enabled, PCII440FXState),
>        VMSTATE_FUNC_V(irq_state->piix3->pci_irq_levels,
> PCII440FXState, marshal_pci_irq_levels, 2)
>
> This avoids bit rot of the old load functions and makes the whole
> thing more robust.  My contention is that there simply isn't a lot of
> these special cases so it's not a lot more work overall.
[...]

Bitrot is only an issue if we keep supporting the old formats for much
longer.

I'd like to challenge the idea that we need to drag along the old
formats for an indefinite period of time.  Is the restriction that to
upgrade from version N to version M, you have to go through version N+1
really so onerous?

A couple of releases down the road, the pre-vmstate formats will be
ancient history.  If we complicate vmstate now to shoehorn pre-vmstate
formats into vmstate, that ancient history will continue to haunt us.
Complicating a program is far easier than the other direction.

Moreover, I'm rather wary of efforts to replace working code (the
existing load functions) by new code that is supposed to do precisely
the same.  It means replacing the devil I know by some untested devil I
don't know.