* [Qemu-devel] [PATCH 0/5] vmstate: Add max_version_id field
@ 2013-10-14 16:45 Eduardo Habkost
2013-10-14 16:45 ` [Qemu-devel] [PATCH 1/5] savevm: Coding style line length fix Eduardo Habkost
` (4 more replies)
0 siblings, 5 replies; 9+ messages in thread
From: Eduardo Habkost @ 2013-10-14 16:45 UTC (permalink / raw)
To: qemu-devel, Juan Quintela; +Cc: Paolo Bonzini, Markus Armbruster
This series cleans up the vmstate save/load code a little, and adds a
max_version_id field to VMStateDescription.
This will be useful in case we need to revert vmstate changes but keep
compatibility with QEMU versions that had a higher version_id. No existing
behavior will be changed by code that doesn't have max_version_id set.
One possible use case for this is the the "cpu" section, where the xsave CPU
state could be made optional by moving it to a "xsave" section, and then revert
version_id back to 11 while still accepting version_id==12 migration data.
Eduardo Habkost (5):
savevm: Coding style line length fix
vmstate: Replace while (...) with for (...)
vmstate: Simplify field-skipping load/save logic
vmstate: Use version_id when saving
vmstate: Add max_version_id field to VMStateDescription
include/migration/vmstate.h | 1 +
savevm.c | 170 +++++++++++++++++++++++---------------------
2 files changed, 89 insertions(+), 82 deletions(-)
--
1.8.3.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH 1/5] savevm: Coding style line length fix
2013-10-14 16:45 [Qemu-devel] [PATCH 0/5] vmstate: Add max_version_id field Eduardo Habkost
@ 2013-10-14 16:45 ` Eduardo Habkost
2013-10-14 16:45 ` [Qemu-devel] [PATCH 2/5] vmstate: Replace while (...) with for (...) Eduardo Habkost
` (3 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: Eduardo Habkost @ 2013-10-14 16:45 UTC (permalink / raw)
To: qemu-devel, Juan Quintela; +Cc: Paolo Bonzini, Markus Armbruster
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
savevm.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/savevm.c b/savevm.c
index 2f631d4..208e7c2 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1729,7 +1729,8 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
addr = *(void **)addr;
}
if (field->flags & VMS_STRUCT) {
- ret = vmstate_load_state(f, field->vmsd, addr, field->vmsd->version_id);
+ ret = vmstate_load_state(f, field->vmsd, addr,
+ field->vmsd->version_id);
} else {
ret = field->info->get(f, addr, size);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH 2/5] vmstate: Replace while (...) with for (...)
2013-10-14 16:45 [Qemu-devel] [PATCH 0/5] vmstate: Add max_version_id field Eduardo Habkost
2013-10-14 16:45 ` [Qemu-devel] [PATCH 1/5] savevm: Coding style line length fix Eduardo Habkost
@ 2013-10-14 16:45 ` Eduardo Habkost
2013-10-14 16:45 ` [Qemu-devel] [PATCH 3/5] vmstate: Simplify field-skipping load/save logic Eduardo Habkost
` (2 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: Eduardo Habkost @ 2013-10-14 16:45 UTC (permalink / raw)
To: qemu-devel, Juan Quintela; +Cc: Paolo Bonzini, Markus Armbruster
This will make it easier to add code that skips some fields, by simply
using "continue".
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
savevm.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/savevm.c b/savevm.c
index 208e7c2..9562669 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1676,7 +1676,7 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
void *opaque, int version_id)
{
- VMStateField *field = vmsd->fields;
+ VMStateField *field;
int ret;
if (version_id > vmsd->version_id) {
@@ -1693,7 +1693,7 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
if (ret)
return ret;
}
- while(field->name) {
+ for (field = vmsd->fields; field->name; field++) {
if ((field->field_exists &&
field->field_exists(opaque, version_id)) ||
(!field->field_exists &&
@@ -1740,7 +1740,6 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
}
}
}
- field++;
}
ret = vmstate_subsection_load(f, vmsd, opaque);
if (ret != 0) {
@@ -1755,12 +1754,12 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
void *opaque)
{
- VMStateField *field = vmsd->fields;
+ VMStateField *field;
if (vmsd->pre_save) {
vmsd->pre_save(opaque);
}
- while(field->name) {
+ for (field = vmsd->fields; field->name; field++) {
if (!field->field_exists ||
field->field_exists(opaque, vmsd->version_id)) {
void *base_addr = opaque + field->offset;
@@ -1800,7 +1799,6 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
}
}
}
- field++;
}
vmstate_subsection_save(f, vmsd, opaque);
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH 3/5] vmstate: Simplify field-skipping load/save logic
2013-10-14 16:45 [Qemu-devel] [PATCH 0/5] vmstate: Add max_version_id field Eduardo Habkost
2013-10-14 16:45 ` [Qemu-devel] [PATCH 1/5] savevm: Coding style line length fix Eduardo Habkost
2013-10-14 16:45 ` [Qemu-devel] [PATCH 2/5] vmstate: Replace while (...) with for (...) Eduardo Habkost
@ 2013-10-14 16:45 ` Eduardo Habkost
2013-10-15 8:01 ` Markus Armbruster
2013-10-15 11:32 ` Paolo Bonzini
2013-10-14 16:45 ` [Qemu-devel] [PATCH 4/5] vmstate: Use version_id when saving Eduardo Habkost
2013-10-14 16:45 ` [Qemu-devel] [PATCH 5/5] vmstate: Add max_version_id field to VMStateDescription Eduardo Habkost
4 siblings, 2 replies; 9+ messages in thread
From: Eduardo Habkost @ 2013-10-14 16:45 UTC (permalink / raw)
To: qemu-devel, Juan Quintela; +Cc: Paolo Bonzini, Markus Armbruster
This makes the code more readable, making each condition that makes a
field be skipped much more visible, and reduces one level of indentation
in the code.
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
savevm.c | 156 ++++++++++++++++++++++++++++++++-------------------------------
1 file changed, 80 insertions(+), 76 deletions(-)
diff --git a/savevm.c b/savevm.c
index 9562669..16276e7 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1694,50 +1694,52 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
return ret;
}
for (field = vmsd->fields; field->name; field++) {
- if ((field->field_exists &&
- field->field_exists(opaque, version_id)) ||
- (!field->field_exists &&
- field->version_id <= version_id)) {
- void *base_addr = opaque + field->offset;
- int i, n_elems = 1;
- int size = field->size;
-
- if (field->flags & VMS_VBUFFER) {
- size = *(int32_t *)(opaque+field->size_offset);
- if (field->flags & VMS_MULTIPLY) {
- size *= field->size;
- }
- }
- if (field->flags & VMS_ARRAY) {
- n_elems = field->num;
- } else if (field->flags & VMS_VARRAY_INT32) {
- n_elems = *(int32_t *)(opaque+field->num_offset);
- } else if (field->flags & VMS_VARRAY_UINT32) {
- n_elems = *(uint32_t *)(opaque+field->num_offset);
- } else if (field->flags & VMS_VARRAY_UINT16) {
- n_elems = *(uint16_t *)(opaque+field->num_offset);
- } else if (field->flags & VMS_VARRAY_UINT8) {
- n_elems = *(uint8_t *)(opaque+field->num_offset);
+ if (field->field_exists && !field->field_exists(opaque, version_id)) {
+ continue;
+ }
+ if (field->version_id > version_id) {
+ continue;
+ }
+
+ void *base_addr = opaque + field->offset;
+ int i, n_elems = 1;
+ int size = field->size;
+
+ if (field->flags & VMS_VBUFFER) {
+ size = *(int32_t *)(opaque+field->size_offset);
+ if (field->flags & VMS_MULTIPLY) {
+ size *= field->size;
}
- if (field->flags & VMS_POINTER) {
- base_addr = *(void **)base_addr + field->start;
+ }
+ if (field->flags & VMS_ARRAY) {
+ n_elems = field->num;
+ } else if (field->flags & VMS_VARRAY_INT32) {
+ n_elems = *(int32_t *)(opaque+field->num_offset);
+ } else if (field->flags & VMS_VARRAY_UINT32) {
+ n_elems = *(uint32_t *)(opaque+field->num_offset);
+ } else if (field->flags & VMS_VARRAY_UINT16) {
+ n_elems = *(uint16_t *)(opaque+field->num_offset);
+ } else if (field->flags & VMS_VARRAY_UINT8) {
+ n_elems = *(uint8_t *)(opaque+field->num_offset);
+ }
+ if (field->flags & VMS_POINTER) {
+ base_addr = *(void **)base_addr + field->start;
+ }
+ for (i = 0; i < n_elems; i++) {
+ void *addr = base_addr + size * i;
+
+ if (field->flags & VMS_ARRAY_OF_POINTER) {
+ addr = *(void **)addr;
}
- for (i = 0; i < n_elems; i++) {
- void *addr = base_addr + size * i;
-
- if (field->flags & VMS_ARRAY_OF_POINTER) {
- addr = *(void **)addr;
- }
- if (field->flags & VMS_STRUCT) {
- ret = vmstate_load_state(f, field->vmsd, addr,
- field->vmsd->version_id);
- } else {
- ret = field->info->get(f, addr, size);
+ if (field->flags & VMS_STRUCT) {
+ ret = vmstate_load_state(f, field->vmsd, addr,
+ field->vmsd->version_id);
+ } else {
+ ret = field->info->get(f, addr, size);
- }
- if (ret < 0) {
- return ret;
- }
+ }
+ if (ret < 0) {
+ return ret;
}
}
}
@@ -1760,43 +1762,45 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
vmsd->pre_save(opaque);
}
for (field = vmsd->fields; field->name; field++) {
- if (!field->field_exists ||
- field->field_exists(opaque, vmsd->version_id)) {
- void *base_addr = opaque + field->offset;
- int i, n_elems = 1;
- int size = field->size;
-
- if (field->flags & VMS_VBUFFER) {
- size = *(int32_t *)(opaque+field->size_offset);
- if (field->flags & VMS_MULTIPLY) {
- size *= field->size;
- }
- }
- if (field->flags & VMS_ARRAY) {
- n_elems = field->num;
- } else if (field->flags & VMS_VARRAY_INT32) {
- n_elems = *(int32_t *)(opaque+field->num_offset);
- } else if (field->flags & VMS_VARRAY_UINT32) {
- n_elems = *(uint32_t *)(opaque+field->num_offset);
- } else if (field->flags & VMS_VARRAY_UINT16) {
- n_elems = *(uint16_t *)(opaque+field->num_offset);
- } else if (field->flags & VMS_VARRAY_UINT8) {
- n_elems = *(uint8_t *)(opaque+field->num_offset);
+ if (field->field_exists &&
+ !field->field_exists(opaque, vmsd->version_id)) {
+ continue;
+ }
+
+ void *base_addr = opaque + field->offset;
+ int i, n_elems = 1;
+ int size = field->size;
+
+ if (field->flags & VMS_VBUFFER) {
+ size = *(int32_t *)(opaque+field->size_offset);
+ if (field->flags & VMS_MULTIPLY) {
+ size *= field->size;
}
- if (field->flags & VMS_POINTER) {
- base_addr = *(void **)base_addr + field->start;
+ }
+ if (field->flags & VMS_ARRAY) {
+ n_elems = field->num;
+ } else if (field->flags & VMS_VARRAY_INT32) {
+ n_elems = *(int32_t *)(opaque+field->num_offset);
+ } else if (field->flags & VMS_VARRAY_UINT32) {
+ n_elems = *(uint32_t *)(opaque+field->num_offset);
+ } else if (field->flags & VMS_VARRAY_UINT16) {
+ n_elems = *(uint16_t *)(opaque+field->num_offset);
+ } else if (field->flags & VMS_VARRAY_UINT8) {
+ n_elems = *(uint8_t *)(opaque+field->num_offset);
+ }
+ if (field->flags & VMS_POINTER) {
+ base_addr = *(void **)base_addr + field->start;
+ }
+ for (i = 0; i < n_elems; i++) {
+ void *addr = base_addr + size * i;
+
+ if (field->flags & VMS_ARRAY_OF_POINTER) {
+ addr = *(void **)addr;
}
- for (i = 0; i < n_elems; i++) {
- void *addr = base_addr + size * i;
-
- if (field->flags & VMS_ARRAY_OF_POINTER) {
- addr = *(void **)addr;
- }
- if (field->flags & VMS_STRUCT) {
- vmstate_save_state(f, field->vmsd, addr);
- } else {
- field->info->put(f, addr, size);
- }
+ if (field->flags & VMS_STRUCT) {
+ vmstate_save_state(f, field->vmsd, addr);
+ } else {
+ field->info->put(f, addr, size);
}
}
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH 4/5] vmstate: Use version_id when saving
2013-10-14 16:45 [Qemu-devel] [PATCH 0/5] vmstate: Add max_version_id field Eduardo Habkost
` (2 preceding siblings ...)
2013-10-14 16:45 ` [Qemu-devel] [PATCH 3/5] vmstate: Simplify field-skipping load/save logic Eduardo Habkost
@ 2013-10-14 16:45 ` Eduardo Habkost
2013-10-14 16:45 ` [Qemu-devel] [PATCH 5/5] vmstate: Add max_version_id field to VMStateDescription Eduardo Habkost
4 siblings, 0 replies; 9+ messages in thread
From: Eduardo Habkost @ 2013-10-14 16:45 UTC (permalink / raw)
To: qemu-devel, Juan Quintela; +Cc: Paolo Bonzini, Markus Armbruster
This will allow fields to have version_id > vmsd->version_id, to allow
us to support loading data with higher version_id.
This patch alone is not useful by itself, but it will be useful when
introducing the max_version_id field to VMStateDescription.
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
savevm.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/savevm.c b/savevm.c
index 16276e7..87773ad 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1766,6 +1766,9 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
!field->field_exists(opaque, vmsd->version_id)) {
continue;
}
+ if (field->version_id > vmsd->version_id) {
+ continue;
+ }
void *base_addr = opaque + field->offset;
int i, n_elems = 1;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH 5/5] vmstate: Add max_version_id field to VMStateDescription
2013-10-14 16:45 [Qemu-devel] [PATCH 0/5] vmstate: Add max_version_id field Eduardo Habkost
` (3 preceding siblings ...)
2013-10-14 16:45 ` [Qemu-devel] [PATCH 4/5] vmstate: Use version_id when saving Eduardo Habkost
@ 2013-10-14 16:45 ` Eduardo Habkost
4 siblings, 0 replies; 9+ messages in thread
From: Eduardo Habkost @ 2013-10-14 16:45 UTC (permalink / raw)
To: qemu-devel, Juan Quintela; +Cc: Paolo Bonzini, Markus Armbruster
This will allow us to load data that has a high version_id, while using
a lower version_id when saving.
Useful in case we need to revert vmstate changes but keep compatibility
with QEMU versions that had a higher version_id.
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
include/migration/vmstate.h | 1 +
savevm.c | 2 +-
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 9d09e60..cc9dbb4 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -126,6 +126,7 @@ struct VMStateDescription {
const char *name;
int unmigratable;
int version_id;
+ int max_version_id;
int minimum_version_id;
int minimum_version_id_old;
LoadStateHandler *load_state_old;
diff --git a/savevm.c b/savevm.c
index 87773ad..89d20d0 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1679,7 +1679,7 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
VMStateField *field;
int ret;
- if (version_id > vmsd->version_id) {
+ if (version_id > MAX(vmsd->version_id, vmsd->max_version_id)) {
return -EINVAL;
}
if (version_id < vmsd->minimum_version_id_old) {
--
1.8.3.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 3/5] vmstate: Simplify field-skipping load/save logic
2013-10-14 16:45 ` [Qemu-devel] [PATCH 3/5] vmstate: Simplify field-skipping load/save logic Eduardo Habkost
@ 2013-10-15 8:01 ` Markus Armbruster
2013-10-15 12:12 ` Eduardo Habkost
2013-10-15 11:32 ` Paolo Bonzini
1 sibling, 1 reply; 9+ messages in thread
From: Markus Armbruster @ 2013-10-15 8:01 UTC (permalink / raw)
To: Eduardo Habkost; +Cc: Paolo Bonzini, qemu-devel, Juan Quintela
Eduardo Habkost <ehabkost@redhat.com> writes:
> This makes the code more readable, making each condition that makes a
> field be skipped much more visible, and reduces one level of indentation
> in the code.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> savevm.c | 156 ++++++++++++++++++++++++++++++++-------------------------------
> 1 file changed, 80 insertions(+), 76 deletions(-)
>
> diff --git a/savevm.c b/savevm.c
> index 9562669..16276e7 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -1694,50 +1694,52 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
> return ret;
> }
> for (field = vmsd->fields; field->name; field++) {
> - if ((field->field_exists &&
> - field->field_exists(opaque, version_id)) ||
> - (!field->field_exists &&
> - field->version_id <= version_id)) {
> - void *base_addr = opaque + field->offset;
> - int i, n_elems = 1;
> - int size = field->size;
> -
> - if (field->flags & VMS_VBUFFER) {
> - size = *(int32_t *)(opaque+field->size_offset);
> - if (field->flags & VMS_MULTIPLY) {
> - size *= field->size;
> - }
> - }
> - if (field->flags & VMS_ARRAY) {
> - n_elems = field->num;
> - } else if (field->flags & VMS_VARRAY_INT32) {
> - n_elems = *(int32_t *)(opaque+field->num_offset);
> - } else if (field->flags & VMS_VARRAY_UINT32) {
> - n_elems = *(uint32_t *)(opaque+field->num_offset);
> - } else if (field->flags & VMS_VARRAY_UINT16) {
> - n_elems = *(uint16_t *)(opaque+field->num_offset);
> - } else if (field->flags & VMS_VARRAY_UINT8) {
> - n_elems = *(uint8_t *)(opaque+field->num_offset);
> + if (field->field_exists && !field->field_exists(opaque, version_id)) {
> + continue;
> + }
> + if (field->version_id > version_id) {
> + continue;
> + }
> +
> + void *base_addr = opaque + field->offset;
> + int i, n_elems = 1;
> + int size = field->size;
> +
> + if (field->flags & VMS_VBUFFER) {
> + size = *(int32_t *)(opaque+field->size_offset);
> + if (field->flags & VMS_MULTIPLY) {
> + size *= field->size;
> }
> - if (field->flags & VMS_POINTER) {
> - base_addr = *(void **)base_addr + field->start;
> + }
> + if (field->flags & VMS_ARRAY) {
> + n_elems = field->num;
> + } else if (field->flags & VMS_VARRAY_INT32) {
> + n_elems = *(int32_t *)(opaque+field->num_offset);
> + } else if (field->flags & VMS_VARRAY_UINT32) {
> + n_elems = *(uint32_t *)(opaque+field->num_offset);
> + } else if (field->flags & VMS_VARRAY_UINT16) {
> + n_elems = *(uint16_t *)(opaque+field->num_offset);
> + } else if (field->flags & VMS_VARRAY_UINT8) {
> + n_elems = *(uint8_t *)(opaque+field->num_offset);
> + }
> + if (field->flags & VMS_POINTER) {
> + base_addr = *(void **)base_addr + field->start;
> + }
> + for (i = 0; i < n_elems; i++) {
> + void *addr = base_addr + size * i;
> +
> + if (field->flags & VMS_ARRAY_OF_POINTER) {
> + addr = *(void **)addr;
> }
> - for (i = 0; i < n_elems; i++) {
> - void *addr = base_addr + size * i;
> -
> - if (field->flags & VMS_ARRAY_OF_POINTER) {
> - addr = *(void **)addr;
> - }
> - if (field->flags & VMS_STRUCT) {
> - ret = vmstate_load_state(f, field->vmsd, addr,
> - field->vmsd->version_id);
> - } else {
> - ret = field->info->get(f, addr, size);
> + if (field->flags & VMS_STRUCT) {
> + ret = vmstate_load_state(f, field->vmsd, addr,
> + field->vmsd->version_id);
> + } else {
> + ret = field->info->get(f, addr, size);
>
> - }
> - if (ret < 0) {
> - return ret;
> - }
> + }
> + if (ret < 0) {
> + return ret;
> }
> }
> }
With whitespace change ignored:
@@ -1694,10 +1694,13 @@
return ret;
}
for (field = vmsd->fields; field->name; field++) {
- if ((field->field_exists &&
- field->field_exists(opaque, version_id)) ||
- (!field->field_exists &&
- field->version_id <= version_id)) {
+ if (field->field_exists && !field->field_exists(opaque, version_id)) {
+ continue;
+ }
+ if (field->version_id > version_id) {
+ continue;
+ }
+
void *base_addr = opaque + field->offset;
int i, n_elems = 1;
int size = field->size;
@@ -1740,4 +1743,3 @@
}
}
}
- }
Consider the case
field->field_exists
&& field->field_exists(opaque, version_id)
&& field->version_id > version_id?
Old code:
(field->field_exists && field->field_exists(opaque, version_id))
yields true
Body executed.
New code:
First field->field_exists && !field->field_exists(opaque, version_id)
yields false, no continue
Then field->version_id > version_id yields true, continue
Body *not* executed.
Why is this okay?
> @@ -1760,43 +1762,45 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
> vmsd->pre_save(opaque);
> }
> for (field = vmsd->fields; field->name; field++) {
> - if (!field->field_exists ||
> - field->field_exists(opaque, vmsd->version_id)) {
> - void *base_addr = opaque + field->offset;
> - int i, n_elems = 1;
> - int size = field->size;
> -
> - if (field->flags & VMS_VBUFFER) {
> - size = *(int32_t *)(opaque+field->size_offset);
> - if (field->flags & VMS_MULTIPLY) {
> - size *= field->size;
> - }
> - }
> - if (field->flags & VMS_ARRAY) {
> - n_elems = field->num;
> - } else if (field->flags & VMS_VARRAY_INT32) {
> - n_elems = *(int32_t *)(opaque+field->num_offset);
> - } else if (field->flags & VMS_VARRAY_UINT32) {
> - n_elems = *(uint32_t *)(opaque+field->num_offset);
> - } else if (field->flags & VMS_VARRAY_UINT16) {
> - n_elems = *(uint16_t *)(opaque+field->num_offset);
> - } else if (field->flags & VMS_VARRAY_UINT8) {
> - n_elems = *(uint8_t *)(opaque+field->num_offset);
> + if (field->field_exists &&
> + !field->field_exists(opaque, vmsd->version_id)) {
> + continue;
> + }
> +
> + void *base_addr = opaque + field->offset;
> + int i, n_elems = 1;
> + int size = field->size;
> +
> + if (field->flags & VMS_VBUFFER) {
> + size = *(int32_t *)(opaque+field->size_offset);
> + if (field->flags & VMS_MULTIPLY) {
> + size *= field->size;
> }
> - if (field->flags & VMS_POINTER) {
> - base_addr = *(void **)base_addr + field->start;
> + }
> + if (field->flags & VMS_ARRAY) {
> + n_elems = field->num;
> + } else if (field->flags & VMS_VARRAY_INT32) {
> + n_elems = *(int32_t *)(opaque+field->num_offset);
> + } else if (field->flags & VMS_VARRAY_UINT32) {
> + n_elems = *(uint32_t *)(opaque+field->num_offset);
> + } else if (field->flags & VMS_VARRAY_UINT16) {
> + n_elems = *(uint16_t *)(opaque+field->num_offset);
> + } else if (field->flags & VMS_VARRAY_UINT8) {
> + n_elems = *(uint8_t *)(opaque+field->num_offset);
> + }
> + if (field->flags & VMS_POINTER) {
> + base_addr = *(void **)base_addr + field->start;
> + }
> + for (i = 0; i < n_elems; i++) {
> + void *addr = base_addr + size * i;
> +
> + if (field->flags & VMS_ARRAY_OF_POINTER) {
> + addr = *(void **)addr;
> }
> - for (i = 0; i < n_elems; i++) {
> - void *addr = base_addr + size * i;
> -
> - if (field->flags & VMS_ARRAY_OF_POINTER) {
> - addr = *(void **)addr;
> - }
> - if (field->flags & VMS_STRUCT) {
> - vmstate_save_state(f, field->vmsd, addr);
> - } else {
> - field->info->put(f, addr, size);
> - }
> + if (field->flags & VMS_STRUCT) {
> + vmstate_save_state(f, field->vmsd, addr);
> + } else {
> + field->info->put(f, addr, size);
> }
> }
> }
With whitespace change ignored:
vmsd->pre_save(opaque);
}
for (field = vmsd->fields; field->name; field++) {
- if (!field->field_exists ||
- field->field_exists(opaque, vmsd->version_id)) {
+ if (field->field_exists &&
+ !field->field_exists(opaque, vmsd->version_id)) {
+ continue;
+ }
+
void *base_addr = opaque + field->offset;
int i, n_elems = 1;
int size = field->size;
@@ -1799,4 +1802,3 @@
}
}
}
- }
Okay.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 3/5] vmstate: Simplify field-skipping load/save logic
2013-10-14 16:45 ` [Qemu-devel] [PATCH 3/5] vmstate: Simplify field-skipping load/save logic Eduardo Habkost
2013-10-15 8:01 ` Markus Armbruster
@ 2013-10-15 11:32 ` Paolo Bonzini
1 sibling, 0 replies; 9+ messages in thread
From: Paolo Bonzini @ 2013-10-15 11:32 UTC (permalink / raw)
To: Eduardo Habkost; +Cc: Markus Armbruster, qemu-devel, Juan Quintela
Il 14/10/2013 18:45, Eduardo Habkost ha scritto:
> + if (field->field_exists && !field->field_exists(opaque, version_id)) {
> + continue;
> + }
> + if (field->version_id > version_id) {
> + continue;
> + }
What Markus observed...
I think the change is fine because we currently never have field_exists
and version_id set for the same field.
However, I suggest to move the change to a separate patch, and swap the
two ifs in this one.
Paolo
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 3/5] vmstate: Simplify field-skipping load/save logic
2013-10-15 8:01 ` Markus Armbruster
@ 2013-10-15 12:12 ` Eduardo Habkost
0 siblings, 0 replies; 9+ messages in thread
From: Eduardo Habkost @ 2013-10-15 12:12 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Paolo Bonzini, qemu-devel, Juan Quintela
On Tue, Oct 15, 2013 at 10:01:12AM +0200, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
>
> > This makes the code more readable, making each condition that makes a
> > field be skipped much more visible, and reduces one level of indentation
> > in the code.
> >
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> > savevm.c | 156 ++++++++++++++++++++++++++++++++-------------------------------
> > 1 file changed, 80 insertions(+), 76 deletions(-)
> >
> > diff --git a/savevm.c b/savevm.c
> > index 9562669..16276e7 100644
> > --- a/savevm.c
> > +++ b/savevm.c
> > @@ -1694,50 +1694,52 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
> > return ret;
> > }
> > for (field = vmsd->fields; field->name; field++) {
> > - if ((field->field_exists &&
> > - field->field_exists(opaque, version_id)) ||
> > - (!field->field_exists &&
> > - field->version_id <= version_id)) {
> > - void *base_addr = opaque + field->offset;
> > - int i, n_elems = 1;
> > - int size = field->size;
> > -
> > - if (field->flags & VMS_VBUFFER) {
> > - size = *(int32_t *)(opaque+field->size_offset);
> > - if (field->flags & VMS_MULTIPLY) {
> > - size *= field->size;
> > - }
> > - }
> > - if (field->flags & VMS_ARRAY) {
> > - n_elems = field->num;
> > - } else if (field->flags & VMS_VARRAY_INT32) {
> > - n_elems = *(int32_t *)(opaque+field->num_offset);
> > - } else if (field->flags & VMS_VARRAY_UINT32) {
> > - n_elems = *(uint32_t *)(opaque+field->num_offset);
> > - } else if (field->flags & VMS_VARRAY_UINT16) {
> > - n_elems = *(uint16_t *)(opaque+field->num_offset);
> > - } else if (field->flags & VMS_VARRAY_UINT8) {
> > - n_elems = *(uint8_t *)(opaque+field->num_offset);
> > + if (field->field_exists && !field->field_exists(opaque, version_id)) {
> > + continue;
> > + }
> > + if (field->version_id > version_id) {
> > + continue;
> > + }
> > +
> > + void *base_addr = opaque + field->offset;
> > + int i, n_elems = 1;
> > + int size = field->size;
> > +
> > + if (field->flags & VMS_VBUFFER) {
> > + size = *(int32_t *)(opaque+field->size_offset);
> > + if (field->flags & VMS_MULTIPLY) {
> > + size *= field->size;
> > }
> > - if (field->flags & VMS_POINTER) {
> > - base_addr = *(void **)base_addr + field->start;
> > + }
> > + if (field->flags & VMS_ARRAY) {
> > + n_elems = field->num;
> > + } else if (field->flags & VMS_VARRAY_INT32) {
> > + n_elems = *(int32_t *)(opaque+field->num_offset);
> > + } else if (field->flags & VMS_VARRAY_UINT32) {
> > + n_elems = *(uint32_t *)(opaque+field->num_offset);
> > + } else if (field->flags & VMS_VARRAY_UINT16) {
> > + n_elems = *(uint16_t *)(opaque+field->num_offset);
> > + } else if (field->flags & VMS_VARRAY_UINT8) {
> > + n_elems = *(uint8_t *)(opaque+field->num_offset);
> > + }
> > + if (field->flags & VMS_POINTER) {
> > + base_addr = *(void **)base_addr + field->start;
> > + }
> > + for (i = 0; i < n_elems; i++) {
> > + void *addr = base_addr + size * i;
> > +
> > + if (field->flags & VMS_ARRAY_OF_POINTER) {
> > + addr = *(void **)addr;
> > }
> > - for (i = 0; i < n_elems; i++) {
> > - void *addr = base_addr + size * i;
> > -
> > - if (field->flags & VMS_ARRAY_OF_POINTER) {
> > - addr = *(void **)addr;
> > - }
> > - if (field->flags & VMS_STRUCT) {
> > - ret = vmstate_load_state(f, field->vmsd, addr,
> > - field->vmsd->version_id);
> > - } else {
> > - ret = field->info->get(f, addr, size);
> > + if (field->flags & VMS_STRUCT) {
> > + ret = vmstate_load_state(f, field->vmsd, addr,
> > + field->vmsd->version_id);
> > + } else {
> > + ret = field->info->get(f, addr, size);
> >
> > - }
> > - if (ret < 0) {
> > - return ret;
> > - }
> > + }
> > + if (ret < 0) {
> > + return ret;
> > }
> > }
> > }
>
> With whitespace change ignored:
>
> @@ -1694,10 +1694,13 @@
> return ret;
> }
> for (field = vmsd->fields; field->name; field++) {
> - if ((field->field_exists &&
> - field->field_exists(opaque, version_id)) ||
> - (!field->field_exists &&
> - field->version_id <= version_id)) {
> + if (field->field_exists && !field->field_exists(opaque, version_id)) {
> + continue;
> + }
> + if (field->version_id > version_id) {
> + continue;
> + }
> +
> void *base_addr = opaque + field->offset;
> int i, n_elems = 1;
> int size = field->size;
> @@ -1740,4 +1743,3 @@
> }
> }
> }
> - }
>
> Consider the case
>
> field->field_exists
> && field->field_exists(opaque, version_id)
> && field->version_id > version_id?
>
> Old code:
>
> (field->field_exists && field->field_exists(opaque, version_id))
> yields true
>
> Body executed.
>
> New code:
>
> First field->field_exists && !field->field_exists(opaque, version_id)
> yields false, no continue
>
> Then field->version_id > version_id yields true, continue
>
> Body *not* executed.
>
> Why is this okay?
Oops! I read and re-read the old and new code, and it looks like I
jumped too far when trying to simplify the original code. Thanks for
catching!
Even if we decided the new behavior is OK, I would prefer to change the
rules _after_ refactoring the code. I will rewrite the patch to keep
exactly the same behavior.
>
> > @@ -1760,43 +1762,45 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
> > vmsd->pre_save(opaque);
> > }
> > for (field = vmsd->fields; field->name; field++) {
> > - if (!field->field_exists ||
> > - field->field_exists(opaque, vmsd->version_id)) {
> > - void *base_addr = opaque + field->offset;
> > - int i, n_elems = 1;
> > - int size = field->size;
> > -
> > - if (field->flags & VMS_VBUFFER) {
> > - size = *(int32_t *)(opaque+field->size_offset);
> > - if (field->flags & VMS_MULTIPLY) {
> > - size *= field->size;
> > - }
> > - }
> > - if (field->flags & VMS_ARRAY) {
> > - n_elems = field->num;
> > - } else if (field->flags & VMS_VARRAY_INT32) {
> > - n_elems = *(int32_t *)(opaque+field->num_offset);
> > - } else if (field->flags & VMS_VARRAY_UINT32) {
> > - n_elems = *(uint32_t *)(opaque+field->num_offset);
> > - } else if (field->flags & VMS_VARRAY_UINT16) {
> > - n_elems = *(uint16_t *)(opaque+field->num_offset);
> > - } else if (field->flags & VMS_VARRAY_UINT8) {
> > - n_elems = *(uint8_t *)(opaque+field->num_offset);
> > + if (field->field_exists &&
> > + !field->field_exists(opaque, vmsd->version_id)) {
> > + continue;
> > + }
> > +
> > + void *base_addr = opaque + field->offset;
> > + int i, n_elems = 1;
> > + int size = field->size;
> > +
> > + if (field->flags & VMS_VBUFFER) {
> > + size = *(int32_t *)(opaque+field->size_offset);
> > + if (field->flags & VMS_MULTIPLY) {
> > + size *= field->size;
> > }
> > - if (field->flags & VMS_POINTER) {
> > - base_addr = *(void **)base_addr + field->start;
> > + }
> > + if (field->flags & VMS_ARRAY) {
> > + n_elems = field->num;
> > + } else if (field->flags & VMS_VARRAY_INT32) {
> > + n_elems = *(int32_t *)(opaque+field->num_offset);
> > + } else if (field->flags & VMS_VARRAY_UINT32) {
> > + n_elems = *(uint32_t *)(opaque+field->num_offset);
> > + } else if (field->flags & VMS_VARRAY_UINT16) {
> > + n_elems = *(uint16_t *)(opaque+field->num_offset);
> > + } else if (field->flags & VMS_VARRAY_UINT8) {
> > + n_elems = *(uint8_t *)(opaque+field->num_offset);
> > + }
> > + if (field->flags & VMS_POINTER) {
> > + base_addr = *(void **)base_addr + field->start;
> > + }
> > + for (i = 0; i < n_elems; i++) {
> > + void *addr = base_addr + size * i;
> > +
> > + if (field->flags & VMS_ARRAY_OF_POINTER) {
> > + addr = *(void **)addr;
> > }
> > - for (i = 0; i < n_elems; i++) {
> > - void *addr = base_addr + size * i;
> > -
> > - if (field->flags & VMS_ARRAY_OF_POINTER) {
> > - addr = *(void **)addr;
> > - }
> > - if (field->flags & VMS_STRUCT) {
> > - vmstate_save_state(f, field->vmsd, addr);
> > - } else {
> > - field->info->put(f, addr, size);
> > - }
> > + if (field->flags & VMS_STRUCT) {
> > + vmstate_save_state(f, field->vmsd, addr);
> > + } else {
> > + field->info->put(f, addr, size);
> > }
> > }
> > }
>
> With whitespace change ignored:
>
> vmsd->pre_save(opaque);
> }
> for (field = vmsd->fields; field->name; field++) {
> - if (!field->field_exists ||
> - field->field_exists(opaque, vmsd->version_id)) {
> + if (field->field_exists &&
> + !field->field_exists(opaque, vmsd->version_id)) {
> + continue;
> + }
> +
> void *base_addr = opaque + field->offset;
> int i, n_elems = 1;
> int size = field->size;
> @@ -1799,4 +1802,3 @@
> }
> }
> }
> - }
>
> Okay.
--
Eduardo
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2013-10-15 12:13 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-10-14 16:45 [Qemu-devel] [PATCH 0/5] vmstate: Add max_version_id field Eduardo Habkost
2013-10-14 16:45 ` [Qemu-devel] [PATCH 1/5] savevm: Coding style line length fix Eduardo Habkost
2013-10-14 16:45 ` [Qemu-devel] [PATCH 2/5] vmstate: Replace while (...) with for (...) Eduardo Habkost
2013-10-14 16:45 ` [Qemu-devel] [PATCH 3/5] vmstate: Simplify field-skipping load/save logic Eduardo Habkost
2013-10-15 8:01 ` Markus Armbruster
2013-10-15 12:12 ` Eduardo Habkost
2013-10-15 11:32 ` Paolo Bonzini
2013-10-14 16:45 ` [Qemu-devel] [PATCH 4/5] vmstate: Use version_id when saving Eduardo Habkost
2013-10-14 16:45 ` [Qemu-devel] [PATCH 5/5] vmstate: Add max_version_id field to VMStateDescription Eduardo Habkost
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).