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

[Halil Pasic <pasic@linux.vnet.ibm.com>]

On 10/31/2016 07:25 PM, Jianjun Duan wrote:
> 
> 
> On 10/31/2016 11:01 AM, Halil Pasic wrote:
>>
>>
>> On 10/31/2016 06:32 PM, Jianjun Duan wrote:
>>>>>> I think this got overly complicated. Here is a little patch on
>>>>>>>>>>> top of your stuff which gets rid of 15 lines and IMHO simplifies
>>>>>>>>>>> things quite a bit.  What do you think? 
>>>>>>>>>>>
>>>>>>>>>>> It is based on/inspired by Dave's proposal with the dummy stuff. 
>>>>>>>>>>>
>>>>>>>>>>> Did not address the typos though.
>>>>>>> It is unlikely the definition of QTAILQ will change, so hard-coded
>>>>>>> value probably was the most simple. Now that we want to address the
>>>>>>> potential changes, I think my code will deal with future changes better.
>>>>>
>>>>> Please elaborate in what way does your version deal better with future
>>>>> changes? IMHO the version with my patch applied covers all the corners
>>>>> your original code covers but it is without any doubt more concise and
>>>>> in my opinion also more straight-forward.
>>> I don't use the internals of head and entry structures if there are
>>> access macro around. Also I didn't use pointer type cast. I don't think
>>> pointer cast is more straightforward.
>>
>>
>> Internals is quite relative since the stuff is part of the header
>> and there is no indication that it's not part of the API. About 
>> pointer type casts I do not understand what you mean by that:
>> my understanding is that we both do casts to a pointer type. And
>> I do think it is more straightforward than defining a macro for the
>> offset for each link-pointer and then basically play the field_at_offset
>> game twice: once to pin point the struct holding all the links, and 
>> once again to pinpoint the individual links.
>>
> It is more concise but not more straightforward for me.
> 
>> As you see I do not believe you that your version is more robust.
>> If you want to convince me _give me a remotely reasonable patch_ which
>> beaks my version but not yours.
>>
>> The straight-forwardness is probably a matter of taste, and that's
>> why I used IMHO. I'm not sure if I understood you correctly, do
>> you think that the current version of the code is more readable
>> that what I have proposed? If you do, I doubt there is a rational 
>> way to settle this taste dispute. If the majority of the people
>> says your approach is more straight-forward then I apologize.
>>
> 
> I agree it is a matter of taste. With current code both versions will
> not break. But if somebody changes the name of the var you directly
> accessed, your code will break. Of course I don't expect somebody to
> just do that.
> 
> It is indeed in the header file and we expect people to change all at
> once if any change is done. I used this argument for hard encoding and
> it is not taken.

There is a huge difference. If somebody changes the name of tqe_next
for instance it will break compile-time and it will also break the
normal (non-raw) macros. Same goes for the case if someone wanted to
remove the Q_TAILQ_HEAD macro for example (or am I missing something). 
In your case there was no compile time error but a possible run-time
memory corruption instead. If you see no difference between these I
really do not know how to do constructive discussion.

Cheers,
Halil

> 
> Thanks,
> Jianjun
>  > Cheers,
>> Halil
>>