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

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

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