Re: [Qemu-devel] [PATCH] wdt_i6300esb: fix vmstate versioning

[Laszlo Ersek <lersek@redhat.com>]

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 <nick@bytemark.co.uk>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Michael Roth <mdroth@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