migration: Improve subsections detection
cover migration: Improve subsections detection
Hi
This series move the subsections detection code form:
- Look that it starts form 5
To:
- Look that it starts form 5 (SUBSECTION)
- Look at the length
- Look that length is bigger than section name
- Look at the idstr and see that it starts with the subsection name.
Please review.
Later, Juan.
Juan Quintela (4):
savevm: teach qemu_fill_buffer to do partial refills
savevm: some coding style cleanups
savevm: improve subsections detection on load Revert "savevm: fix corruption in vmstate_subsection_load()."
savevm.c | 112 ++++++++++++++++++++++++++++++++++++++++++++++--------------- 1 files changed, 84 insertions(+), 28 deletions(-)
Signature · 1 line
1.7.6.4
PATCH 1/4 savevm: teach qemu_fill_buffer to do partial refills
We will need on next patch to be able to lookahead on next patch
--- savevm.c | 14 +++++++++++--- 1 files changed, 11 insertions(+), 3 deletions(-)
savevm.c +11 -3
@@ -455,6 +455,7 @@ void qemu_fflush(QEMUFile *f)static void qemu_fill_buffer (QEMUFile *f) {int len; +int used;if (!f->get_buffer)return ;
@@ -462,10 +463,17 @@ static void qemu_fill_buffer(QEMUFile *f)if (f->is_write)abort (); - len = f->get_buffer (f->opaque, f->buf, f->buf_offset,IO_BUF_SIZE ); + used = f->buf_size - f->buf_index; +if (used >0 ) { +memmove (f->buf, f->buf + f->buf_index, used); + } + f->buf_index =0 ; + f->buf_size = used; + + len = f->get_buffer (f->opaque, f->buf + used, f->buf_offset, +IO_BUF_SIZE - used);if (len >0 ) { - f->buf_index =0 ; - f->buf_size = len; + f->buf_size += len; f->buf_offset += len; }else if (len != -EAGAIN ) f->has_error =1 ;
Signature · 1 line
1.7.6.4
Re: [Qemu-devel] [PATCH 1/4] savevm: teach qemu_fill_buffer to do partial refills
Quoted 3 lines · Juan Quintela
We will need on next patch to be able to lookahead on next patch
Signed-off-by Juan Quintela<[email protected]>
Regards,
Anthony Liguori
Quoted 37 lines
--- savevm.c | 14 +++++++++++--- 1 files changed, 11 insertions(+), 3 deletions(-)savevm.c +11
-3@@ -455,6 +455,7 @@ void qemu_fflush(QEMUFile *f)static void qemu_fill_buffer (QEMUFile *f) {int len; +int used;if (!f->get_buffer)return ;@@ -462,10 +463,17 @@ static void qemu_fill_buffer(QEMUFile *f)if (f->is_write)abort (); - len = f->get_buffer (f->opaque, f->buf, f->buf_offset,IO_BUF_SIZE ); + used = f->buf_size - f->buf_index; +if (used>0 ) { +memmove (f->buf, f->buf + f->buf_index, used); + } + f->buf_index =0 ; + f->buf_size = used; + + len = f->get_buffer (f->opaque, f->buf + used, f->buf_offset, +IO_BUF_SIZE - used);if (len>0 ) { - f->buf_index =0 ; - f->buf_size = len; + f->buf_size += len; f->buf_offset += len; }else if (len != -EAGAIN ) f->has_error =1 ;
PATCH 2/4 savevm: some coding style cleanups
In reply to Juan QuintelaThis patch will make moving code on next patches and having checkpatch happy easier.
--- savevm.c | 21 ++++++++++++++------- 1 files changed, 14 insertions(+), 7 deletions(-)
savevm.c +14 -7
@@ -536,8 +536,9 @@ int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int size1) {int size, l; -if (f->is_write) +if (f->is_write) {abort (); + } size = size1;while (size >0 ) {
@@ -545,11 +546,13 @@ int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int size1)if (l ==0 ) {qemu_fill_buffer (f); l = f->buf_size - f->buf_index; -if (l ==0 ) +if (l ==0 ) {break ; + } } -if (l > size) +if (l > size) { l = size; + }memcpy (buf, f->buf + f->buf_index, l); f->buf_index += l; buf += l;
@@ -560,26 +563,30 @@ int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int size1)static int qemu_peek_byte (QEMUFile *f) { -if (f->is_write) +if (f->is_write) {abort (); + }if (f->buf_index >= f->buf_size) {qemu_fill_buffer (f); -if (f->buf_index >= f->buf_size) +if (f->buf_index >= f->buf_size) {return 0 ; + } }return f->buf[f->buf_index]; }int qemu_get_byte (QEMUFile *f) { -if (f->is_write) +if (f->is_write) {abort (); + }if (f->buf_index >= f->buf_size) {qemu_fill_buffer (f); -if (f->buf_index >= f->buf_size) +if (f->buf_index >= f->buf_size) {return 0 ; + } }return f->buf[f->buf_index++]; }
Signature · 1 line
1.7.6.4
Re: [Qemu-devel] [PATCH 2/4] savevm: some coding style cleanups
Quoted 4 lines · Juan Quintela
This patch will make moving code on next patches and having checkpatch happy easier.
Signed-off-by Juan Quintela<[email protected]>
Regards,
Anthony Liguori
Quoted 70 lines
--- savevm.c | 21 ++++++++++++++------- 1 files changed, 14 insertions(+), 7 deletions(-)savevm.c +14
-7@@ -536,8 +536,9 @@ int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int size1) {int size, l; -if (f->is_write) +if (f->is_write) {abort (); + } size = size1;while (size>0 ) {@@ -545,11 +546,13 @@ int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int size1)if (l ==0 ) {qemu_fill_buffer (f); l = f->buf_size - f->buf_index; -if (l ==0 ) +if (l ==0 ) {break ; + } } -if (l> size) +if (l> size) { l = size; + }memcpy (buf, f->buf + f->buf_index, l); f->buf_index += l; buf += l;@@ -560,26 +563,30 @@ int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int size1)static int qemu_peek_byte (QEMUFile *f) { -if (f->is_write) +if (f->is_write) {abort (); + }if (f->buf_index>= f->buf_size) {qemu_fill_buffer (f); -if (f->buf_index>= f->buf_size) +if (f->buf_index>= f->buf_size) {return 0 ; + } }return f->buf[f->buf_index]; }int qemu_get_byte (QEMUFile *f) { -if (f->is_write) +if (f->is_write) {abort (); + }if (f->buf_index>= f->buf_size) {qemu_fill_buffer (f); -if (f->buf_index>= f->buf_size) +if (f->buf_index>= f->buf_size) {return 0 ; + } }return f->buf[f->buf_index++]; }
PATCH 3/4 savevm: improve subsections detection on load
In reply to Juan QuintelaWe add qemu_peek_buffer, that is identical to qemu_get_buffer, just that it don't update f->buf_index.
We add a paramenter to qemu_peek_byte() to be able to peek more than one byte.
Once this is done, to see if we have a subsection we look:
- 1st byte is QEMU_VM_SUBSECTION
- 2nd byte is a length, and is bigger than section name
- 3rd element is a string that starts with section_name
So, we shouldn't have false positives (yes, content could still get us wrong but probabilities are really low).
--- savevm.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++++--------- 1 files changed, 60 insertions(+), 11 deletions(-)
savevm.c +60 -11
@@ -532,6 +532,37 @@ void qemu_put_byte(QEMUFile *f, int v)qemu_fflush (f); } +static int qemu_peek_buffer (QEMUFile *f,uint8_t *buf,int size1,int offset) +{ +int size, l; +int index = f->buf_index + offset; + +if (f->is_write) { +abort (); + } + + size = size1; +while (size >0 ) { + l = f->buf_size - index; +if (l ==0 ) { +qemu_fill_buffer (f); + index = f->buf_index + offset; + l = f->buf_size - index; +if (l ==0 ) { +break ; + } + } +if (l > size) { + l = size; + } +memcpy (buf, f->buf + index, l); + index += l; + buf += l; + size -= l; + } +return size1 - size; +} +int qemu_get_buffer (QEMUFile *f,uint8_t *buf,int size1) {int size, l;
@@ -561,19 +592,22 @@ int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int size1)return size1 - size; } -static int qemu_peek_byte (QEMUFile *f) +static int qemu_peek_byte (QEMUFile *f,int offset) { +int index = f->buf_index + offset; +if (f->is_write) {abort (); } -if (f->buf_index >= f->buf_size) { +if (index >= f->buf_size) {qemu_fill_buffer (f); -if (f->buf_index >= f->buf_size) { + index = f->buf_index + offset; +if (index >= f->buf_size) {return 0 ; } } -return f->buf[f->buf_index]; +return f->buf[index]; }int qemu_get_byte (QEMUFile *f)
@@ -1687,22 +1721,37 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,return 0 ; } -while (qemu_peek_byte (f) ==QEMU_VM_SUBSECTION ) { +while (qemu_peek_byte (f,0 ) ==QEMU_VM_SUBSECTION ) {char idstr[256 ];int ret; -uint8_t version_id, len; +uint8_t version_id, len, size;const VMStateDescription *sub_vmsd; -qemu_get_byte (f);/* subsection */ - len =qemu_get_byte (f); -qemu_get_buffer (f, (uint8_t *)idstr, len); - idstr[len] =0 ; - version_id =qemu_get_be32 (f); + len =qemu_peek_byte (f,1 ); +if (len <strlen (vmsd->name) +1 ) { +/* subsection name has be be "section_name/a" */ +return 0 ; + } + size =qemu_peek_buffer (f, (uint8_t *)idstr, len,2 ); +if (size != len) { +return 0 ; + } + idstr[size] =0 ; +if (strncmp (vmsd->name, idstr,strlen (vmsd->name)) !=0 ) { +/* it don't have a valid subsection name */ +return 0 ; + } sub_vmsd =vmstate_get_subsection (sub, idstr);if (sub_vmsd ==NULL ) {return -ENOENT ; } +qemu_get_byte (f);/* subsection */ +qemu_get_byte (f);/* len */ +qemu_get_buffer (f, (uint8_t *)idstr, len); + idstr[len] =0 ; + version_id =qemu_get_be32 (f); +assert (!sub_vmsd->subsections); ret =vmstate_load_state (f, sub_vmsd, opaque, version_id);if (ret) {
Signature · 1 line
1.7.6.4
Re: [Qemu-devel] [PATCH 3/4] savevm: improve subsections detection on load
Quoted 51 lines · Juan Quintela
We add
qemu_peek_buffer, that is identical toqemu_get_buffer, just that it don't updatef->buf_index.We add a paramenter to
qemu_peek_byte()to be able to peek more than one byte.Once this is done, to see if we have a subsection we look:
- 1st byte isQEMU_VM_SUBSECTION
- 2nd byte is a length, and is bigger than section name
- 3rd element is a string that starts withsection_nameSo, we shouldn't have false positives (yes, content could still get us wrong but probabilities are really low).
Signed-off-by Juan Quintela<[email protected]>--- savevm.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++++--------- 1 files changed, 60 insertions(+), 11 deletions(-)savevm.c +24
-0@@ -532,6 +532,37 @@ void qemu_put_byte(QEMUFile *f, int v)qemu_fflush (f); } +static int qemu_peek_buffer (QEMUFile *f,uint8_t *buf,int size1,int offset) +{ +int size, l; +int index = f->buf_index + offset; + +if (f->is_write) { +abort (); + } + + size = size1; +while (size>0 ) { + l = f->buf_size - index; +if (l ==0 ) { +qemu_fill_buffer (f); + index = f->buf_index + offset; + l = f->buf_size - index; +if (l ==0 ) { +break ; + } + } +if (l> size) { + l = size; + } +memcpy (buf, f->buf + index, l);
savevm.c void qemu_put_byte(QEMUFile *f, int v) · line 568
Quoted 10 lines
savevm.c +7
-0+ index += l; + buf += l; + size -= l; + } +return size1 - size; +} +int qemu_get_buffer (QEMUFile *f,uint8_t *buf,int size1) {int size, l;
Can we implement get_buffer in terms of peek_buffer and just increment f->buf_index in get_buffer?
Quoted 44 lines
savevm.c +9
-11@@ -561,19 +592,22 @@ int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int size1)return size1 - size; } -static int qemu_peek_byte (QEMUFile *f) +static int qemu_peek_byte (QEMUFile *f,int offset) { +int index = f->buf_index + offset; +if (f->is_write) {abort (); } -if (f->buf_index>= f->buf_size) { +if (index>= f->buf_size) {qemu_fill_buffer (f); -if (f->buf_index>= f->buf_size) { + index = f->buf_index + offset; +if (index>= f->buf_size) {return 0 ; } } -return f->buf[f->buf_index]; +return f->buf[index]; }int qemu_get_byte (QEMUFile *f)@@ -1687,22 +1721,37 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,return 0 ; } -while (qemu_peek_byte (f) ==QEMU_VM_SUBSECTION ) { +while (qemu_peek_byte (f,0 ) ==QEMU_VM_SUBSECTION ) {char idstr[256 ];int ret; -uint8_t version_id, len; +uint8_t version_id, len, size;const VMStateDescription *sub_vmsd; -qemu_get_byte (f);/* subsection */ - len =qemu_get_byte (f); -qemu_get_buffer (f, (uint8_t *)idstr, len); - idstr[len] =0 ; - version_id =qemu_get_be32 (f);
savevm.c static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd, · line 1739
Quoted 10 lines
savevm.c +10
-0+ len =qemu_peek_byte (f,1 ); +if (len<strlen (vmsd->name) +1 ) { +/* subsection name has be be "section_name/a" */ +return 0 ; + } + size =qemu_peek_buffer (f, (uint8_t *)idstr, len,2 ); +if (size != len) { +return 0 ; + } + idstr[size] =0 ;
Regards,
Anthony Liguori
Re: [Qemu-devel] [PATCH 3/4] savevm: improve subsections detection on load
Quoted 7 lines · Anthony Liguori
Quoted 4 lines
int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int size1) { int size, l;Can we implement
get_bufferin terms ofpeek_bufferand just incrementf->buf_indexinget_buffer?
No, get_buffer works even if size1 is greater than the size of the buffer, peek_buffer doesn't.
Paolo
PATCH 4/4 Revert "savevm: fix corruption in vmstate_subsection_load()."
In reply to Juan QuintelaThis reverts commit eb60260de0b050a5e8ab725e84d377d0b44c43ae.
Conflicts:
savevm.c
We changed qemu_peek_byte() prototype, just fixed the rejects.
--- savevm.c | 10 +--------- 1 files changed, 1 insertions(+), 9 deletions(-)
savevm.c +1 -9
@@ -1715,12 +1715,6 @@ static const VMStateDescription *vmstate_get_subsection(const VMStateSubsectionstatic int vmstate_subsection_load (QEMUFile *f,const VMStateDescription *vmsd,void *opaque) { -const VMStateSubsection *sub = vmsd->subsections; - -if (!sub || !sub->needed) { -return 0 ; - } -while (qemu_peek_byte (f,0 ) ==QEMU_VM_SUBSECTION ) {char idstr[256 ];int ret;
@@ -1742,7 +1736,7 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,/* it don't have a valid subsection name */ return 0 ; } - sub_vmsd =vmstate_get_subsection (sub, idstr); + sub_vmsd =vmstate_get_subsection (vmsd->subsections, idstr);if (sub_vmsd ==NULL ) {return -ENOENT ; }
@@ -1752,7 +1746,6 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd, idstr[len] =0 ; version_id =qemu_get_be32 (f); -assert (!sub_vmsd->subsections); ret =vmstate_load_state (f, sub_vmsd, opaque, version_id);if (ret) {return ret;
@@ -1776,7 +1769,6 @@ static void vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd,qemu_put_byte (f, len);qemu_put_buffer (f, (uint8_t *)vmsd->name, len);qemu_put_be32 (f, vmsd->version_id); -assert (!vmsd->subsections);vmstate_save_state (f, vmsd, opaque); } sub++;
Signature · 1 line
1.7.6.4
Re: [Qemu-devel] [PATCH 4/4] Revert "savevm: fix corruption in vmstate_subsection_load()."
Quoted 9 lines · Juan Quintela
This reverts commit
eb60260de0b050a5e8ab725e84d377d0b44c43ae.Conflicts:
savevm.cWe changed
qemu_peek_byte()prototype, just fixed the rejects.Signed-off-by Juan Quintela<[email protected]>
Regards,
Anthony Liguori
Quoted 46 lines
--- savevm.c | 10 +--------- 1 files changed, 1 insertions(+), 9 deletions(-)savevm.c +1
-9@@ -1715,12 +1715,6 @@ static const VMStateDescription *vmstate_get_subsection(const VMStateSubsectionstatic int vmstate_subsection_load (QEMUFile *f,const VMStateDescription *vmsd,void *opaque) { -const VMStateSubsection *sub = vmsd->subsections; - -if (!sub || !sub->needed) { -return 0 ; - } -while (qemu_peek_byte (f,0 ) ==QEMU_VM_SUBSECTION ) {char idstr[256 ];int ret;@@ -1742,7 +1736,7 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,/* it don't have a valid subsection name */ return 0 ; } - sub_vmsd =vmstate_get_subsection (sub, idstr); + sub_vmsd =vmstate_get_subsection (vmsd->subsections, idstr);if (sub_vmsd ==NULL ) {return -ENOENT ; }@@ -1752,7 +1746,6 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd, idstr[len] =0 ; version_id =qemu_get_be32 (f); -assert (!sub_vmsd->subsections); ret =vmstate_load_state (f, sub_vmsd, opaque, version_id);if (ret) {return ret;@@ -1776,7 +1769,6 @@ static void vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd,qemu_put_byte (f, len);qemu_put_buffer (f, (uint8_t *)vmsd->name, len);qemu_put_be32 (f, vmsd->version_id); -assert (!vmsd->subsections);vmstate_save_state (f, vmsd, opaque); } sub++;
Quoted 11 lines · Juan Quintela
Hi
This series move the subsections detection code form:
- Look that it starts form 5
To:
- Look that it starts form 5 (SUBSECTION)
- Look at the length
- Look that length is bigger than section name
- Look at the idstr and see that it starts with the subsection name.Please review.
Looks good as an alternative to the new migration format.
Thanks,
Paolo
Quoted 10 lines · Juan Quintela
Hi
This series move the subsections detection code form:
- Look that it starts form 5
To:
- Look that it starts form 5 (SUBSECTION)
- Look at the length
- Look that length is bigger than section name
- Look at the idstr and see that it starts with the subsection name.
Quoted 2 lines
Please review.
The original intent with subsections was to register them as a new vmstate section, with just a name relationship.
Can we rename .subsections to .old_and_semi_broken_subsections, and introduce a new .subsections field that works properly in all cases?
Signature · 1 line
error compiling committee.c: too many arguments to function
Quoted 11 lines · Avi Kivity
Quoted 10 lines · Juan Quintela
Hi
This series move the subsections detection code form:
- Look that it starts form 5
To:
- Look that it starts form 5 (SUBSECTION)
- Look at the length
- Look that length is bigger than section name
- Look at the idstr and see that it starts with the subsection name.
Quoted 8 lines
Quoted 2 lines
Please review.
The original intent with subsections was to register them as a new vmstate section, with just a name relationship.
Can we rename .subsections to .old_and_semi_broken_subsections, and introduce a new .subsections field that works properly in all cases?
That gets a bit hairy. We don't really rely on section order today and I think this would introduce a section order requirement. I don't think it's a problem and agree it's probably a better path but it does require a little bit of consideration.
Regards,
Anthony Liguori
Quoted 1 line
Quoted 8 lines · Anthony Liguori
Quoted 5 lines
The original intent with subsections was to register them as a new vmstate section, with just a name relationship.
Can we rename .subsections to .old_and_semi_broken_subsections, and introduce a new .subsections field that works properly in all cases?
That gets a bit hairy. We don't really rely on section order today and I think this would introduce a section order requirement.
The information is there, since a subsection is contained within its section. In practice working in FIFO order preserves it.
Signature · 1 line
error compiling committee.c: too many arguments to function
Quoted 11 lines · Avi Kivity
Quoted 10 lines · Juan Quintela
Hi
This series move the subsections detection code form:
- Look that it starts form 5
To:
- Look that it starts form 5 (SUBSECTION)
- Look at the length
- Look that length is bigger than section name
- Look at the idstr and see that it starts with the subsection name.
Quoted 8 lines
Quoted 2 lines
Please review.
The original intent with subsections was to register them as a new vmstate section, with just a name relationship.
Can we rename .subsections to .old_and_semi_broken_subsections, and introduce a new .subsections field that works properly in all cases?
Not easily. We can try to do something, but the problem is that subsection are just that "sub", and we should have to be very careful with post-load handlers.
Will think about it, thought.
Later, Juan.
Quoted 18 lines · Juan Quintela
Quoted 17 lines · Avi Kivity
Quoted 10 lines · Juan Quintela
Hi
This series move the subsections detection code form:
- Look that it starts form 5
To:
- Look that it starts form 5 (SUBSECTION)
- Look at the length
- Look that length is bigger than section name
- Look at the idstr and see that it starts with the subsection name.Quoted 2 lines
Please review.
The original intent with subsections was to register them as a new vmstate section, with just a name relationship.
Quoted 7 lines
Quoted 2 lines
Can we rename .subsections to .old_and_semi_broken_subsections, and introduce a new .subsections field that works properly in all cases?
Not easily. We can try to do something, but the problem is that subsection are just that "sub", and we should have to be very careful with post-load handlers.
If necessary the subsection's post-load handler can re-run the main post-load handler (or it can even be the same post-load handler, unconditionally). We just have to make sure that post-load handlers are idempotent.
Signature · 1 line
error compiling committee.c: too many arguments to function