2011/1/20 Yoshiaki Tamura <tamura.yoshi...@lab.ntt.co.jp>:
> 2011/1/20 Paolo Bonzini <pbonz...@redhat.com>:
>> On 01/20/2011 09:57 AM, Yoshiaki Tamura wrote:
>>>
>>> 2011/1/20 Paolo Bonzini<pbonz...@redhat.com>:
>>>>
>>>> On 12/14/2010 10:07 AM, Yoshiaki Tamura wrote:
>>>>>
>>>>> Although it's rare to happen in live migration, when the head of a
>>>>> byte stream contains 0x05
>>>>
>>>> IIUC, this happens if you have VMS_STRUCT and the field after the
>>>> VMS_STRUCT
>>>> starts with 0x5.
>>>>
>>>> I think you should also add this in vmstate_subsection_load:
>>>>
>>>> sub_vmsd = vmstate_get_subsection(sub, idstr);
>>>> if (sub_vmsd == NULL) {
>>>> return -ENOENT;
>>>> }
>>>> + assert (!sub_vmsd->subsections);
>>>> ret = vmstate_load_state(f, sub_vmsd, opaque, version_id);
>>>>
>>>> and this in vmstate_load_state:
>>>>
>>>> if (field->flags& VMS_STRUCT) {
>>>> + assert (!vmsd->subsections);
>>>> ret = vmstate_load_state(f, field->vmsd, addr,
>>>> field->vmsd->version_id);
>>>> }
>>>
>>> Hi Paolo,
>>>
>>> You mean, having subsection nested and under VMS_STRUCT are
>>> violations?
>>
>> I believe so, because the protocol doesn't allow you to distinguish:
>>
>> - in the case of nested subsections, whether 2 consecutive subsections are
>> siblings, or the second is nested into the first. In fact, your patch also
>> fixes a latent bug in case a device supports more than one subsection, and
>> both are present in the data stream. When vmstate_subsection_load is called
>> for the first subsection, it would see a 0x5 byte (the beginning of the
>> second subsection), eat it and then fail with ENOENT. The second subsection
>> would then fail to load.
>>
>> - in the case of VMS_STRUCT, whether a 0x5 byte after the VMS_STRUCT is a
>> subsection or part of the parent data stream. This is, I believe, the
>> source of your bug.
>
> Thank you for the explanation. It's very helpful because I
> didn't know the background of subsection. Kemari is kind of
> stress test of live migration.
>
>> I don't think it is possible to fix these problems in the file format while
>> preserving backwards compatibility with pre-subsection QEMU (which was a
>> fundamental requirement of subsections). So, I think your patch is correct
>> and fixes the practical bugs. However, we can be even stronger and assert
>> that the problematic vmstate descriptions are not used.
>>
>> Even better, asserts matching the ones above could be added to
>> vmstate_subsection_save and vmstate_save_state, as well.
if (field->flags & VMS_STRUCT) {
+ assert (!vmsd->subsections);
ret = vmstate_load_state(f, field->vmsd, addr,
field->vmsd->version_id);
}
This assertion always fail for "cpu" in my environment.
Yoshi
> I see. Let me fold the assertion you pointed to the original
> patch for now. Because I'm not an expert in subsection, I would
> like to leave further improvements to the others.
>
> Yoshi
>
>>
>> Paolo
>>
>>
>
