[Qemu-devel] vmstate within vmstate (VMS_STRUCT) versioning is broken ?

[Hans de Goede <hdegoede@redhat.com>]

Hi All,

While working on a bugfix for USB migration I noticed that the versioning
of VMS_STRUCT does not work as I would expect.

Currently when loading VMS_STRUCT state, the vmsd version gets passed:
vmstate_load_state(f, field->vmsd, addr, field->vmsd->version_id);

This means that any later added fields always get loaded even if
there not present. Or they would if the field->version_id was not bumped
in sync with the vmsd describing the struct.

If the field version_id was properly bumped, like for example is done
with vmstate_ide_drive, see the VMSTATE_IDE_DRIVES macro in hw/ide/internal.h,
then the following check in vmstate_load_state fails:
          (!field->field_exists &&
              field->version_id <= version_id)) {

And the struct will not be loaded at all, rather then only the new fields
in the struct not being loaded!

An obvious fix for this would be to:
1) Not do the version check for structs, iow not do:
          (!field->field_exists &&
              field->version_id <= version_id)) {
    For structs
2) Change the loading of the struct from:
         vmstate_load_state(f, field->vmsd, addr, field->vmsd->version_id);
    to:
         vmstate_load_state(f, field->vmsd, addr, field->version_id);

But I'm afraid that if we do that now, while having the old code for
a long time, we may hit some unpleasant surprises.

Note that the current behavior means atleast for the ide driver vmstate, that
instead of not loading the new cdrom_changed attribute when loading an old
vmstate, the entire drive state is not loaded !! Which is clearly not what
the ide code expects, and my suggested changes would make the code behave
as expected, and would make things work how I myself expected them to work
while working on the USB migration.

I guess the best thing todo would be to audit for all uses of VMS_STRUCT,
and see if that making the changes I suggest is feasible. But before doing
such an audit I would first like some input from others on this.

Regards,

Hans