qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 3/5] vmstate: Simplify field-skipping load/save


From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH 3/5] vmstate: Simplify field-skipping load/save logic
Date: Tue, 15 Oct 2013 10:01:12 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.2 (gnu/linux)

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]