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
