Patchwork [3/6] vmstate: Add support for alias ID

login
register
mail settings
Submitter Jan Kiszka
Date May 12, 2010, 9:19 p.m.
Message ID <6eda7a61ed240ba027131fbc393822e2a5318654.1273699147.git.jan.kiszka@web.de>
Download mbox | patch
Permalink /patch/52438/
State New
Headers show

Comments

Jan Kiszka - May 12, 2010, 9:19 p.m.
From: Jan Kiszka <jan.kiszka@siemens.com>

Some legacy users (mostly PC devices) of vmstate_register manage
instance IDs on their own, and that unfortunately in a way that is
incompatible with automatically generated ones. This so far prevents
switching those users to vmstates that are registered by qdev.

To establish a migration path, this patch introduces the concept of
alias IDs. They can be passed to an extended vmstate registration
service, and qdev provides a set service to be used during device init.
find_se will consider the alias in addition to the default ID. We can
then start generating the default ID automatically and writing it on
vmsave, thus converting that format without breaking support for upward
migration.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/hw.h   |    3 +++
 hw/qdev.c |   13 +++++++++++--
 hw/qdev.h |    2 ++
 savevm.c  |   16 +++++++++++++---
 4 files changed, 29 insertions(+), 5 deletions(-)
Blue Swirl - May 13, 2010, 7:15 p.m.
On 5/13/10, Jan Kiszka <jan.kiszka@web.de> wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
>
>  Some legacy users (mostly PC devices) of vmstate_register manage
>  instance IDs on their own, and that unfortunately in a way that is
>  incompatible with automatically generated ones. This so far prevents
>  switching those users to vmstates that are registered by qdev.
>
>  To establish a migration path, this patch introduces the concept of
>  alias IDs. They can be passed to an extended vmstate registration
>  service, and qdev provides a set service to be used during device init.
>  find_se will consider the alias in addition to the default ID. We can
>  then start generating the default ID automatically and writing it on
>  vmsave, thus converting that format without breaking support for upward
>  migration.

If this is only for compatibility, I think the name should show it,
like vmstate_set_compat_instance_id(), or
vmstate_set_legacy_instance_id(). That way, if there happens to be an
incompatible version bump, the function name suggests that it can be
removed.

The function should also take a last_legacy_version_id parameter.
Consider for example that a vmstate format with the legacy ID is
currently used with version_id of 2. We also start using this
compatibility system. A new, compatible version 3 arrives but we only
want to support legacy ID for version 2, as indicated by
last_legacy_version_id=2. Then with a new version, let's say 5, which
is no longer compatible with 2 or 3, the legacy ID stuff can finally
be thrown away. qdev.c should check if last_legacy_id >=
minimum_version_id and complain otherwise.
Jan Kiszka - May 13, 2010, 7:40 p.m.
Blue Swirl wrote:
> On 5/13/10, Jan Kiszka <jan.kiszka@web.de> wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>>  Some legacy users (mostly PC devices) of vmstate_register manage
>>  instance IDs on their own, and that unfortunately in a way that is
>>  incompatible with automatically generated ones. This so far prevents
>>  switching those users to vmstates that are registered by qdev.
>>
>>  To establish a migration path, this patch introduces the concept of
>>  alias IDs. They can be passed to an extended vmstate registration
>>  service, and qdev provides a set service to be used during device init.
>>  find_se will consider the alias in addition to the default ID. We can
>>  then start generating the default ID automatically and writing it on
>>  vmsave, thus converting that format without breaking support for upward
>>  migration.
> 
> If this is only for compatibility, I think the name should show it,
> like vmstate_set_compat_instance_id(), or
> vmstate_set_legacy_instance_id(). That way, if there happens to be an
> incompatible version bump, the function name suggests that it can be
> removed.

Hmm, makes some sense, not for the vmstate interface (no new code
outside the core should touch it anymore), but for clarifying the qdev part.

> 
> The function should also take a last_legacy_version_id parameter.
> Consider for example that a vmstate format with the legacy ID is
> currently used with version_id of 2. We also start using this
> compatibility system. A new, compatible version 3 arrives but we only
> want to support legacy ID for version 2, as indicated by
> last_legacy_version_id=2. Then with a new version, let's say 5, which
> is no longer compatible with 2 or 3, the legacy ID stuff can finally
> be thrown away. qdev.c should check if last_legacy_id >=
> minimum_version_id and complain otherwise.

OK, will look into this.

Jan

Patch

diff --git a/hw/hw.h b/hw/hw.h
index 2d39724..d124716 100644
--- a/hw/hw.h
+++ b/hw/hw.h
@@ -758,5 +758,8 @@  extern void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
                                void *opaque);
 extern int vmstate_register(int instance_id, const VMStateDescription *vmsd,
                             void *base);
