Re: [Qemu-devel] [PATCH 2/5] migration/vmstate: split up vmstate_base_addr

[Halil Pasic]

On 02/21/2017 01:07 PM, Dr. David Alan Gilbert wrote:
> * Halil Pasic (address@hidden) 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

s/VMS_VBUFFER| VMS_MULTIPLY/VMS_VBUFFER|VMS_MULTIPLY|VMS_ALLOC/

>> substantially simpler than the original vmstate_base_addr.
> 
> Note that VMSTATE_VBUFFER_MULTIPLY does use VMS_VBUFFER|VMS_MULTIPLY - but
> not with alloc (and I started using VMSTATE_VBUFFER_MULTIPLY in a recent
> patch that went in).  

Adding |VMS_ALLOC to avoid confusion. I was hoping it's clear
form the context that we are talking about scenarios where
allocation is relevant.

> I'm not sure I see what the error is that you fixed.

I'm referring to number of bytes allocated. It was
vmstate_n_elems(opaque, field) * field->size
and became
vmstate_n_elems(opaque, field) * vmstate_size(opaque, field)

vmstate_size does extra handling for VMS_VBUFFER and
VMS_MULTIPLY.


> 
> However, I'm ok with it:
> 
> Reviewed-by: Dr. David Alan Gilbert <address@hidden>
> 

Cheers,
Halil

>> 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 <address@hidden>
>> ---
>>  migration/vmstate.c | 39 +++++++++++++++++----------------------
>>  1 file changed, 17 insertions(+), 22 deletions(-)
>>
>> diff --git a/migration/vmstate.c b/migration/vmstate.c
>> index 36efa80..836a7a4 100644
>> --- a/migration/vmstate.c
>> +++ b/migration/vmstate.c
>> @@ -52,29 +52,15 @@ static int vmstate_size(void *opaque, VMStateField 
>> *field)
>>      return size;
>>  }
>>  
>> -static void *vmstate_base_addr(void *opaque, VMStateField *field, bool 
>> alloc)
>> +static void vmstate_handle_alloc(void *ptr, VMStateField *field, void 
>> *opaque)
> 
>>  {
>> -    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);
>> -            }
>> +    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,
>> @@ -116,10 +102,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  || !n_elems);
>> +            }
>>              for (i = 0; i < n_elems; i++) {
>>                  void *curr_elem = first_elem + size * i;
>>  
>> @@ -321,13 +312,17 @@ 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;
>>  
>>              trace_vmstate_save_state_loop(vmsd->name, field->name, n_elems);
>> +            if (field->flags & VMS_POINTER) {
>> +                first_elem = *(void **)first_elem;
>> +                assert(first_elem  || !n_elems);
>> +            }
>>              for (i = 0; i < n_elems; i++) {
>>                  void *curr_elem = first_elem + size * i;
>>  
>> -- 
>> 2.8.4
>>
> --
> Dr. David Alan Gilbert / address@hidden / Manchester, UK
>