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