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

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

On 11/01/2016 04:02 PM, Paolo Bonzini wrote:
> 
> 
> On 31/10/2016 14:10, Halil Pasic wrote:
>> I think this got overly complicated.
> 
> I agree. :)
> 
>> 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.
> 
> I still prefer my field_at_offset proposal, but I'm obviously biased.

:) have searched it

   *field_at_offset(void **, elm, (entry) + QTAILQ_NEXT_OFFSET) = NULL;
   *field_at_offset(void ***, elm, (entry) + QTAILQ_PREV_OFFSET) =
       *field_at_offset(void ***, head, QTAILQ_LAST_OFFSET);
   **field_at_offset(void ***, head, QTAILQ_LAST_OFFSET) = (elm);
   *field_at_offset(void ***, head, QTAILQ_LAST_OFFSET) =
        field_at_offset(void **, elm, (entry) + QTAILQ_NEXT_OFFSET);

That was for the insert of course.

A lot of * for my taste and we still have the QTAILQ_NEXT_OFFSET
and QTAILQ_PREV_OFFSET macros which are supposed to capture
what QTAILQRawEntry does (namely where is the next and the
prev pointer within the entry -- not the element). 

Obviously I'm biased too ;)

> Squashing your changes on top of 2/3 is fine too.  But this change makes
> little sense:
> 
>>   */
>>  #define QTAILQ_RAW_NEXT(elm, entry)                                            \
>> -        (*field_at_offset(elm, entry + QTAILQ_NEXT_OFFSET, void **))
>> +        ((field_at_offset(elm, entry, QTAILQRawEntry *)->tqe_next))
>>  #define QTAILQ_RAW_PREV(elm, entry)                                            \
>> -        (*field_at_offset(elm, entry + QTAILQ_PREV_OFFSET, void ***))
>> +        (field_at_offset(elm, entry, QTAILQRawEntry *)->tqe_prev)
>> +
> 
> field_at_offset seems to be out of place.

Me scratching head. This is the place where we pinpoint the qtailq 
entry which is at offset entry within the element and reinterpret
the pointer as a pointer to QTAILQRawEntry. In the end we end up
with a void pointer to the next or prev element.

I think
#define QTAILQ_RAW_NEXT(elm, entry_offset)                                         \
        ((field_at_offset(elm, entry_offset, QTAILQRawEntry *)->tqe_next)
would be more readable though.

Could you explain whats wrong with that?

Thank you very much for commenting!

Halil

> 
> Paolo
>