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