Re: [Qemu-devel] [RFC PATCH 3/4] migration/vmstate: fix array of pointers to struct

[Halil Pasic]

On 10/25/2016 12:13 PM, Dr. David Alan Gilbert wrote:
[..]
>>              for (i = 0; i < n_elems; i++) {
>> -                void *addr = base_addr + size * i;
>> +                void *curr_elem = first_elem + size * i;
> 
> This diff is quite confusing because a lot of it involves the
> rename of 'addr' to 'curr_elem' at the same time as you change
> the structure.  It would be better to split the renaming into
> a separate patch to make this clearer or just leave the name
> the same.
> 

You are absolutely right this is a Frankestein of a cleanup
patch and the actual patch. I will split the cleanup out.

The patch is also conceptually based on my remove .start patch
it's just that I wanted to make the RFC independent so it can
be tested more easily.

[..]
>> @@ -720,6 +722,27 @@ const VMStateInfo vmstate_info_uint64 = {
>>      .put  = put_uint64,
>>  };
>>  
>> +static int get_nullptr(QEMUFile *f, void *pv, size_t size)
>> +{
>> +    int8_t tmp;
>> +    qemu_get_s8s(f, &tmp);
>> +    assert(tmp == 0);
> 
> There's no need for the assert there, just return -EINVAL,
> then we'll get a clear error.

Will do.

> Also, '0' is a bad value to use just as a check - if the field is wrong then
> 0 often appears in the next byte anyway; 
> 

Absolutely right. How about -1?

> However, I'm not sure it's worth having the info_nullptr;
> if we just leave out the whole info_nullptr then you should still
> be protected by the section footer, although this may be
> able to give a better error.
> 

IMHO this can (in some cases) guard against the case we have the
same number of elements on source and on target, but at different
positions (e.g. {ptr0, NULL, NULL} and {NULL, ptr1, NULL}. The footers
should not be able to detect this.

Thank you very much for the thorough review! I will wait a bit
to see if more discussion happens, and then send out a non RFC
version with the issues addressed.

Halil