Re: [Qemu-devel] [RFC PATCH v2 5/8] migration/vmstate: renames in (load|save)_state

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
> The vmstate_(load|save)_state start out with an a void *opaque pointing
> to some struct, and manipulate one or more elements of one field within
> that struct.
> 
> First the field within the struct is pinpointed as opaque + offset, then
> if this is a pointer the pointer is dereferenced to obtain a pointer to
> the first element of the vmstate field. Pointers to further elements if
> any are calculated as first_element + i * element_size (where i is the
> zero based index of the element in question).
> 
> Currently base_addr and addr is used as a variable name for the pointer
> to the first element and the pointer to the current element being
> processed. This is suboptimal because base_addr is somewhat
> counter-intuitive (because obtained as base + offset) and both base_addr
> and addr not very descriptive (that we have a pointer should be clear
> from the fact that it is declared as a pointer).
> 
> Let make things easier to understand by renaming base_addr to first_elem
> and addr to curr_elem. This has the additional benefit of harmonizing
> with other names within the scope (n_elems, vmstate_n_elems).
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> --
> 
> This patch roots in the difficulties I faced when trying to figure out
> the code in question. In the meanwhile I'm quite fine with the current
> names because I have it already figured out (after a couple of months
> not looking on this code however, who knows). Thus my main motivation
> is making things easier for the next new guy, but if the old guys say
> not worth it I can very well accept that too.
> ---
>  migration/vmstate.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index 8767e40..a86fb7e 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -108,21 +108,21 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>               field->field_exists(opaque, version_id)) ||
>              (!field->field_exists &&
>               field->version_id <= version_id)) {
> -            void *base_addr = vmstate_base_addr(opaque, field, true);
> +            void *first_elem = vmstate_base_addr(opaque, field, true);
>              int i, n_elems = vmstate_n_elems(opaque, field);
>              int size = vmstate_size(opaque, field);
>  
>              for (i = 0; i < n_elems; i++) {
> -                void *addr = base_addr + size * i;
> +                void *curr_elem = first_elem + size * i;
>  
>                  if (field->flags & VMS_ARRAY_OF_POINTER) {
> -                    addr = *(void **)addr;
> +                    curr_elem = *(void **)curr_elem;
>                  }
>                  if (field->flags & VMS_STRUCT) {
> -                    ret = vmstate_load_state(f, field->vmsd, addr,
> +                    ret = vmstate_load_state(f, field->vmsd, curr_elem,
>                                               field->vmsd->version_id);
>                  } else {
> -                    ret = field->info->get(f, addr, size);
> +                    ret = field->info->get(f, curr_elem, size);
>  
>                  }
>                  if (ret >= 0) {
> @@ -310,25 +310,25 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
>      while (field->name) {
>          if (!field->field_exists ||
>              field->field_exists(opaque, vmsd->version_id)) {
> -            void *base_addr = vmstate_base_addr(opaque, field, false);
> +            void *first_elem = vmstate_base_addr(opaque, field, false);
>              int i, n_elems = vmstate_n_elems(opaque, field);
>              int size = vmstate_size(opaque, field);
>              int64_t old_offset, written_bytes;
>              QJSON *vmdesc_loop = vmdesc;
>  
>              for (i = 0; i < n_elems; i++) {
> -                void *addr = base_addr + size * i;
> +                void *curr_elem = first_elem + size * i;
>  
>                  vmsd_desc_field_start(vmsd, vmdesc_loop, field, i, n_elems);
>                  old_offset = qemu_ftell_fast(f);
> -
>                  if (field->flags & VMS_ARRAY_OF_POINTER) {
> -                    addr = *(void **)addr;
> +                    assert(curr_elem);
> +                    curr_elem = *(void **)curr_elem;
>                  }
>                  if (field->flags & VMS_STRUCT) {
> -                    vmstate_save_state(f, field->vmsd, addr, vmdesc_loop);
> +                    vmstate_save_state(f, field->vmsd, curr_elem, vmdesc_loop);
>                  } else {
> -                    field->info->put(f, addr, size);
> +                    field->info->put(f, curr_elem, size);
>                  }
>  
>                  written_bytes = qemu_ftell_fast(f) - old_offset;
> -- 
> 2.8.4
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK