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

[Eduardo Habkost]

On Tue, Oct 15, 2013 at 10:01:12AM +0200, Markus Armbruster wrote:
> 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?

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