* [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.