* Juan Quintela (quint...@redhat.com) wrote:
> Peter Xu <pet...@redhat.com> 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 <pet...@redhat.com>
>
> 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()
Do you mean that we end up with two functions, one that does it
automatically, and one that takes an ID?
Dave
> 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,
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
