Re: [PATCH v3] vmstate: fix subsection load name check

[Fabiano Rosas <farosas@suse.de>]

Alexandr Moshkov <dtalexundeer@yandex-team.ru> writes:

Sorry, but I'll have to nitpick on the commit message, I can't parse
this without looking at the code.

> When loading a subset, its name is checked for the parent prefix. The

What do you mean by subset?

> following bug may occur here:
>
> Let's say there is a vmstate named "virtio-blk", it has a subsection
> named "virtio-blk/subsection", and it also has another vmstate named
> "virtio" in the fields.
> Then, during the migration, when trying to load this subsection for

What is "this subsection" in this context? 'virtio-blk/subsection' ? Or do
you mean the 'virtio' field.

> "virtio", the prefix condition will pass for "virtio-blk/subsection" and

What's "the prefix condition" exactly? Could you be more explicit?

> then the migration will break, because this vmstate does not have such a
> subsection.
>
> In other words, if a field inside vmstate1 is set via vmstate2 with a
> name that is a prefix of the parent vmstate, then the field can "steal"
> a subsection belonging to the parent state.
>

This is what I read from the above paragraph. Is this it?

vmstate1 "abc"         vmstate2 "abcdef"
  field -> vmstate2
  subsection

> Looks like it happens because migration stream for "virtio-blk" looks
> like this:
>
>  [virtio-blk header] [virtio-blk fields] [virtio-blk subsections]
>
> "virtio-blk" contains "virtio" field, so migration stream is:
>
>  [virtio-blk header] [virtio header] [virtio fields] [virtio
> subsections] [virtio-blk subsections]
>

So the code sees the [virtio-blk subsections] as a continuation of the
preceding [virtio subsections] because the string "virtio", which is the
parent of [virtio subsection] is matched in the string "virtio-blk"?

> And when we load the subsections of the "virtio" device,
> vmstate_subsection_load() uses qemu_peek_byte() to try to figure out if
> this is his subsection. This is where we encounter an error.
>
> Thus, the error occurs due to the fact that vmsd does not know how many
> subsections it has when loading (this does not appear anywhere in the
> migration stream), so it tries to load all the appropriate ones by
> names.
>
> To fix this issue, we call vmstate_get_subsection() and, in case of
> fail, ignore it, pop the VMSD stack and will try to define this
> subsection for outer VMSD
>
> Signed-off-by: Alexandr Moshkov <dtalexundeer@yandex-team.ru>
> ---
>  migration/vmstate.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index 4d28364f7b..8f3a69c26e 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -639,9 +639,7 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
>          sub_vmsd = vmstate_get_subsection(vmsd->subsections, idstr);
>          if (sub_vmsd == NULL) {
>              trace_vmstate_subsection_load_bad(vmsd->name, idstr, "(lookup)");
> -            error_setg(errp, "VM subsection '%s' in '%s' does not exist",
> -                       idstr, vmsd->name);
> -            return -ENOENT;
> +            return 0;
>          }
>          qemu_file_skip(f, 1); /* subsection */
>          qemu_file_skip(f, 1); /* len */