Re: [Qemu-devel] [PATCH] scsi: esp: fix migration

[Amit Shah <amit.shah@redhat.com>]

On (Wed) 29 Jun 2016 [14:00:17], Juan Quintela wrote:
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> > On 27/06/2016 09:20, Amit Shah wrote:
> >> On (Mon) 20 Jun 2016 [16:33:26], Paolo Bonzini wrote:
> >>> Commit 926cde5 ("scsi: esp: make cmdbuf big enough for maximum CDB size",
> >>> 2016-06-16) changed the size of a migrated field.  Split it in two
> >>> parts, and only migrate the second part in a new vmstate version.
> >> 
> >> With this patch, the static checker fails in this way:
> >> 
> >> Section "esp", Description "esp": expected field "cmdlen", got
> >> "cmdbuf"; skipping rest
> >> Section "dc390", Description "esp": expected field "cmdlen", got
> >> "cmdbuf"; skipping rest
> >> Section "am53c974", Description "esp": expected field "cmdlen", got
> >> "cmdbuf"; skipping rest
> >> 
> >> Note it doesn't complain about the version numbers.  That's because:
> >> 
> >>>  const VMStateDescription vmstate_esp = {
> >>>      .name ="esp",
> >>> -    .version_id = 3,
> >>> +    .version_id = 4,
> >>>      .minimum_version_id = 3,
> >> 
> >> this suggests older versions can still be accepted for incoming
> >> migration, which isn't true.
> >
> > Sure they can:
> >
> > -        VMSTATE_BUFFER(cmdbuf, ESPState),
> > +        VMSTATE_PARTIAL_BUFFER(cmdbuf, ESPState, 16),
> > +        VMSTATE_BUFFER_START_MIDDLE_V(cmdbuf, ESPState, 16, 4),
> 
> Amit, would it help the checker if we do something like:
> 
> -        VMSTATE_BUFFER(cmdbuf, ESPState),
> +        VMSTATE_PARTIAL_BUFFER_TEST(cmdbuf, ESPState, 16, v_is_3),
> +        VMSTATE_BUFFER_TEST(cmdbuf, ESPState, from_4),
> 
> Yes, VMSTATE_PARTIAL_BUFFER_TEST don't exist, but it is trivial to
> define.

Yea, I think it'll help.  But I'm also thinking what else we do in
code that we can offload to vmstate.  Or if there's something that we
just don't express in vmstate yet.

		Amit