| Message ID | 20161108095603.72301-7-pasic@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
* Halil Pasic (pasic@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 > substantially simpler than the original vmstate_base_addr. > > 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 <pasic@linux.vnet.ibm.com> > --- > migration/vmstate.c | 41 ++++++++++++++++++----------------------- > 1 file changed, 18 insertions(+), 23 deletions(-) > > diff --git a/migration/vmstate.c b/migration/vmstate.c > index a86fb7e..10a7645 100644 > --- a/migration/vmstate.c > +++ b/migration/vmstate.c > @@ -50,29 +50,15 @@ static int vmstate_size(void *opaque, VMStateField *field) > return size; > } > > -static void *vmstate_base_addr(void *opaque, VMStateField *field, bool alloc) > -{ > - 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) { OK, that test isn't really needed because vmstate_size already has it. > - 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); > - } > +static void vmstate_handle_alloc(void *ptr, VMStateField *field, void *opaque) > +{ > + 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, > @@ -108,10 +94,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); > + } > for (i = 0; i < n_elems; i++) { > void *curr_elem = first_elem + size * i; > > @@ -310,12 +301,16 @@ 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; > > + if (field->flags & VMS_POINTER) { > + first_elem = *(void **)first_elem; > + assert(first_elem); Can you make that assert(first_elem || !n_elems) please. and same above. Dave > + } > for (i = 0; i < n_elems; i++) { > void *curr_elem = first_elem + size * i; > > -- > 2.8.4 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 12/15/2016 02:29 PM, Dr. David Alan Gilbert wrote: >> + vmstate_handle_alloc(first_elem, field, opaque); >> + if (field->flags & VMS_POINTER) { >> + first_elem = *(void **)first_elem; >> + assert(first_elem); >> + } >> for (i = 0; i < n_elems; i++) { >> void *curr_elem = first_elem + size * i; >> >> @@ -310,12 +301,16 @@ 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; >> >> + if (field->flags & VMS_POINTER) { >> + first_elem = *(void **)first_elem; >> + assert(first_elem); > Can you make that assert(first_elem || !n_elems) please. > and same above. > > Dave Good catch! This could indeed be some dynamic length thing with 0 elements. I'm not sure if I'm going to respin this year though. Halil > >> + } >> for (i = 0; i < n_elems; i++) { >> void *curr_elem = first_elem + size * i; >> >> -- >> 2.8.4 >>
* Halil Pasic (pasic@linux.vnet.ibm.com) wrote: > > > On 12/15/2016 02:29 PM, Dr. David Alan Gilbert wrote: > >> + vmstate_handle_alloc(first_elem, field, opaque); > >> + if (field->flags & VMS_POINTER) { > >> + first_elem = *(void **)first_elem; > >> + assert(first_elem); > >> + } > >> for (i = 0; i < n_elems; i++) { > >> void *curr_elem = first_elem + size * i; > >> > >> @@ -310,12 +301,16 @@ 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; > >> > >> + if (field->flags & VMS_POINTER) { > >> + first_elem = *(void **)first_elem; > >> + assert(first_elem); > > Can you make that assert(first_elem || !n_elems) please. > > and same above. > > > > Dave > > Good catch! This could indeed be some dynamic length > thing with 0 elements. > > I'm not sure if I'm going to respin this year though. Well I'm not going to review it this year if you do :-) So have a good new year and lets see to it then! Dave > Halil > > > > >> + } > >> for (i = 0; i < n_elems; i++) { > >> void *curr_elem = first_elem + size * i; > >> > >> -- > >> 2.8.4 > >> > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff --git a/migration/vmstate.c b/migration/vmstate.c index a86fb7e..10a7645 100644 --- a/migration/vmstate.c +++ b/migration/vmstate.c @@ -50,29 +50,15 @@ static int vmstate_size(void *opaque, VMStateField *field) return size; } -static void *vmstate_base_addr(void *opaque, VMStateField *field, bool alloc) -{ - 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); - } +static void vmstate_handle_alloc(void *ptr, VMStateField *field, void *opaque) +{ + 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, @@ -108,10 +94,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); + } for (i = 0; i < n_elems; i++) { void *curr_elem = first_elem + size * i; @@ -310,12 +301,16 @@ 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; + if (field->flags & VMS_POINTER) { + first_elem = *(void **)first_elem; + assert(first_elem); + } for (i = 0; i < n_elems; i++) { void *curr_elem = first_elem + size * i;
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. 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 <pasic@linux.vnet.ibm.com> --- migration/vmstate.c | 41 ++++++++++++++++++----------------------- 1 file changed, 18 insertions(+), 23 deletions(-)