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

[Paolo Bonzini <pbonzini@redhat.com>]

> >> +    /*
> >> +     * 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.

Sure, don't be worried. :)  True, there are quite a few VMStateInfos (about
35) but the changes are simple.

> >> +#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
                                                          ^^^^^^^^^^^^^^^^

Actually, .first and .next are always 0.  I misread your changes to qemu/queue.h.
See below.

> > 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.

You are already adding QTAILQ_RAW_*, aren't you?  Those interfaces
would need to know about the layout, and you are passing redundant
information:

- .next_offset should always be 0
- .prev_offset should always be sizeof(void *)
- .first_offset should always be 0
- .last_offset should always be sizeof(void *)

so you only need head and entry, which you can store in .offset and
.num_offset.  The .vmsd field you have in ->metadata can be stored
in VMStateField's .vmsd, and likewise for .size (which can be stored
in VMStateField's .vmsd).

Thanks,

Paolo