| Message ID | 20161108095603.72301-6-pasic@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
* 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
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;
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> -- 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(-)