| Message ID | 1398091304-10677-9-git-send-email-quintela@redhat.com |
|---|---|
| State | New |
| Headers | show |
* Juan Quintela (quintela@redhat.com) wrote: > From: "Michael S. Tsirkin" <mst@redhat.com> > > Move size offset and number of elements math out > to functions, to reduce code duplication. In my original review of Michael's patch I said I was OK with it, but I'd prefer if we had something better than 'int' for vmstate_n_elems, but didn't want to hold up his fix series; if this is part of the huge patch series then we might as well tidy this up. How about: 1) Make vmstate_n_elems return uint32_t or unsigned int 2) Make it check in the int32_t case for a -ve number print a warning and return 0. at the moment it's broken in the corner case of VMS_VARRAY_UINT32 and a huge value. Dave > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > Signed-off-by: Juan Quintela <quintela@redhat.com> > --- > vmstate.c | 100 ++++++++++++++++++++++++++++++++------------------------------ > 1 file changed, 52 insertions(+), 48 deletions(-) > > diff --git a/vmstate.c b/vmstate.c > index bcf1cde..e0debfa 100644 > --- a/vmstate.c > +++ b/vmstate.c > @@ -10,6 +10,50 @@ static void vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd, > static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd, > void *opaque); > > +static int vmstate_n_elems(void *opaque, VMStateField *field) > +{ > + int n_elems = 1; > + > + if (field->flags & VMS_ARRAY) { > + n_elems = field->num; > + } else if (field->flags & VMS_VARRAY_INT32) { > + n_elems = *(int32_t *)(opaque+field->num_offset); > + } else if (field->flags & VMS_VARRAY_UINT32) { > + n_elems = *(uint32_t *)(opaque+field->num_offset); > + } else if (field->flags & VMS_VARRAY_UINT16) { > + n_elems = *(uint16_t *)(opaque+field->num_offset); > + } else if (field->flags & VMS_VARRAY_UINT8) { > + n_elems = *(uint8_t *)(opaque+field->num_offset); > + } > + > + return n_elems; > +} > + > +static int vmstate_size(void *opaque, VMStateField *field) > +{ > + int size = field->size; > + > + if (field->flags & VMS_VBUFFER) { > + size = *(int32_t *)(opaque+field->size_offset); > + if (field->flags & VMS_MULTIPLY) { > + size *= field->size; > + } > + } > + > + return size; > +} > + > +static void *vmstate_base_addr(void *opaque, VMStateField *field) > +{ > + void *base_addr = opaque + field->offset; > + > + if (field->flags & VMS_POINTER) { > + base_addr = *(void **)base_addr + field->start; > + } > + > + return base_addr; > +} > + > int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd, > void *opaque, int version_id) > { > @@ -37,30 +81,10 @@ 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 = opaque + field->offset; > - int i, n_elems = 1; > - int size = field->size; > - > - if (field->flags & VMS_VBUFFER) { > - size = *(int32_t *)(opaque+field->size_offset); > - if (field->flags & VMS_MULTIPLY) { > - size *= field->size; > - } > - } > - if (field->flags & VMS_ARRAY) { > - n_elems = field->num; > - } else if (field->flags & VMS_VARRAY_INT32) { > - n_elems = *(int32_t *)(opaque+field->num_offset); > - } else if (field->flags & VMS_VARRAY_UINT32) { > - n_elems = *(uint32_t *)(opaque+field->num_offset); > - } else if (field->flags & VMS_VARRAY_UINT16) { > - n_elems = *(uint16_t *)(opaque+field->num_offset); > - } else if (field->flags & VMS_VARRAY_UINT8) { > - n_elems = *(uint8_t *)(opaque+field->num_offset); > - } > - if (field->flags & VMS_POINTER) { > - base_addr = *(void **)base_addr + field->start; > - } > + void *base_addr = vmstate_base_addr(opaque, field); > + 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; > > @@ -109,30 +133,10 @@ 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 = opaque + field->offset; > - int i, n_elems = 1; > - int size = field->size; > - > - if (field->flags & VMS_VBUFFER) { > - size = *(int32_t *)(opaque+field->size_offset); > - if (field->flags & VMS_MULTIPLY) { > - size *= field->size; > - } > - } > - if (field->flags & VMS_ARRAY) { > - n_elems = field->num; > - } else if (field->flags & VMS_VARRAY_INT32) { > - n_elems = *(int32_t *)(opaque+field->num_offset); > - } else if (field->flags & VMS_VARRAY_UINT32) { > - n_elems = *(uint32_t *)(opaque+field->num_offset); > - } else if (field->flags & VMS_VARRAY_UINT16) { > - n_elems = *(uint16_t *)(opaque+field->num_offset); > - } else if (field->flags & VMS_VARRAY_UINT8) { > - n_elems = *(uint8_t *)(opaque+field->num_offset); > - } > - if (field->flags & VMS_POINTER) { > - base_addr = *(void **)base_addr + field->start; > - } > + void *base_addr = vmstate_base_addr(opaque, field); > + 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; > > -- > 1.9.0 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Tue, Apr 22, 2014 at 10:59:33AM +0100, Dr. David Alan Gilbert wrote: > * Juan Quintela (quintela@redhat.com) wrote: > > From: "Michael S. Tsirkin" <mst@redhat.com> > > > > Move size offset and number of elements math out > > to functions, to reduce code duplication. > > > In my original review of Michael's patch I said I was OK with it, but I'd > prefer if we had something better than 'int' for vmstate_n_elems, but > didn't want to hold up his fix series; if this is part of the huge patch > series then we might as well tidy this up. > > How about: > 1) Make vmstate_n_elems return uint32_t or unsigned int > 2) Make it check in the int32_t case for a -ve number print a warning and > return 0. > > at the moment it's broken in the corner case of VMS_VARRAY_UINT32 and a huge > value. > > Dave OK just to record what we discussed on IRC, it's probably OK to address this comment in a follow-up patch, so that we don't all get this patchbomb again. > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > Signed-off-by: Juan Quintela <quintela@redhat.com> > > --- > > vmstate.c | 100 ++++++++++++++++++++++++++++++++------------------------------ > > 1 file changed, 52 insertions(+), 48 deletions(-) > > > > diff --git a/vmstate.c b/vmstate.c > > index bcf1cde..e0debfa 100644 > > --- a/vmstate.c > > +++ b/vmstate.c > > @@ -10,6 +10,50 @@ static void vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd, > > static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd, > > void *opaque); > > > > +static int vmstate_n_elems(void *opaque, VMStateField *field) > > +{ > > + int n_elems = 1; > > + > > + if (field->flags & VMS_ARRAY) { > > + n_elems = field->num; > > + } else if (field->flags & VMS_VARRAY_INT32) { > > + n_elems = *(int32_t *)(opaque+field->num_offset); > > + } else if (field->flags & VMS_VARRAY_UINT32) { > > + n_elems = *(uint32_t *)(opaque+field->num_offset); > > + } else if (field->flags & VMS_VARRAY_UINT16) { > > + n_elems = *(uint16_t *)(opaque+field->num_offset); > > + } else if (field->flags & VMS_VARRAY_UINT8) { > > + n_elems = *(uint8_t *)(opaque+field->num_offset); > > + } > > + > > + return n_elems; > > +} > > + > > +static int vmstate_size(void *opaque, VMStateField *field) > > +{ > > + int size = field->size; > > + > > + if (field->flags & VMS_VBUFFER) { > > + size = *(int32_t *)(opaque+field->size_offset); > > + if (field->flags & VMS_MULTIPLY) { > > + size *= field->size; > > + } > > + } > > + > > + return size; > > +} > > + > > +static void *vmstate_base_addr(void *opaque, VMStateField *field) > > +{ > > + void *base_addr = opaque + field->offset; > > + > > + if (field->flags & VMS_POINTER) { > > + base_addr = *(void **)base_addr + field->start; > > + } > > + > > + return base_addr; > > +} > > + > > int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd, > > void *opaque, int version_id) > > { > > @@ -37,30 +81,10 @@ 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 = opaque + field->offset; > > - int i, n_elems = 1; > > - int size = field->size; > > - > > - if (field->flags & VMS_VBUFFER) { > > - size = *(int32_t *)(opaque+field->size_offset); > > - if (field->flags & VMS_MULTIPLY) { > > - size *= field->size; > > - } > > - } > > - if (field->flags & VMS_ARRAY) { > > - n_elems = field->num; > > - } else if (field->flags & VMS_VARRAY_INT32) { > > - n_elems = *(int32_t *)(opaque+field->num_offset); > > - } else if (field->flags & VMS_VARRAY_UINT32) { > > - n_elems = *(uint32_t *)(opaque+field->num_offset); > > - } else if (field->flags & VMS_VARRAY_UINT16) { > > - n_elems = *(uint16_t *)(opaque+field->num_offset); > > - } else if (field->flags & VMS_VARRAY_UINT8) { > > - n_elems = *(uint8_t *)(opaque+field->num_offset); > > - } > > - if (field->flags & VMS_POINTER) { > > - base_addr = *(void **)base_addr + field->start; > > - } > > + void *base_addr = vmstate_base_addr(opaque, field); > > + 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; > > > > @@ -109,30 +133,10 @@ 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 = opaque + field->offset; > > - int i, n_elems = 1; > > - int size = field->size; > > - > > - if (field->flags & VMS_VBUFFER) { > > - size = *(int32_t *)(opaque+field->size_offset); > > - if (field->flags & VMS_MULTIPLY) { > > - size *= field->size; > > - } > > - } > > - if (field->flags & VMS_ARRAY) { > > - n_elems = field->num; > > - } else if (field->flags & VMS_VARRAY_INT32) { > > - n_elems = *(int32_t *)(opaque+field->num_offset); > > - } else if (field->flags & VMS_VARRAY_UINT32) { > > - n_elems = *(uint32_t *)(opaque+field->num_offset); > > - } else if (field->flags & VMS_VARRAY_UINT16) { > > - n_elems = *(uint16_t *)(opaque+field->num_offset); > > - } else if (field->flags & VMS_VARRAY_UINT8) { > > - n_elems = *(uint8_t *)(opaque+field->num_offset); > > - } > > - if (field->flags & VMS_POINTER) { > > - base_addr = *(void **)base_addr + field->start; > > - } > > + void *base_addr = vmstate_base_addr(opaque, field); > > + 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; > > > > -- > > 1.9.0 > > > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff --git a/vmstate.c b/vmstate.c index bcf1cde..e0debfa 100644 --- a/vmstate.c +++ b/vmstate.c @@ -10,6 +10,50 @@ static void vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd, static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd, void *opaque); +static int vmstate_n_elems(void *opaque, VMStateField *field) +{ + int n_elems = 1; + + if (field->flags & VMS_ARRAY) { + n_elems = field->num; + } else if (field->flags & VMS_VARRAY_INT32) { + n_elems = *(int32_t *)(opaque+field->num_offset); + } else if (field->flags & VMS_VARRAY_UINT32) { + n_elems = *(uint32_t *)(opaque+field->num_offset); + } else if (field->flags & VMS_VARRAY_UINT16) { + n_elems = *(uint16_t *)(opaque+field->num_offset); + } else if (field->flags & VMS_VARRAY_UINT8) { + n_elems = *(uint8_t *)(opaque+field->num_offset); + } + + return n_elems; +} + +static int vmstate_size(void *opaque, VMStateField *field) +{ + int size = field->size; + + if (field->flags & VMS_VBUFFER) { + size = *(int32_t *)(opaque+field->size_offset); + if (field->flags & VMS_MULTIPLY) { + size *= field->size; + } + } + + return size; +} + +static void *vmstate_base_addr(void *opaque, VMStateField *field) +{ + void *base_addr = opaque + field->offset; + + if (field->flags & VMS_POINTER) { + base_addr = *(void **)base_addr + field->start; + } + + return base_addr; +} + int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd, void *opaque, int version_id) { @@ -37,30 +81,10 @@ 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 = opaque + field->offset; - int i, n_elems = 1; - int size = field->size; - - if (field->flags & VMS_VBUFFER) { - size = *(int32_t *)(opaque+field->size_offset); - if (field->flags & VMS_MULTIPLY) { - size *= field->size; - } - } - if (field->flags & VMS_ARRAY) { - n_elems = field->num; - } else if (field->flags & VMS_VARRAY_INT32) { - n_elems = *(int32_t *)(opaque+field->num_offset); - } else if (field->flags & VMS_VARRAY_UINT32) { - n_elems = *(uint32_t *)(opaque+field->num_offset); - } else if (field->flags & VMS_VARRAY_UINT16) { - n_elems = *(uint16_t *)(opaque+field->num_offset); - } else if (field->flags & VMS_VARRAY_UINT8) { - n_elems = *(uint8_t *)(opaque+field->num_offset); - } - if (field->flags & VMS_POINTER) { - base_addr = *(void **)base_addr + field->start; - } + void *base_addr = vmstate_base_addr(opaque, field); + 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; @@ -109,30 +133,10 @@ 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 = opaque + field->offset; - int i, n_elems = 1; - int size = field->size; - - if (field->flags & VMS_VBUFFER) { - size = *(int32_t *)(opaque+field->size_offset); - if (field->flags & VMS_MULTIPLY) { - size *= field->size; - } - } - if (field->flags & VMS_ARRAY) { - n_elems = field->num; - } else if (field->flags & VMS_VARRAY_INT32) { - n_elems = *(int32_t *)(opaque+field->num_offset); - } else if (field->flags & VMS_VARRAY_UINT32) { - n_elems = *(uint32_t *)(opaque+field->num_offset); - } else if (field->flags & VMS_VARRAY_UINT16) { - n_elems = *(uint16_t *)(opaque+field->num_offset); - } else if (field->flags & VMS_VARRAY_UINT8) { - n_elems = *(uint8_t *)(opaque+field->num_offset); - } - if (field->flags & VMS_POINTER) { - base_addr = *(void **)base_addr + field->start; - } + void *base_addr = vmstate_base_addr(opaque, field); + 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;