On Tue, Apr 22, 2014 at 10:59:33AM +0100, Dr. David Alan Gilbert wrote:
> * Juan Quintela (quint...@redhat.com) wrote:
> > From: "Michael S. Tsirkin" <m...@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 <m...@redhat.com>
> > Cc: "Dr. David Alan Gilbert" <dgilb...@redhat.com>
> > Signed-off-by: Juan Quintela <quint...@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 / dgilb...@redhat.com / Manchester, UK
