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

[Juan Quintela <quintela@redhat.com>]

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.

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

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

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