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

[Dr. David Alan Gilbert]

* Jianjun Duan (address@hidden) wrote:
> Hi Sascha,
> 
> On 06/02/2016 08:01 AM, Sascha Silbe wrote:
> > Dear Jianjun,
> > 
> > Jianjun Duan <address@hidden> writes:
> > 
> > [include/migration/vmstate.h]
> >> @@ -185,6 +185,8 @@ enum VMStateFlags {
> >>       * to determine the number of entries in the array. Only valid in
> >>       * combination with one of VMS_VARRAY*. */
> >>      VMS_MULTIPLY_ELEMENTS = 0x4000,
> >> +    /* For fields which need customized handling, such as QTAILQ in 
> >> queue.h*/
> >> +    VMS_CSTM            = 0x8000,
> > 
> > Can you describe (in the comment) how this customised handling is
> > performed, please? I.e. describe what exactly happens if the flag is
> > set, from the point of view of an API consumer.
> 
> I will add more comments. When this flag is set VMStateInfo.get/put will
> be used in vmstate_load/save_state instead of recursive call. And the
> user should implement VMStateInfo.get/put to handle the concerned data
> structure.
> > Also, why do you need this flag at all? The only change I can see is
> > that you pass additional information to VMStateInfo.get() / .put(),
> > using NULL if it's not set. Why don't you just always pass the
> > additional information? If the additional information is not needed by
> > get() / put() the parameter will be unused anyway.
> You can do it without creating this flag. Instead just to check if info
> is set in the field. However I think it is more readable and more robust
> to check this flag in vmstate_load/get_state.

Apologies for not getting around to this sooner; I was on holiday when you first
sent it.

But:
  a) Is there a reason to use the 'bool' between each element; can't you count 
the
     length of the queue, send the length and then send the contents?  In that 
case
     it should look a lot more like an ARRAY case on the wire.
  b) I think you should really try and split it into two parts:
    1) A VMSTATE_ARRAY_CUSTOM (?) - so it's an array of elements but we've got 
a special
       way of allocating/counting/reading the elements
    2) A version of that which is for a QTAILQ.
       It's important that whatever ends up on the migration stream doesn't 
reflect
       that it happens to be implemented as a QTAILQ; so if you decide to change
       it later the migration compatibility doesn't break.
  c) Use new trace_ names for get_qtailq rather than misusing 
trace_vmstate_load_state*
  d) Add a trace_ for put_qtailq as well - that way when it goes horribly wrong 
we can
     turn the tracing on on both sides :-)
  e) Is there anyway to make QTAILQ_RAW_INSERT_TAIL any more readable?
     I don't think I understand why you can't use the standard QTAILQ macros.
  f) You might need to fix up Amit's scripts/vmstate-static-checker.py

Dave

> > Sascha
> > 
> 
> Thanks,
> Jianjun
> 
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK