From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MlOUS-0005Xl-RP for qemu-devel@nongnu.org; Wed, 09 Sep 2009 10:49:44 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MlOUN-0005QF-I6 for qemu-devel@nongnu.org; Wed, 09 Sep 2009 10:49:43 -0400 Received: from [199.232.76.173] (port=47724 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MlOUN-0005Q6-Bc for qemu-devel@nongnu.org; Wed, 09 Sep 2009 10:49:39 -0400 Received: from mx1.redhat.com ([209.132.183.28]:2922) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MlOUM-0005a3-O8 for qemu-devel@nongnu.org; Wed, 09 Sep 2009 10:49:39 -0400 From: Juan Quintela In-Reply-To: <4AA7BEA7.6080906@codemonkey.ws> (Anthony Liguori's message of "Wed, 09 Sep 2009 09:41:43 -0500") References: <4AA7BEA7.6080906@codemonkey.ws> Date: Wed, 09 Sep 2009 16:49:32 +0200 Message-ID: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Subject: [Qemu-devel] Re: The State of the SaveVM format List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: qemu-devel@nongnu.org, "Michael S. Tsirkin" Anthony Liguori wrote: > Hi Juan, > This is not quite an accurate representation. Sorry. > 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. However, you still point > to the old savevm function which is: > if (version_id == 3) > s->scancode_set=qemu_get_be32(f); > else > s->scancode_set=2; Problem here is this value, there is no way to set default values different of zero. That is why there is still the old function. > Which has to be an error. But this is the real problem with leaving > the old functions. It encourages sloppiness. I don't like to leave old versions either. Just we have to get a balance between leaving old versions or get new ones. > Let's look at a more complex example (piix_pci): > > VMSTATE_PCI_DEVICE(dev, PCII440FXState), > VMSTATE_UINT8(smm_enabled, PCII440FXState), > > This is v3. You have an old load function that duplicates this > functionality but has an additional field: > > if (version_id == 2) > for (i = 0; i < 4; i++) > d->irq_state->piix3->pci_irq_levels[i] = qemu_get_be32(f); > > All I'm suggesting, is that instead of leaving that old function, you > introduce: > > 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_PCI_DEVICE(dev, PCII440FXState), > VMSTATE_UINT8(smm_enabled, PCII440FXState), > VMSTATE_FUNC_V(irq_state->piix3->pci_irq_levels, > PCII440FXState, marshal_pci_irq_levels, 2) > > This avoids bit rot of the old load functions and makes the whole > thing more robust. My contention is that there simply isn't a lot of > these special cases so it's not a lot more work overall. Ok, will take a look at doing the _own_ marshaling, it has other features, now a field becomes optional, that is the next item. [ Optional features stuff removed ] > I think the discussion around optional features is orthogonal to how > to support older savevm formats so let's keep it separate. I > generally share your concern about test matrix explosion. Ok. I am trying to port all the pc.c stuff to new VMState without adding more marshaling (we already have the old load functions). After I finish with that, we can look at the remaining cases and look at a course of action? > Regards, > > Anthony Liguori Thanks, Juan.