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

[Halil Pasic]

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?

Halil