Re: [Qemu-devel] [PATCH 008/124] vmstate: Reduce code duplication

[Michael S. Tsirkin]

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