Re: [PATCH 1/2] migration: Boost SaveStateEntry.instance_id to 64 bits

[Juan Quintela]

Peter Xu <address@hidden> wrote:
> It was "int" and used as 32bits fields (see save_section_header()).
> It's unsafe already because sizeof(int) could be 2 on i386, I think.
> So at least uint32_t would suite more.  While it also uses "-1" as a
> placeholder of "we want to generate the instance ID automatically".
> Hence a more proper value should be int64_t.
>
> This will start to be useful after next patch in which we can start to
> convert a real uint32_t value as instance ID.
>
> Signed-off-by: Peter Xu <address@hidden>

Hi

Being more helpful,  I think that it is better to just:

* change instance_id to be an uint32_t (notice that for all architectures
  that we support, it is actually int32_t).

* export calculate_new_instance_id() and adjust callers that use -1.

or

* export a new function that just use the calculate_new_instance_id()

A fast search shows:

10 callers of vmstate_register() with -1
1 caller of vmstate_register_with_alias_id with -1 (but it is the one
  that sets all qdev devices).
1 caller of vmstate_register_with_alias_id in apic, where it can be -1.
1 caller of register_savevm_live() with -1 (spapr)

And call it a day?

What do you think, Juan.

> ---
>  include/migration/register.h |  2 +-
>  include/migration/vmstate.h  |  4 ++--
>  migration/savevm.c           | 10 +++++-----
>  stubs/vmstate.c              |  2 +-
>  4 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/include/migration/register.h b/include/migration/register.h
> index a13359a08d..54f42c7413 100644
> --- a/include/migration/register.h
> +++ b/include/migration/register.h
> @@ -69,7 +69,7 @@ typedef struct SaveVMHandlers {
>  } SaveVMHandlers;
>  
>  int register_savevm_live(const char *idstr,
> -                         int instance_id,
> +                         int64_t instance_id,
>                           int version_id,
>                           const SaveVMHandlers *ops,
>                           void *opaque);
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 1fbfd099dd..6a7498463c 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -1114,14 +1114,14 @@ int vmstate_save_state_v(QEMUFile *f, const 
> VMStateDescription *vmsd,
>  bool vmstate_save_needed(const VMStateDescription *vmsd, void *opaque);
>  
>  /* Returns: 0 on success, -1 on failure */
> -int vmstate_register_with_alias_id(DeviceState *dev, int instance_id,
> +int vmstate_register_with_alias_id(DeviceState *dev, int64_t instance_id,
>                                     const VMStateDescription *vmsd,
>                                     void *base, int alias_id,
>                                     int required_for_version,
>                                     Error **errp);
>  
>  /* Returns: 0 on success, -1 on failure */
> -static inline int vmstate_register(DeviceState *dev, int instance_id,
> +static inline int vmstate_register(DeviceState *dev, int64_t instance_id,
>                                     const VMStateDescription *vmsd,
>                                     void *opaque)
>  {
> diff --git a/migration/savevm.c b/migration/savevm.c
> index bb9462a54d..dc9281c897 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -233,7 +233,7 @@ typedef struct CompatEntry {
>  typedef struct SaveStateEntry {
>      QTAILQ_ENTRY(SaveStateEntry) entry;
>      char idstr[256];
> -    int instance_id;
> +    int64_t instance_id;
>      int alias_id;
>      int version_id;
>      /* version id read from the stream */
> @@ -668,7 +668,7 @@ void dump_vmstate_json_to_file(FILE *out_file)
>  static int calculate_new_instance_id(const char *idstr)
>  {
>      SaveStateEntry *se;
> -    int instance_id = 0;
> +    int64_t instance_id = 0;
>  
>      QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
>          if (strcmp(idstr, se->idstr) == 0
> @@ -730,7 +730,7 @@ static void savevm_state_handler_insert(SaveStateEntry 
> *nse)
>     Meanwhile pass -1 as instance_id if you do not already have a clearly
>     distinguishing id for all instances of your device class. */
>  int register_savevm_live(const char *idstr,
> -                         int instance_id,
> +                         int64_t instance_id,
>                           int version_id,
>                           const SaveVMHandlers *ops,
>                           void *opaque)
> @@ -784,7 +784,7 @@ void unregister_savevm(DeviceState *dev, const char 
> *idstr, void *opaque)
>      }
>  }
>  
> -int vmstate_register_with_alias_id(DeviceState *dev, int instance_id,
> +int vmstate_register_with_alias_id(DeviceState *dev, int64_t instance_id,
>                                     const VMStateDescription *vmsd,
>                                     void *opaque, int alias_id,
>                                     int required_for_version,
> @@ -1566,7 +1566,7 @@ int qemu_save_device_state(QEMUFile *f)
>      return qemu_file_get_error(f);
>  }
>  
> -static SaveStateEntry *find_se(const char *idstr, int instance_id)
> +static SaveStateEntry *find_se(const char *idstr, int64_t instance_id)
>  {
>      SaveStateEntry *se;
>  
> diff --git a/stubs/vmstate.c b/stubs/vmstate.c
> index e1e89b87f0..699003f3b0 100644
> --- a/stubs/vmstate.c
> +++ b/stubs/vmstate.c
> @@ -4,7 +4,7 @@
>  const VMStateDescription vmstate_dummy = {};
>  
>  int vmstate_register_with_alias_id(DeviceState *dev,
> -                                   int instance_id,
> +                                   int64_t instance_id,
>                                     const VMStateDescription *vmsd,
>                                     void *base, int alias_id,
>                                     int required_for_version,