Re: [Qemu-devel] [RFC PATCH v2 6/8] migration/vmstate: split up vmstate_base_addr

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* 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