On 02/21/2017 01:07 PM, Dr. David Alan Gilbert wrote: > * Halil Pasic (pa...@linux.vnet.ibm.com) 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
s/VMS_VBUFFER| VMS_MULTIPLY/VMS_VBUFFER|VMS_MULTIPLY|VMS_ALLOC/ >> 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). Adding |VMS_ALLOC to avoid confusion. I was hoping it's clear form the context that we are talking about scenarios where allocation is relevant. > I'm not sure I see what the error is that you fixed. I'm referring to number of bytes allocated. It was vmstate_n_elems(opaque, field) * field->size and became vmstate_n_elems(opaque, field) * vmstate_size(opaque, field) vmstate_size does extra handling for VMS_VBUFFER and VMS_MULTIPLY. > > However, I'm ok with it: > > Reviewed-by: Dr. David Alan Gilbert <dgilb...@redhat.com> > Cheers, Halil >> 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 <pa...@linux.vnet.ibm.com> >> --- >> 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 / dgilb...@redhat.com / Manchester, UK >