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