+extern int vmstate_register_with_alias_id(int instance_id,
+                                          const VMStateDescription *vmsd,
+                                          void *base, int alias_id);
 void vmstate_unregister(const VMStateDescription *vmsd, void *opaque);
 #endif
diff --git a/hw/qdev.c b/hw/qdev.c
index d3bf0fa..445d4d7 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -93,6 +93,7 @@  static DeviceState *qdev_create_from_info(BusState *bus, DeviceInfo *info)
         assert(bus->allow_hotplug);
         dev->hotplugged = 1;
     }
+    dev->instance_id_alias = -1;
     dev->state = DEV_STATE_CREATED;
     return dev;
 }
@@ -278,12 +279,20 @@  int qdev_init(DeviceState *dev)
         return rc;
     }
     qemu_register_reset(qdev_reset, dev);
-    if (dev->info->vmsd)
-        vmstate_register(-1, dev->info->vmsd, dev);
+    if (dev->info->vmsd) {
+        vmstate_register_with_alias_id(-1, dev->info->vmsd, dev,
+                                       dev->instance_id_alias);
+    }
     dev->state = DEV_STATE_INITIALIZED;
     return 0;
 }
 
+void qdev_set_instance_id_alias(DeviceState *dev, int alias_id)
+{
+    assert(dev->state == DEV_STATE_CREATED);
+    dev->instance_id_alias = alias_id;
+}
+
 int qdev_unplug(DeviceState *dev)
 {
     if (!dev->parent_bus->allow_hotplug) {
diff --git a/hw/qdev.h b/hw/qdev.h
index d8fbc73..b7dbee4 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -44,6 +44,7 @@  struct DeviceState {
     QLIST_HEAD(, BusState) child_bus;
     int num_child_bus;
     QLIST_ENTRY(DeviceState) sibling;
+    int instance_id_alias;
 };
 
 typedef void (*bus_dev_printfn)(Monitor *mon, DeviceState *dev, int indent);
@@ -112,6 +113,7 @@  int qdev_device_help(QemuOpts *opts);
 DeviceState *qdev_device_add(QemuOpts *opts);
 int qdev_init(DeviceState *dev) QEMU_WARN_UNUSED_RESULT;
 void qdev_init_nofail(DeviceState *dev);
+void qdev_set_instance_id_alias(DeviceState *dev, int alias_id);
 int qdev_unplug(DeviceState *dev);
 void qdev_free(DeviceState *dev);
 int qdev_simple_unplug_cb(DeviceState *dev);
diff --git a/savevm.c b/savevm.c
index 74e553c..f155582 100644
--- a/savevm.c
+++ b/savevm.c
@@ -991,6 +991,7 @@  typedef struct SaveStateEntry {
     QTAILQ_ENTRY(SaveStateEntry) entry;
     char idstr[256];
     int instance_id;
+    int alias_id;
     int version_id;
     int section_id;
     SaveSetParamsHandler *set_params;
@@ -1079,8 +1080,9 @@  void unregister_savevm(const char *idstr, void *opaque)
     }
 }
 
-int vmstate_register(int instance_id, const VMStateDescription *vmsd,
-                     void *opaque)
+int vmstate_register_with_alias_id(int instance_id,
+                                   const VMStateDescription *vmsd,
+                                   void *opaque, int alias_id)
 {
     SaveStateEntry *se;
 
@@ -1093,6 +1095,7 @@  int vmstate_register(int instance_id, const VMStateDescription *vmsd,
     se->load_state = NULL;
     se->opaque = opaque;
     se->vmsd = vmsd;
+    se->alias_id = alias_id;
 
     if (instance_id == -1) {
         se->instance_id = calculate_new_instance_id(vmsd->name);
@@ -1104,6 +1107,12 @@  int vmstate_register(int instance_id, const VMStateDescription *vmsd,
     return 0;
 }
 
+int vmstate_register(int instance_id, const VMStateDescription *vmsd,
+                     void *opaque)
+{
+    return vmstate_register_with_alias_id(instance_id, vmsd, opaque, -1);
+}
+
 void vmstate_unregister(const VMStateDescription *vmsd, void *opaque)
 {
     SaveStateEntry *se, *new_se;
@@ -1433,7 +1442,8 @@  static SaveStateEntry *find_se(const char *idstr, int instance_id)
 
     QTAILQ_FOREACH(se, &savevm_handlers, entry) {
         if (!strcmp(se->idstr, idstr) &&
-            instance_id == se->instance_id)
+            (instance_id == se->instance_id ||
+             instance_id == se->alias_id))
             return se;
     }
     return NULL;