[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC PATCH 3/4] migration/vmstate: fix array of pointer
From: |
Halil Pasic |
Subject: |
Re: [Qemu-devel] [RFC PATCH 3/4] migration/vmstate: fix array of pointers to struct |
Date: |
Wed, 26 Oct 2016 14:08:39 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 |
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
[Qemu-devel] [RFC PATCH 1/4] tests/test-vmstate.c: add save_buffer util func, Halil Pasic, 2016/10/21