Re: [Qemu-devel] [PATCH v3 3/4] savevm: define new unambiguous migration format

[Anthony Liguori]

On 08/12/2011 10:11 AM, Anthony Liguori wrote:
> On 08/12/2011 04:59 AM, Paolo Bonzini wrote:
> > With the current migration format, VMS_STRUCTs with subsections
> > are ambiguous. The protocol cannot tell whether a 0x5 byte after
> > the VMS_STRUCT is a subsection or part of the parent data stream.
> > In the past QEMU assumed it was always a part of a subsection; after
> > commit eb60260 (savevm: fix corruption in vmstate_subsection_load().,
> > 2011-02-03) the choice depends on whether the VMS_STRUCT has subsections
> > defined.
> >
> > Unfortunately, this means that if a destination has no subsections
> > defined for the struct, it will happily read subsection data into
> > its own fields. And if you are "lucky" enough to stumble on a
> > zero byte at the right time, it will be interpreted as QEMU_VM_EOF
> > and migration will be interrupted.
> >
> > There is no way out of this except defining an incompatible
> > migration protocol with a sentinel at the end of embedded structs.
> > Of course, this is restricted to new machine models.
> >
> > Signed-off-by: Paolo Bonzini<address@hidden>
>
> Because of the change we made for 0.15, this is no longer strictly
> needed. It only matters if we add a subsection to a structure, right?

I'm definitely not saying we shouldn't do this BTW.  But trying to 
understand whether we can position this as a strictly optional feature 
(iow, a capability) or whether this requires a hard break in the 
migration protocol.

Regards,

Anthony Liguori

> Regards,
>
> Anthony Liguori
>
> > ---
> > hw/boards.h | 3 +++
> > hw/pc_piix.c | 5 +++++
> > savevm.c | 24 ++++++++++++++++--------
> > 3 files changed, 24 insertions(+), 8 deletions(-)
> >
> > diff --git a/hw/boards.h b/hw/boards.h
> > index 560dbaf..f20d58e 100644
> > --- a/hw/boards.h
> > +++ b/hw/boards.h
> > @@ -5,6 +5,9 @@
> >
> > #include "qdev.h"
> >
> > +#define QEMU_VM_FILE_VERSION_0_15 0x00000003
> > +#define QEMU_VM_FILE_VERSION 0x00000004
> > +
> > typedef void QEMUMachineInitFunc(ram_addr_t ram_size,
> > const char *boot_device,
> > const char *kernel_filename,
> > diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> > index 88a5f3b..8af1f6f 100644
> > --- a/hw/pc_piix.c
> > +++ b/hw/pc_piix.c
> > @@ -284,6 +284,7 @@ static QEMUMachine pc_machine_v0_15 = {
> > .desc = "Standard PC",
> > .init = pc_init_pci,
> > .max_cpus = 255,
> > + .migration_format = QEMU_VM_FILE_VERSION_0_15,
> > };
> >
> > static QEMUMachine pc_machine_v0_13 = {
> > @@ -291,6 +292,7 @@ static QEMUMachine pc_machine_v0_13 = {
> > .desc = "Standard PC",
> > .init = pc_init_pci_no_kvmclock,
> > .max_cpus = 255,
> > + .migration_format = QEMU_VM_FILE_VERSION_0_15,
> > .compat_props = (GlobalProperty[]) {
> > {
> > .driver = "virtio-9p-pci",
> > @@ -330,6 +332,7 @@ static QEMUMachine pc_machine_v0_12 = {
> > .desc = "Standard PC",
> > .init = pc_init_pci_no_kvmclock,
> > .max_cpus = 255,
> > + .migration_format = QEMU_VM_FILE_VERSION_0_15,
> > .compat_props = (GlobalProperty[]) {
> > {
> > .driver = "virtio-serial-pci",
> > @@ -373,6 +376,7 @@ static QEMUMachine pc_machine_v0_11 = {
> > .desc = "Standard PC, qemu 0.11",
> > .init = pc_init_pci_no_kvmclock,
> > .max_cpus = 255,
> > + .migration_format = QEMU_VM_FILE_VERSION_0_15,
> > .compat_props = (GlobalProperty[]) {
> > {
> > .driver = "virtio-blk-pci",
> > @@ -424,6 +428,7 @@ static QEMUMachine pc_machine_v0_10 = {
> > .desc = "Standard PC, qemu 0.10",
> > .init = pc_init_pci_no_kvmclock,
> > .max_cpus = 255,
> > + .migration_format = QEMU_VM_FILE_VERSION_0_15,
> > .compat_props = (GlobalProperty[]) {
> > {
> > .driver = "virtio-blk-pci",
> > diff --git a/savevm.c b/savevm.c
> > index 87f2b71..a362ad7 100644
> > --- a/savevm.c
> > +++ b/savevm.c
> > @@ -158,6 +158,14 @@ void qemu_announce_self(void)
> >
> > #define IO_BUF_SIZE 32768
> >
> > +#define QEMU_VM_EOF 0x00
> > +#define QEMU_VM_SECTION_START 0x01
> > +#define QEMU_VM_SECTION_PART 0x02
> > +#define QEMU_VM_SECTION_END 0x03
> > +#define QEMU_VM_SECTION_FULL 0x04
> > +#define QEMU_VM_SUBSECTION 0x05
> > +#define QEMU_VM_SUBSECTIONS_END 0x06
> > +
> > struct QEMUFile {
> > QEMUFilePutBufferFunc *put_buffer;
> > QEMUFileGetBufferFunc *get_buffer;
> > @@ -1358,6 +1366,11 @@ int vmstate_load_state(QEMUFile *f, const
> > VMStateDescription *vmsd,
> > }
> > if (field->flags& VMS_STRUCT) {
> > ret = vmstate_load_state(f, field->vmsd, addr, field->vmsd->version_id);
> > + if (qemu_current_migration_format()>= 4) {
> > + if (qemu_get_byte(f) != QEMU_VM_SUBSECTIONS_END) {
> > + return -EINVAL;
> > + }
> > + }
> > } else {
> > ret = field->info->get(f, addr, size);
> >
> > @@ -1429,6 +1442,9 @@ void vmstate_save_state(QEMUFile *f, const
> > VMStateDescription *vmsd,
> > }
> > if (field->flags& VMS_STRUCT) {
> > vmstate_save_state(f, field->vmsd, addr);
> > + if (qemu_current_migration_format()>= 4) {
> > + qemu_put_byte(f, QEMU_VM_SUBSECTIONS_END);
> > + }
> > } else {
> > field->info->put(f, addr, size);
> > }
> > @@ -1458,14 +1474,6 @@ static void vmstate_save(QEMUFile *f,
> > SaveStateEntry *se)
> >
> > #define QEMU_VM_FILE_MAGIC 0x5145564d
> > #define QEMU_VM_FILE_VERSION_COMPAT 0x00000002
> > -#define QEMU_VM_FILE_VERSION 0x00000003
> > -
> > -#define QEMU_VM_EOF 0x00
> > -#define QEMU_VM_SECTION_START 0x01
> > -#define QEMU_VM_SECTION_PART 0x02
> > -#define QEMU_VM_SECTION_END 0x03
> > -#define QEMU_VM_SECTION_FULL 0x04
> > -#define QEMU_VM_SUBSECTION 0x05
> >
> > bool qemu_savevm_state_blocked(Monitor *mon)
> > {