Re: [Qemu-devel] [QEMU RFC PATCH v2 4/6] Migration: migrate QTAILQ

[Jianjun Duan <duanj@linux.vnet.ibm.com>]

On 05/25/2016 01:14 AM, Paolo Bonzini wrote:
> 
> 
> On 24/05/2016 19:55, Jianjun Duan wrote:
>> +/*
>> + * Offsets of layout of a tail queue head.
>> + */
>> +#define QTAILQ_FIRST_OFFSET(head_type) \
>> +        ((size_t) ((char *) &((head_type *)0)->tqh_first - (char *)0))
>> +
>> +#define QTAILQ_LAST_OFFSET(head_type) \
>> +        ((size_t) ((char *) &((head_type *)0)->tqh_last - (char *)0))
>> +
>> +/*
>> + * Offsets of layout of a tail queue element.
>> + */
>> +#define QTAILQ_NEXT_OFFSET(ele_type, field)                            \
>> +        ((size_t) ((char *) &((ele_type *)0)->field.tqe_next -         \
>> +                   (char *) &((ele_type *)0)->field))
>> +
>> +#define QTAILQ_PREV_OFFSET(ele_type, field) \
>> +        ((size_t) ((char *) &((ele_type *)0)->field.tqe_prev -         \
>> +                   (char *) &((ele_type *)0)->field))
> 
> Please use offsetof.
OK.

> 
>> +    /*
>> +     * Following 3 fields are for VMStateField which needs customized handling,
>> +     * such as QTAILQ in qemu/queue.h, lists, and tree.
>> +     */
>> +    const void *meta_data;
>> +    int (*extend_get)(QEMUFile *f, const void *metadata, void *opaque);
>> +    void (*extend_put)(QEMUFile *f, const void *metadata, void *opaque,
>> +                       QJSON *vmdesc);
>>  } VMStateField;
> 
> Do not add these two function pointers to VMStateField, instead add
> QJSON* and VMStateField* arguments as needed to VMStateInfo's get and put.
> 
That is definitely the ideal way. However, VMStateInfo's get/put are
already used extensively. If I change them, I need change all the
calling sites of them. Not very sure about whether it will be welcomed.
>> +#define VMSTATE_QTAILQ_METADATA(_field, _state, _type, _next, _vmsd) { \
>> +    .first = QTAILQ_FIRST_OFFSET(typeof_field(_state, _field)),        \
>> +    .last =  QTAILQ_LAST_OFFSET(typeof_field(_state, _field)),         \
>> +    .next = QTAILQ_NEXT_OFFSET(_type, _next),                          \
>> +    .prev = QTAILQ_PREV_OFFSET(_type, _next),                          \
>> +    .entry = offsetof(_type, _next),                                   \
>> +    .size = sizeof(_type),                                             \
>> +    .vmsd = &(_vmsd),                                                  \
>> +}
> 
> .last and .prev are unnecessary, since they come just after .first and
> .next (and their use is hidden behind QTAILQ_RAW_*).  .first and .next
> can be placed in .offset and .num_offset respectively.  So you don't
> really need an extra metadata struct.
> 
> If you prefer you could have something like
> 
>     union {
>         size_t num_offset;    /* VMS_VARRAY */
>         size_t size_offset;   /* VMS_VBUFFER */
>         size_t next_offset;   /* VMS_TAILQ */
>     } offsets;
> 
Actually I explored the approach in which I use a VMSD to encode all the
information. But a VMSD describes actual memory layout. Interpreting it
another way could be confusing.

One of the assumption about QTAILQ is that we can only use the
interfaces defined in queue.h to access it. I intend not to depend on
its actually layout. From this perspective, these 6 parameters are
needed.

> Thanks,
> 
> Paolo
> 
Thanks,
Jianjun