From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43)
	id 1MlP04-0004JE-Ji
	for qemu-devel@nongnu.org; Wed, 09 Sep 2009 11:22:24 -0400
Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43)
	id 1MlOzz-0004BH-7w
	for qemu-devel@nongnu.org; Wed, 09 Sep 2009 11:22:23 -0400
Received: from [199.232.76.173] (port=51280 helo=monty-python.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.43) id 1MlOzz-0004B6-1w
	for qemu-devel@nongnu.org; Wed, 09 Sep 2009 11:22:19 -0400
Received: from mx1.redhat.com ([209.132.183.28]:20426)
	by monty-python.gnu.org with esmtp (Exim 4.60)
	(envelope-from <kraxel@redhat.com>) id 1MlOzy-00035P-1R
	for qemu-devel@nongnu.org; Wed, 09 Sep 2009 11:22:18 -0400
Message-ID: <4AA7C823.8080909@redhat.com>
Date: Wed, 09 Sep 2009 17:22:11 +0200
From: Gerd Hoffmann <kraxel@redhat.com>
MIME-Version: 1.0
Subject: Re: [Qemu-devel] The State of the SaveVM format
References: <m3hbvcijxc.fsf@neno.mitica> <4AA7BEA7.6080906@codemonkey.ws>
In-Reply-To: <4AA7BEA7.6080906@codemonkey.ws>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
List-Id: qemu-devel.nongnu.org
List-Unsubscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.gnu.org/pipermail/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Anthony Liguori <anthony@codemonkey.ws>
Cc: "Michael S. Tsirkin" <mst@redhat.com>, qemu-devel@nongnu.org, Juan Quintela <quintela@redhat.com>

   Hi,

> Today, you make no attempt to support older versions even if their
> format is quite sane. Take ps2_kbd as an example.
>
> The new format (v3) is:
>
> VMSTATE_STRUCT(common, PS2KbdState, 0, vmstate_ps2_common, PS2State),
> VMSTATE_INT32(scan_enabled, PS2KbdState),
> VMSTATE_INT32(translate, PS2KbdState),
> VMSTATE_INT32_V(scancode_set, PS2KbdState,3),
>
> This is nice and should support v2 and v3.

It doesn't ...

> if (version_id == 3)
> s->scancode_set=qemu_get_be32(f);
> else
> s->scancode_set=2;

... setting scancode_set when loading v2 is missing.

I think vmstate fields need a default value to handle cases like this 
one without having to keep the old load function.

> Which has to be an error. But this is the real problem with leaving the
> old functions. It encourages sloppiness.

I think we can kill most of the old load functions.  I'd keep the old 
ones only in case emulating the old load function with vmstate would 
make it unreasonable complex.

> static void marshal_pci_irq_levels(void *opaque, const char *name,
> size_t offset, int load, int version)
> {
> if (version == 2) {
> for (i = 0; i < 4; i++)
> d->irq_state->piix3->pci_irq_levels[i] = qemu_get_be32(f);
> }
> }

> VMSTATE_FUNC_V(irq_state->piix3->pci_irq_levels, PCII440FXState,
> marshal_pci_irq_levels, 2)

No.  I don't want any free-form C code in vmstate.  That will kill quite 
a few of the vmstate advantages.  Imagine a tool dumping snapshot data. 
  What this tool should do when it finds such a FUNC field?  It has 
absolutely no idea what is in there ...

cheers,
   Gerd