Eduardo Habkost <ehabk...@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 <ehabk...@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.
