Patchwork [v3,3/4] savevm: define new unambiguous migration format

login
register
mail settings
Submitter Paolo Bonzini
Date Aug. 12, 2011, 9:59 a.m.
Message ID <1313143181-7921-4-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/109811/
State New
Headers show

Comments

Paolo Bonzini - Aug. 12, 2011, 9:59 a.m.
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 <pbonzini@redhat.com>
---
 hw/boards.h  |    3 +++
 hw/pc_piix.c |    5 +++++
 savevm.c     |   24 ++++++++++++++++--------
 3 files changed, 24 insertions(+), 8 deletions(-)
Anthony Liguori - Aug. 12, 2011, 3:11 p.m.
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<pbonzini@redhat.com>

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?

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)
>   {
Anthony Liguori - Aug. 12, 2011, 3:16 p.m.
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<pbonzini@redhat.com>
>
> 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)
>> {
>
>
Paolo Bonzini - Aug. 12, 2011, 3:46 p.m.
On 08/12/2011 05:16 PM, Anthony Liguori wrote:
>>
>> 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?

Yes.  We can also check the current migration format in the "needed" 
function for such subsections, if necessary.  This is what I plan to do 
for the floppy subsection, skipping it for old machine types that do not 
use the new migration format.

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

This what? :)  Adding a subsection to a structure, or using the new 
migration format?  If you mean using the new format only when you have 
subsections in a structure, then you'll need quite a bit of new code to 
try all VMState handlers and see if they'll need a subsection.  I don't 
think it's worth it, also because we all hope this is a temporary 
solution until we have a serious format (e.g. ASN-1).

Paolo

Patch

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