* Halil Pasic ([email protected]) wrote: > Currently vmstate_base_addr does several things: it pinpoints the field > within the struct, possibly allocates memory and possibly does the first > pointer dereference. Obviously allocation is needed only for load. > > Let us split up the functionality in vmstate_base_addr and move the > address manipulations (that is everything but the allocation logic) to > load and save so it becomes more obvious what is actually going on. Like > this all the address calculations (and the handling of the flags > controlling these) is in one place and the sequence is more obvious. > > The newly introduced function vmstate_handle_alloc also fixes the > allocation for the unused VMS_VBUFFER| VMS_MULTIPLY scenario and is > substantially simpler than the original vmstate_base_addr.
Note that VMSTATE_VBUFFER_MULTIPLY does use VMS_VBUFFER|VMS_MULTIPLY - but not with alloc (and I started using VMSTATE_VBUFFER_MULTIPLY in a recent patch that went in). I'm not sure I see what the error is that you fixed. However, I'm ok with it: Reviewed-by: Dr. David Alan Gilbert <[email protected]> > In load and save some asserts are added so it's easier to debug > situations where we would end up with a null pointer dereference. > > Signed-off-by: Halil Pasic <[email protected]> > --- > migration/vmstate.c | 39 +++++++++++++++++---------------------- > 1 file changed, 17 insertions(+), 22 deletions(-) > > diff --git a/migration/vmstate.c b/migration/vmstate.c > index 36efa80..836a7a4 100644 > --- a/migration/vmstate.c > +++ b/migration/vmstate.c > @@ -52,29 +52,15 @@ static int vmstate_size(void *opaque, VMStateField *field) > return size; > } > > -static void *vmstate_base_addr(void *opaque, VMStateField *field, bool alloc) > +static void vmstate_handle_alloc(void *ptr, VMStateField *field, void > *opaque) > { > - void *base_addr = opaque + field->offset; > - > - if (field->flags & VMS_POINTER) { > - if (alloc && (field->flags & VMS_ALLOC)) { > - gsize size = 0; > - if (field->flags & VMS_VBUFFER) { > - size = vmstate_size(opaque, field); > - } else { > - int n_elems = vmstate_n_elems(opaque, field); > - if (n_elems) { > - size = n_elems * field->size; > - } > - } > - if (size) { > - *(void **)base_addr = g_malloc(size); > - } > + if (field->flags & VMS_POINTER && field->flags & VMS_ALLOC) { > + gsize size = vmstate_size(opaque, field); > + size *= vmstate_n_elems(opaque, field); > + if (size) { > + *(void **)ptr = g_malloc(size); > } > - base_addr = *(void **)base_addr; > } > - > - return base_addr; > } > > int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd, > @@ -116,10 +102,15 @@ int vmstate_load_state(QEMUFile *f, const > VMStateDescription *vmsd, > field->field_exists(opaque, version_id)) || > (!field->field_exists && > field->version_id <= version_id)) { > - void *first_elem = vmstate_base_addr(opaque, field, true); > + void *first_elem = opaque + field->offset; > int i, n_elems = vmstate_n_elems(opaque, field); > int size = vmstate_size(opaque, field); > > + vmstate_handle_alloc(first_elem, field, opaque); > + if (field->flags & VMS_POINTER) { > + first_elem = *(void **)first_elem; > + assert(first_elem || !n_elems); > + } > for (i = 0; i < n_elems; i++) { > void *curr_elem = first_elem + size * i; > > @@ -321,13 +312,17 @@ void vmstate_save_state(QEMUFile *f, const > VMStateDescription *vmsd, > while (field->name) { > if (!field->field_exists || > field->field_exists(opaque, vmsd->version_id)) { > - void *first_elem = vmstate_base_addr(opaque, field, false); > + void *first_elem = opaque + field->offset; > 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; > > trace_vmstate_save_state_loop(vmsd->name, field->name, n_elems); > + if (field->flags & VMS_POINTER) { > + first_elem = *(void **)first_elem; > + assert(first_elem || !n_elems); > + } > for (i = 0; i < n_elems; i++) { > void *curr_elem = first_elem + size * i; > > -- > 2.8.4 > -- Dr. David Alan Gilbert / [email protected] / Manchester, UK
