On 05/22/13 00:32, Michael Roth wrote:
> When this VMSD was introduced it's version fields were set to
> sizeof(I6300State), making them essentially random from build to build,
> version to version.
>
> To fix this, we lock in a high version id and low minimum version id to
> support old->new migration from all prior versions of this device's
> state. This should work since the device state has not changed since
> its introduction.
>
> The potentially breaks migration from 1.5+ to 1.5, but since the
> versioning was essentially random prior to this patch, new->old
> migration was not consistently functional to begin with.
>
> Reported-by: Nicholas Thomas <n...@bytemark.co.uk>
> Suggested-by: Peter Maydell <peter.mayd...@linaro.org>
> Cc: qemu-sta...@nongnu.org
> Signed-off-by: Michael Roth <mdr...@linux.vnet.ibm.com>
> ---
> hw/watchdog/wdt_i6300esb.c | 19 ++++++++++++++++---
> 1 file changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/hw/watchdog/wdt_i6300esb.c b/hw/watchdog/wdt_i6300esb.c
> index 1407fba..851b664 100644
> --- a/hw/watchdog/wdt_i6300esb.c
> +++ b/hw/watchdog/wdt_i6300esb.c
> @@ -374,9 +374,22 @@ static const MemoryRegionOps i6300esb_ops = {
>
> static const VMStateDescription vmstate_i6300esb = {
> .name = "i6300esb_wdt",
> - .version_id = sizeof(I6300State),
> - .minimum_version_id = sizeof(I6300State),
> - .minimum_version_id_old = sizeof(I6300State),
> + /* With this VMSD's introduction, version_id/minimum_version_id were
> + * erroneously set to sizeof(I6300State), causing a somewhat random
> + * version_id to be set for every build. This eventually broke
> + * migration.
> + *
> + * To correct this without breaking old->new migration for older versions
> + * of QEMU, we've set version_id to a value high enough to exceed all
> past
> + * values of sizeof(I6300State) across various build environments, and
> have
> + * reset minimum_version_id_old/minimum_version_id to 1, since this VMSD
> + * has never changed and thus can except all past versions.
As a non-native speaker I think you mean "accept".
> + *
> + * For future changes we can treat these values as we normally would.
> + */
> + .version_id = 10000,
> + .minimum_version_id = 1,
> + .minimum_version_id_old = 1,
> .fields = (VMStateField []) {
> VMSTATE_PCI_DEVICE(dev, I6300State),
> VMSTATE_INT32(reboot_enabled, I6300State),
>
Otherwise looks good to me (which may not mean much :))
Laszlo
