Re: [Qemu-devel] [QEMU PATCH v10 1/3] migration: extend VMStateInfo

[Juan Quintela <quintela@redhat.com>]

Jianjun Duan <duanj@linux.vnet.ibm.com> wrote:
> On 11/02/2016 03:40 AM, Juan Quintela wrote:
>> Jianjun Duan <duanj@linux.vnet.ibm.com> wrote:
>>> Current migration code cannot handle some data structures such as
>>> QTAILQ in qemu/queue.h. Here we extend the signatures of put/get
>>> in VMStateInfo so that customized handling is supported.
>>>
>>> Signed-off-by: Jianjun Duan <duanj@linux.vnet.ibm.com>
>> 
>>> +/* VMStateInfo allows customized migration of objects that don't fit in
>>> + * any category in VMStateFlags. Additional information can be passed
>>> + * into get and put in terms of field and vmdesc parameters.
>>> + * For primitive data types such as integer, field and vmdesc parameters
>>> + * should be ignored inside get/put. */
>>>  struct VMStateInfo {
>>>      const char *name;
>>> -    int (*get)(QEMUFile *f, void *pv, size_t size);
>>> -    void (*put)(QEMUFile *f, void *pv, size_t size);
>>> +    int (*get)(QEMUFile *f, void *pv, size_t size, VMStateField *field);
>>> +    void (*put)(QEMUFile *f, void *pv, size_t size, VMStateField *field,
>>> +                QJSON *vmdesc);
>> 
>> If we are changing put type, I would like to add an error value to it.
>> 
> so we change the return type to int?

Yeap.  Right now it is impossible to return errors.  If we change the
prototype, better to just add the return of the error, and that
everything return  0 for now.

Thanks, Juan.

>>>  static void vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd,
>>>                                      void *opaque, QJSON *vmdesc);
>>> @@ -83,6 +84,7 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>>>  
>>>      trace_vmstate_load_state(vmsd->name, version_id);
>>>      if (version_id > vmsd->version_id) {
>>> +        error_report("%s %s",  vmsd->name, "too new");
>>>          trace_vmstate_load_state_end(vmsd->name, "too new", -EINVAL);
>>>          return -EINVAL;
>>>      }
>>> @@ -93,6 +95,7 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>>>              trace_vmstate_load_state_end(vmsd->name, "old path", ret);
>>>              return ret;
>>>          }
>>> +        error_report("%s %s",  vmsd->name, "too old");
>>>          trace_vmstate_load_state_end(vmsd->name, "too old", -EINVAL);
>>>          return -EINVAL;
>>>      }
>> 
>> This two are good, but don't belong to this patch, please sent separately.
>> 
> OK.
>>> @@ -122,8 +125,10 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>>>                      ret = vmstate_load_state(f, field->vmsd, addr,
>>>                                               field->vmsd->version_id);
>>>                  } else {
>>> -                    ret = field->info->get(f, addr, size);
>>> -
>>> +                    /* field is always passed in. But it should be ignored by
>>> +                     * get when not needed. It is only needed in cases* of
>>> +                     * customized handling, such as migrating QTAILQ. */
>>> +                    ret = field->info->get(f, addr, size, field);
>> 
>> I would not put this comment here, better on the header when the field
>> is declared?  Same for the next one.
>> 
> OK.
>
> Thanks,
> Jianjun
>>>                  }
>>>                  if (ret >= 0) {
>>>                      ret = qemu_file_get_error(f);
>>> @@ -328,7 +333,11 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
>>>                  if (field->flags & VMS_STRUCT) {
>>>                      vmstate_save_state(f, field->vmsd, addr, vmdesc_loop);
>>>                  } else {
>>> -                    field->info->put(f, addr, size);
>>> +                    /* field and vmdesc_loop are always passed in. But they
>>> +                     * should be ignored by put when not needed. They are
>>> +                     * only needed in cases f customized handling, such as
>>> +                     * migrating QTAILQ. */
>>> +                    field->info->put(f, addr, size, field, vmdesc_loop);
>>>                  }
>> 
>> 
>> Rest of patch is ok.
>>