Re: [Qemu-devel] [Qemu-ppc] [QEMU PATCH v8 2/3] migration: migrate QTAILQ

[Jianjun Duan]

On 10/27/2016 04:16 AM, Halil Pasic wrote:
> 
> 
> On 10/26/2016 07:33 PM, Jianjun Duan wrote:
>>>>>>>> +#define QTAILQ_FIRST_OFFSET 0
>>>>>>>>>>>>>> +#define QTAILQ_LAST_OFFSET (sizeof(void *))
>>>>>>>>>>>>
>>>>>>>>>>>> OK, you might want to add some asserts somewhere in a .c just to 
>>>>>>>>>>>> catch if
>>>>>>>>>>>> any of these offsets change.
>>>>>>>>>>>>
>>>>>>>>>> if the layout of QTAILQ changes, the version id for the vmsd should 
>>>>>>>>>> also
>>>>>>>>>> be changed. It will break migration between different versions. 
>>>>>>>>>> However
>>>>>>>>>> the operation doesn't depend on these values.
>>>>>>>>
>>>>>>>> No, changing layout of QTAILQ doesn't need to change the version id of 
>>>>>>>> vmsd;
>>>>>>>> it's an internal change.  But if someone does make the change and 
>>>>>>>> forgets
>>>>>>>> to update your OFFSET macros it'll cause memory corruption.
>>>>>>>> You could catch that with an assert (possibly build time).
>>>>>>>>
>>>>>> If we use these constant I would agree an assertion is necessary. By
>>>>>> using a macro we leave the door open for change. and if someone changes
>>>>>> the layout, he or she better change the macros and the version id. If an
>>>>>> assertion is added, then that assertion needs to be changed to reflect
>>>>>> the change, then in the unlikely situations we could have several
>>>>>> version of layout/macro/assertions floating around. That is too much. SO
>>>>>> version id is the best guard here.
>>>>
>>>> Version id's are irrelevant here.
>>>> The macros are irrelevant here.
>>>> I'm talking about a potential mismatch between the definition of 
>>>> QTAILQ_LAST_OFFSET
>>>> and the definition of Q_TAILQ_HEAD.
>>>>
>>>> Dave
>>>>
>> I suppose anyone who changes the layout should also change the macro and
>> version ID of the relevant vmsd. Similar issue was discussed before as
>> the early version tried to generate all these offsets based on the
>> element and head type. You can see in version 2 discussion.
>>
>>
>> Thanks,
>> Jianjun
> 
> IMHO Dave is right, although a change in head/entry does not seem too
> likely to me. Just want to point out that Dave's proposal (discussed here
> https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg03770.html)
> does not have these issues and it appears more elegant to me. Jianjun
> why do you prefer doing the offsets manually?
> 
It came to the current shape based on previous comments stretching back
almost 6 months. I can change it to whatever reviewers want.

Thanks,
Jianjun
> Halil
> 
>