Add section footers to detect corrupted migration streams
cover Add section footers to detect corrupted migration streams
From: "Dr. David Alan Gilbert" <[email protected]>
Badly formatted migration streams can go undetected or produce misleading errors due to a lock of checking at the end of sections. In particular a section that adds an extra 0x00 at the end causes what looks like a normal end of stream and thus doesn't produce any errors, and something that ends in a 0x01..0x04 kind of look like real section headers and then fail when the section parser tries to figure out which section they are. This is made worse by the choice of 0x00..0x04 being small numbers that are particularly common in normal section data.
This patch series adds a section footer consisting of a marker (0x7e - ~) followed by the section-id that was also sent in the header. If they mismatch then it throws an error explaining which section was being loaded.
The footers are tied to new machine types (on both pc types).
Dr. David Alan Gilbert (4):
Merge section header writing
Disable section footers on older machine types Add a protective section footer
Teach analyze-migration.py about section footers
hw/i386/pc_piix.c | 2 + hw/i386/pc_q35.c | 2 + include/migration/migration.h | 2 + savevm.c | 141 ++++++++++++++++++++++++++++-------------- scripts/analyze-migration.py | 5 ++ 5 files changed, 107 insertions(+), 45 deletions(-)
Signature · 1 line
2.4.1
PATCH 1/4 Merge section header writing
From: "Dr. David Alan Gilbert" <[email protected]>
The header writing for device sections is open coded in a few places, merge it into one.
--- savevm.c | 73 +++++++++++++++++++++++++--------------------------------------- 1 file changed, 28 insertions(+), 45 deletions(-)
savevm.c +28 -45
@@ -602,6 +602,27 @@ static void vmstate_save(QEMUFile *f, SaveStateEntry *se, QJSON *vmdesc)vmstate_save_state (f, se->vmsd, se->opaque, vmdesc); } +/* +* Write the header for device section (QEMU_VM_SECTION START/END/PART/FULL) +*/ +static void save_section_header (QEMUFile *f,SaveStateEntry *se, +uint8_t section_type) +{ +qemu_put_byte (f, section_type); +qemu_put_be32 (f, se->section_id); + +if (section_type ==QEMU_VM_SECTION_FULL || + section_type ==QEMU_VM_SECTION_START ) { +/* ID string */ +size_t len =strlen (se->idstr); +qemu_put_byte (f, len); +qemu_put_buffer (f, (uint8_t *)se->idstr, len); + +qemu_put_be32 (f, se->instance_id); +qemu_put_be32 (f, se->version_id); + } +} +bool qemu_savevm_state_blocked (Error **errp) {SaveStateEntry *se;
@@ -634,8 +655,6 @@ void qemu_savevm_state_begin(QEMUFile *f,qemu_put_be32 (f,QEMU_VM_FILE_VERSION ); QTAILQ_FOREACH(se, &savevm_handlers, entry) { -int len; -if (!se->ops || !se->ops->save_live_setup) {continue ; }
@@ -644,17 +663,7 @@ void qemu_savevm_state_begin(QEMUFile *f,continue ; } } -/* Section type */ -qemu_put_byte (f,QEMU_VM_SECTION_START ); -qemu_put_be32 (f, se->section_id); - -/* ID string */ - len =strlen (se->idstr); -qemu_put_byte (f, len); -qemu_put_buffer (f, (uint8_t *)se->idstr, len); - -qemu_put_be32 (f, se->instance_id); -qemu_put_be32 (f, se->version_id); +save_section_header (f, se,QEMU_VM_SECTION_START ); ret = se->ops->save_live_setup (f, se->opaque);if (ret <0 ) {
@@ -689,9 +698,8 @@ int qemu_savevm_state_iterate(QEMUFile *f)return 0 ; }trace_savevm_section_start (se->idstr, se->section_id); -/* Section type */ -qemu_put_byte (f,QEMU_VM_SECTION_PART ); -qemu_put_be32 (f, se->section_id); + +save_section_header (f, se,QEMU_VM_SECTION_PART ); ret = se->ops->save_live_iterate (f, se->opaque);trace_savevm_section_end (se->idstr, se->section_id, ret);
@@ -737,9 +745,8 @@ void qemu_savevm_state_complete(QEMUFile *f) } }trace_savevm_section_start (se->idstr, se->section_id); -/* Section type */ -qemu_put_byte (f,QEMU_VM_SECTION_END ); -qemu_put_be32 (f, se->section_id); + +save_section_header (f, se,QEMU_VM_SECTION_END ); ret = se->ops->save_live_complete (f, se->opaque);trace_savevm_section_end (se->idstr, se->section_id, ret);
@@ -753,8 +760,6 @@ void qemu_savevm_state_complete(QEMUFile *f)json_prop_int (vmdesc,"page_size" ,TARGET_PAGE_SIZE );json_start_array (vmdesc,"devices" ); QTAILQ_FOREACH(se, &savevm_handlers, entry) { -int len; -if ((!se->ops || !se->ops->save_state) && !se->vmsd) {continue ; }
@@ -764,17 +769,7 @@ void qemu_savevm_state_complete(QEMUFile *f)json_prop_str (vmdesc,"name" , se->idstr);json_prop_int (vmdesc,"instance_id" , se->instance_id); -/* Section type */ -qemu_put_byte (f,QEMU_VM_SECTION_FULL ); -qemu_put_be32 (f, se->section_id); - -/* ID string */ - len =strlen (se->idstr); -qemu_put_byte (f, len); -qemu_put_buffer (f, (uint8_t *)se->idstr, len); - -qemu_put_be32 (f, se->instance_id); -qemu_put_be32 (f, se->version_id); +save_section_header (f, se,QEMU_VM_SECTION_FULL );vmstate_save (f, se, vmdesc);
@@ -873,8 +868,6 @@ static int qemu_save_device_state(QEMUFile *f)cpu_synchronize_all_states (); QTAILQ_FOREACH(se, &savevm_handlers, entry) { -int len; -if (se->is_ram) {continue ; }
@@ -882,17 +875,7 @@ static int qemu_save_device_state(QEMUFile *f)continue ; } -/* Section type */ -qemu_put_byte (f,QEMU_VM_SECTION_FULL ); -qemu_put_be32 (f, se->section_id); - -/* ID string */ - len =strlen (se->idstr); -qemu_put_byte (f, len); -qemu_put_buffer (f, (uint8_t *)se->idstr, len); - -qemu_put_be32 (f, se->instance_id); -qemu_put_be32 (f, se->version_id); +save_section_header (f, se,QEMU_VM_SECTION_FULL );vmstate_save (f, se,NULL ); }
Signature · 1 line
2.4.1
Re: [Qemu-devel] [PATCH 1/4] Merge section header writing
Quoted 6 lines · Dr. David Alan Gilbert (git)
From: "Dr. David Alan Gilbert" <[email protected]>
The header writing for device sections is open coded in a few places, merge it into one.
Signed-off-by Dr. David Alan Gilbert <[email protected]>
PATCH 2/4 Disable section footers on older machine types
In reply to Dr. David Alan Gilbert (git)From: "Dr. David Alan Gilbert" <[email protected]>
The next patch adds section footers; but we don't want to break migration compatibility so disable them on older machine types
--- hw/i386/pc_piix.c | 2 ++ hw/i386/pc_q35.c | 2 ++ include/migration/migration.h | 1 + savevm.c | 7 +++++++ 4 files changed, 12 insertions(+)
hw/i386/pc_piix.c +2 -0
@@ -52,6 +52,7 @@#ifdef CONFIG_XEN # include <xen/hvm/hvm_info_table.h> #endif +#include "migration/migration.h" #define MAX_IDE_BUS 2
@@ -312,6 +313,7 @@ static void pc_init_pci(MachineState *machine)static void pc_compat_2_3 (MachineState *machine) { +savevm_skip_section_footers (); }static void pc_compat_2_2 (MachineState *machine)
hw/i386/pc_q35.c +2 -0
@@ -45,6 +45,7 @@#include "hw/usb.h" #include "hw/cpu/icc_bus.h" #include "qemu/error-report.h" +#include "migration/migration.h" /* ICH9 AHCI has 6 ports */ #define MAX_SATA_PORTS 6
@@ -291,6 +292,7 @@ static void pc_q35_init(MachineState *machine)static void pc_compat_2_3 (MachineState *machine) { +savevm_skip_section_footers (); }static void pc_compat_2_2 (MachineState *machine)
include/migration/migration.h +1 -0
@@ -180,4 +180,5 @@ size_t ram_control_save_page(QEMUFile *f, ram_addr_t block_offset, ram_addr_t offset, size_t size, uint64_t *bytes_sent); +void savevm_skip_section_footers (void );#endif
savevm.c +7 -0
@@ -51,6 +51,8 @@#define ARP_PTYPE_IP 0x0800#define ARP_OP_REQUEST_REV 0x3 +static bool skip_section_footers; +static int announce_self_create (uint8_t *buf,uint8_t *mac_addr) {
@@ -602,6 +604,11 @@ static void vmstate_save(QEMUFile *f, SaveStateEntry *se, QJSON *vmdesc)vmstate_save_state (f, se->vmsd, se->opaque, vmdesc); } +void savevm_skip_section_footers (void ) +{ + skip_section_footers = true; +} +/* * Write the header for device section (QEMU_VM_SECTION START/END/PART/FULL) */
Signature · 1 line
2.4.1
Re: [Qemu-devel] [PATCH 2/4] Disable section footers on older machine types
Quoted 7 lines · Dr. David Alan Gilbert (git)
From: "Dr. David Alan Gilbert" <[email protected]>
The next patch adds section footers; but we don't want to break migration compatibility so disable them on older machine types
Signed-off-by Dr. David Alan Gilbert <[email protected]>
But ....
Power people
ARM people
Do you care now about cross version migration?
Later, Juan.
Re: [Qemu-devel] [PATCH 2/4] Disable section footers on older machine types
Quoted 7 lines · * Juan Quintela ([email protected]) wrote:
Quoted 6 lines · Dr. David Alan Gilbert (git)
From: "Dr. David Alan Gilbert" <[email protected]>
The next patch adds section footers; but we don't want to break migration compatibility so disable them on older machine types
Quoted 7 lines
Quoted 1 line
Signed-off-by Dr. David Alan Gilbert <[email protected]>Reviewed-by Juan Quintela <[email protected]>But ....
Power people
It looks like Power does:
ppc/spapr.c
1828:static void spapr_compat_2_3(Object *obj)
like the PC ones; so I guess we need to roll this flag, and your two flags into a common function somewhere and clal it from there?
Dave
Quoted 5 lines
ARM people
Do you care now about cross version migration?
Later, Juan.
--
Dr. David Alan Gilbert / [email protected] / Manchester, UK
PATCH 3/4 Add a protective section footer
In reply to Dr. David Alan Gilbert (git)From: "Dr. David Alan Gilbert" <[email protected]>
Badly formatted migration streams can go undetected or produce misleading errors due to a lock of checking at the end of sections. In particular a section that adds an extra 0x00 at the end causes what looks like a normal end of stream and thus doesn't produce any errors, and something that ends in a 0x01..0x04 kind of look like real section headers and then fail when the section parser tries to figure out which section they are. This is made worse by the choice of 0x00..0x04 being small numbers that are particularly common in normal section data.
This patch adds a section footer consisting of a marker (0x7e - ~) followed by the section-id that was also sent in the header. If they mismatch then it throws an error explaining which section was being loaded.
--- include/migration/migration.h | 1 + savevm.c | 61 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+)
include/migration/migration.h +1 -0
@@ -34,6 +34,7 @@#define QEMU_VM_SECTION_FULL 0x04#define QEMU_VM_SUBSECTION 0x05#define QEMU_VM_VMDESCRIPTION 0x06 +#define QEMU_VM_SECTION_FOOTER 0x7estruct MigrationParams {bool blk;
savevm.c +61 -0
109 lines not on this page · raw message
Signature · 1 line
2.4.1
Re: [Qemu-devel] [PATCH 3/4] Add a protective section footer
Quoted 12 lines · Dr. David Alan Gilbert (git)
From: "Dr. David Alan Gilbert" <[email protected]>
Badly formatted migration streams can go undetected or produce misleading errors due to a lock of checking at the end of sections. In particular a section that adds an extra
0x00at the end causes what looks like a normal end of stream and thus doesn't produce any errors, and something that ends in a 0x01..0x04 kind of look like real section headers and then fail when the section parser tries to figure out which section they are. This is made worse by the choice of 0x00..0x04 being small numbers that are particularly common in normal section data.
Quoted 6 lines
This patch adds a section footer consisting of a marker (
0x7e- ~) followed by the section-id that was also sent in the header. If they mismatch then it throws an error explaining which section was being loaded.Signed-off-by Dr. David Alan Gilbert <[email protected]>
PATCH 4/4 Teach analyze-migration.py about section footers
In reply to Dr. David Alan Gilbert (git)From: "Dr. David Alan Gilbert" <[email protected]>
--- scripts/analyze-migration.py | 5 +++++ 1 file changed, 5 insertions(+)
scripts/analyze-migration.py +5 -0
@@ -474,6 +474,7 @@ class MigrationDump(object): QEMU_VM_SECTION_FULL = 0x04 QEMU_VM_SUBSECTION = 0x05 QEMU_VM_VMDESCRIPTION = 0x06 + QEMU_VM_SECTION_FOOTER= 0x7e def __init__(self, filename): self.section_classes = { ( 'ram', 0 ) : [ RamSection, None ],
@@ -526,6 +527,10 @@ class MigrationDump(object): elif section_type == self.QEMU_VM_SECTION_PART or section_type == self.QEMU_VM_SECTION_END: section_id = file.read32() self.sections[section_id].read() + elif section_type == self.QEMU_VM_SECTION_FOOTER: + read_section_id = file.read32() + if read_section_id != section_id: + raise Exception("Mismatched section footer: %x vs %x" % (read_section_id, section_id)) else: raise Exception("Unknown section type: %d" % section_type) file.close()
Signature · 1 line
2.4.1
Quoted 12 lines · Dr. David Alan Gilbert (git)
From: "Dr. David Alan Gilbert" <[email protected]>
Badly formatted migration streams can go undetected or produce misleading errors due to a lock of checking at the end of sections. In particular a section that adds an extra
0x00at the end causes what looks like a normal end of stream and thus doesn't produce any errors, and something that ends in a 0x01..0x04 kind of look like real section headers and then fail when the section parser tries to figure out which section they are. This is made worse by the choice of 0x00..0x04 being small numbers that are particularly common in normal section data.
Quoted 4 lines
This patch series adds a section footer consisting of a marker (
0x7e- ~) followed by the section-id that was also sent in the header. If they mismatch then it throws an error explaining which section was being loaded.
Good idea. Is it redundant with the recent addition of self-describing json that newer machine types send? Does it let us detect a corrupted stream earlier in the process? Or is the main benefit that it gives better error messages at the point corruption is first detected?
Quoted 2 lines
The footers are tied to new machine types (on both pc types).
Good that you tied it to machine type, but is it enough? When we added the optional section for giving the json representation of the stream, we ended up having to add a knob to turn off that section, so that backwards migration from a new qemu to an older one did not send it. I'm wondering if we'll need to expose a knob to turn off footers, again for the sake of backwards migration in downstream distros.
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Quoted 18 lines · * Eric Blake ([email protected]) wrote:
Quoted 12 lines · Dr. David Alan Gilbert (git)
From: "Dr. David Alan Gilbert" <[email protected]>
Badly formatted migration streams can go undetected or produce misleading errors due to a lock of checking at the end of sections. In particular a section that adds an extra
0x00at the end causes what looks like a normal end of stream and thus doesn't produce any errors, and something that ends in a 0x01..0x04 kind of look like real section headers and then fail when the section parser tries to figure out which section they are. This is made worse by the choice of 0x00..0x04 being small numbers that are particularly common in normal section data.Quoted 4 lines
This patch series adds a section footer consisting of a marker (
0x7e- ~) followed by the section-id that was also sent in the header. If they mismatch then it throws an error explaining which section was being loaded.
Quoted 4 lines
Good idea.
Is it redundant with the recent addition of self-describing json that newer machine types send?
No, that self-describing json goes at the end, and is for parsing by an external-entity; it doesn't help detect corruption on loading.
Quoted 3 lines
Does it let us detect a corrupted
stream earlier in the process? Or is the main benefit that it gives better error messages at the point corruption is first detected?
Both; there are two cases that often happen; both triggered by a section reading too little or too much, and it gets back to the main loop and we read the next byte:
1) the next byte on the stream is a 0x00 - that's read as an end-of-migration marker, we start the VM and you get a hung VM with no errors.
2) the next byte is between 0x01..0x04 - and it looks like a section header, then we try and read the next few bytes to figure out which section; this could a) result in an error saying it's an unknown section or
b) Happen to match a section ID and then get an error about a problem in that section. In either case you don't get an error pointing to the previous section which was the actual problem.
Quoted 9 lines
Quoted 2 lines
The footers are tied to new machine types (on both pc types).
Good that you tied it to machine type, but is it enough? When we added the optional section for giving the json representation of the stream, we ended up having to add a knob to turn off that section, so that backwards migration from a new qemu to an older one did not send it. I'm wondering if we'll need to expose a knob to turn off footers, again for the sake of backwards migration in downstream distros.
That knob is already the knob that I've created and tied to the machine type; the downstream distros will just turn the same knob in their old machine types.
Dave
Quoted 5 lines
Signature · 3 lines
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
--
Dr. David Alan Gilbert / [email protected] / Manchester, UK
Quoted 10 lines · Dr. David Alan Gilbert
Quoted 3 lines
Does it let us detect a corrupted
stream earlier in the process? Or is the main benefit that it gives better error messages at the point corruption is first detected?Both; there are two cases that often happen; both triggered by a section reading too little or too much, and it gets back to the main loop and we read the next byte:
1) the next byte on the stream is a0x00- that's read as an end-of-migration marker, we start the VM and you get a hung VM with no errors.
Quoted 6 lines
2) the next byte is between 0x01..0x04 - and it looks like a section header, then we try and read the next few bytes to figure out which section; this could a) result in an error saying it's an unknown section or
b) Happen to match a section ID and then get an error about a problem in that section. In either case you don't get an error pointing to the previous section which was the actual problem.
Probably worth incorporating into the commit body then :)
Quoted 11 lines
Quoted 10 lines
Quoted 3 lines
The footers are tied to new machine types (on both pc types).
Good that you tied it to machine type, but is it enough? When we added the optional section for giving the json representation of the stream, we ended up having to add a knob to turn off that section, so that backwards migration from a new qemu to an older one did not send it. I'm wondering if we'll need to expose a knob to turn off footers, again for the sake of backwards migration in downstream distros.
Quoted 3 lines
That knob is already the knob that I've created and tied to the machine type; the downstream distros will just turn the same knob in their old machine types.
I'm not asking about the machine type defaults, but a command line
override: -machine suppress-vmdesc=on, commit 9850c604
[uggh - why'd we give that option an inverted name? Just so we could have a default of off? It might have been nicer as -machine send-vmdesc=off with a default of on for new machine types]
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Quoted 12 lines · * Eric Blake ([email protected]) wrote:
Quoted 10 lines · Dr. David Alan Gilbert
Quoted 3 lines
Does it let us detect a corrupted
stream earlier in the process? Or is the main benefit that it gives better error messages at the point corruption is first detected?Both; there are two cases that often happen; both triggered by a section reading too little or too much, and it gets back to the main loop and we read the next byte:
1) the next byte on the stream is a0x00- that's read as an end-of-migration marker, we start the VM and you get a hung VM with no errors.
Quoted 8 lines
Quoted 6 lines
2) the next byte is between 0x01..0x04 - and it looks like a section header, then we try and read the next few bytes to figure out which section; this could a) result in an error saying it's an unknown section or
b) Happen to match a section ID and then get an error about a problem in that section. In either case you don't get an error pointing to the previous section which was the actual problem.Probably worth incorporating into the commit body then :)
Well the original text does say:
Badly formatted migration streams can go undetected or produce misleading errors due to a lock of checking at the end of sections. In particular a section that adds an extra 0x00 at the end causes what looks like a normal end of stream and thus doesn't produce any errors, and something that ends in a 0x01..0x04 kind of look like real section headers and then fail when the section parser tries to figure out which section they are. This is made worse by the choice of 0x00..0x04 being small numbers that are particularly common in normal section data.
which is pretty close to that; do you want me to flip that other explanation in?
Quoted 11 lines
Quoted 11 lines
Quoted 10 lines
Quoted 3 lines
The footers are tied to new machine types (on both pc types).
Good that you tied it to machine type, but is it enough? When we added the optional section for giving the json representation of the stream, we ended up having to add a knob to turn off that section, so that backwards migration from a new qemu to an older one did not send it. I'm wondering if we'll need to expose a knob to turn off footers, again for the sake of backwards migration in downstream distros.
Quoted 6 lines
Quoted 3 lines
That knob is already the knob that I've created and tied to the machine type; the downstream distros will just turn the same knob in their old machine types.
I'm not asking about the machine type defaults, but a command line
override: -machine suppress-vmdesc=on, commit9850c604
That shouldn't be necessary; VMdesc was 'odd' in that it wrote data after the end marker which broke some implicit rules about the behaviour of the streams and the way they interacted with the file buffers. I'd have sympathy for the opposite direction - i.e. turning on the footer protection for older machine types when you know you've got modern qemu's; but it doesn't seem worth the extra boiler plate unless someone wants to do it (especially since it looks from that like it takes two functions and ~20 lines of code to add one boolean flag to the machine type!).
Quoted 3 lines
[uggh - why'd we give that option an inverted name? Just so we could have a default of off? It might have been nicer as -machine send-vmdesc=off with a default of on for new machine types]
We seem to do that a lot - and indeed my code does that to keep it consistent with the other uses. I think we like to assume 0 initialised flags do the 'normal' thing; still nothing like a double or triple negative to keep you on your guard.
Dave
Quoted 4 lines
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
--
Dr. David Alan Gilbert / [email protected] / Manchester, UK
Quoted 33 lines · Dr. David Alan Gilbert
Quoted 12 lines · * Eric Blake ([email protected]) wrote:
Quoted 10 lines · Dr. David Alan Gilbert
Quoted 3 lines
Does it let us detect a corrupted
stream earlier in the process? Or is the main benefit that it gives better error messages at the point corruption is first detected?Both; there are two cases that often happen; both triggered by a section reading too little or too much, and it gets back to the main loop and we read the next byte:
1) the next byte on the stream is a0x00- that's read as an end-of-migration marker, we start the VM and you get a hung VM with no errors.Quoted 8 lines
Quoted 6 lines
2) the next byte is between 0x01..0x04 - and it looks like a section header, then we try and read the next few bytes to figure out which section; this could a) result in an error saying it's an unknown section or
b) Happen to match a section ID and then get an error about a problem in that section. In either case you don't get an error pointing to the previous section which was the actual problem.Probably worth incorporating into the commit body then :)
Well the original text does say:
Badly formatted migration streams can go undetected or produce misleading errors due to a lock of checking at the end of sections. In particular a section that adds an extra0x00at the end causes what looks like a normal end of stream and thus doesn't produce any errors, and something that ends in a 0x01..0x04 kind of look like real section headers and then fail when the section parser tries to figure out which section they are. This is made worse by the choice of 0x00..0x04 being small numbers that are particularly common in normal section data.
Quoted 1 line
which is pretty close to that; do you want me to flip that other explanation in?
Up to you, but I found the bullet points a bit more descriptive (for example, bullet 1 mentions a hung VM, while the original text says "no errors" but doesn't mention "hung").
Quoted 12 lines
Quoted 2 lines
I'm not asking about the machine type defaults, but a command line
override: -machine suppress-vmdesc=on, commit9850c604That shouldn't be necessary; VMdesc was 'odd' in that it wrote data after the end marker which broke some implicit rules about the behaviour of the streams and the way they interacted with the file buffers. I'd have sympathy for the opposite direction - i.e. turning on the footer protection for older machine types when you know you've got modern qemu's; but it doesn't seem worth the extra boiler plate unless someone wants to do it (especially since it looks from that like it takes two functions and ~20 lines of code to add one boolean flag to the machine type!).
We may still add it later. And since suppress-vmdesc was added later, I can live with the decision if you don't want to add command-line override on the initial commit series.
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Quoted 63 lines · Dr. David Alan Gilbert
Quoted 12 lines · * Eric Blake ([email protected]) wrote:
Quoted 10 lines · Dr. David Alan Gilbert
Quoted 3 lines
Does it let us detect a corrupted
stream earlier in the process? Or is the main benefit that it gives better error messages at the point corruption is first detected?Both; there are two cases that often happen; both triggered by a section reading too little or too much, and it gets back to the main loop and we read the next byte:
1) the next byte on the stream is a0x00- that's read as an end-of-migration marker, we start the VM and you get a hung VM with no errors.Quoted 8 lines
Quoted 6 lines
2) the next byte is between 0x01..0x04 - and it looks like a section header, then we try and read the next few bytes to figure out which section; this could a) result in an error saying it's an unknown section or
b) Happen to match a section ID and then get an error about a problem in that section. In either case you don't get an error pointing to the previous section which was the actual problem.Probably worth incorporating into the commit body then :)
Well the original text does say:
Badly formatted migration streams can go undetected or produce misleading errors due to a lock of checking at the end of sections. In particular a section that adds an extra0x00at the end causes what looks like a normal end of stream and thus doesn't produce any errors, and something that ends in a 0x01..0x04 kind of look like real section headers and then fail when the section parser tries to figure out which section they are. This is made worse by the choice of 0x00..0x04 being small numbers that are particularly common in normal section data.which is pretty close to that; do you want me to flip that other explanation in?
Quoted 11 lines
Quoted 11 lines
Quoted 10 lines
Quoted 3 lines
The footers are tied to new machine types (on both pc types).
Good that you tied it to machine type, but is it enough? When we added the optional section for giving the json representation of the stream, we ended up having to add a knob to turn off that section, so that backwards migration from a new qemu to an older one did not send it. I'm wondering if we'll need to expose a knob to turn off footers, again for the sake of backwards migration in downstream distros.
Quoted 6 lines
Quoted 3 lines
That knob is already the knob that I've created and tied to the machine type; the downstream distros will just turn the same knob in their old machine types.
I'm not asking about the machine type defaults, but a command line
override: -machine suppress-vmdesc=on, commit9850c604That shouldn't be necessary; VMdesc was 'odd' in that it wrote data after the end marker which broke some implicit rules about the behaviour of the streams and the way they interacted with the file buffers. I'd have sympathy for the opposite direction - i.e. turning on the footer protection for older machine types when you know you've got modern qemu's; but it doesn't seem worth the extra boiler plate unless someone wants to do it (especially since it looks from that like it takes two functions and ~20 lines of code to add one boolean flag to the machine type!).
Quoted 8 lines
Quoted 3 lines
[uggh - why'd we give that option an inverted name? Just so we could have a default of off? It might have been nicer as -machine send-vmdesc=off with a default of on for new machine types]
We seem to do that a lot - and indeed my code does that to keep it consistent with the other uses. I think we like to assume 0 initialised flags do the 'normal' thing; still nothing like a double or triple negative to keep you on your guard.
In my case here, is that we want the "new" functionality to be the default, and just disable it for the old machine types.
So, for now on, section footers are sent. Except, if we are on an old machine type, that we don't sent them then. So, the only obvious way that I can think of is naming it as "skip_foo". Notice that we have also to accomodate the achitecture that don't care about cross-version migration, and there, we just want the new behaviour.
If you have a nicer approach on mind, I am all for it.
THanks, Juan.
Quoted 11 lines
Dave
Quoted 4 lines
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org--
Dr. David Alan Gilbert / [email protected] / Manchester, UK
Quoted 12 lines · Eric Blake
Quoted 10 lines · Dr. David Alan Gilbert
Quoted 3 lines
Does it let us detect a corrupted
stream earlier in the process? Or is the main benefit that it gives better error messages at the point corruption is first detected?Both; there are two cases that often happen; both triggered by a section reading too little or too much, and it gets back to the main loop and we read the next byte:
1) the next byte on the stream is a0x00- that's read as an end-of-migration marker, we start the VM and you get a hung VM with no errors.
Quoted 8 lines
Quoted 6 lines
2) the next byte is between 0x01..0x04 - and it looks like a section header, then we try and read the next few bytes to figure out which section; this could a) result in an error saying it's an unknown section or
b) Happen to match a section ID and then get an error about a problem in that section. In either case you don't get an error pointing to the previous section which was the actual problem.Probably worth incorporating into the commit body then :)
How about docs/migration.txt? The 00/NN message gets lost..
Amit
Quoted 4 lines · Dr. David Alan Gilbert (git)
From: "Dr. David Alan Gilbert" <[email protected]>
Badly formatted migration streams can go undetected or produce misleading errors due to a lock of checking at the end of sections.
s/lock/lack
in patch 3 commit msg as well.
Otherwise, looks good.
Amit