Re: [Qemu-devel] [PATCH] migration: Correctly handle subsections with no 'needed' function

[Juan Quintela]

Peter Maydell <address@hidden> wrote:
> On 7 August 2018 at 14:17, Juan Quintela <address@hidden> wrote:
>> Peter Maydell <address@hidden> wrote:
>>> Currently the vmstate subsection handling code treats a subsection
>>> with no 'needed' function pointer as if it were the subsection
>>> list terminator, so the subsection is never transferred and nor
>>> is any subsection following it in the list.
>
>>> @@ -486,8 +486,8 @@ static int vmstate_subsection_save(QEMUFile *f, const 
>>> VMStateDescription *vmsd,
>>>      int ret = 0;
>>>
>>>      trace_vmstate_subsection_save_top(vmsd->name);
>>> -    while (sub && *sub && (*sub)->needed) {
>>> -        if ((*sub)->needed(opaque)) {
>>> +    while (sub && *sub) {
>>> +        if (vmstate_save_needed(*sub, opaque)) {
>>>              const VMStateDescription *vmsdsub = *sub;
>>>              uint8_t len;
>>
>>
>> I am not so sure about this one.  Why are we having a subsection without
>> a ->needed function?  I don't know why that it is useful for.  if we
>> don't have a needed function, then it is just better to just add that
>> "subsection" to the normal section, increase the version number and live
>> with that, no?
>
> No, because then you can't migrate from an older QEMU without
> the subsection to one with it. AIUI version-number bumps are
> deprecated if you care about migration compatibility, because
> they don't work if you're potentially going to be backporting
> changes to older versions.
>
> Also, what prompted me to write this patch was that for 3.0
> I had to fix several bugs where subsections had been written
> with no needed function and the migration code was (a)
> silently ignoring the subsection and (b) silently ignoring
> any subsection following. If you want to make it a bug to

b) is clearly a bug.

a) ... I am not sure that this should be correct.

> not have a needed function, you need to have code that checks
> for this in device init and aborts, so that device authors
> don't fall into this trap.

Here you have a good point.

Later, Juan.