Re: [Qemu-devel] [PATCH for-2.8] migration: Fix return code of ram_save_iterate()

[Thomas Huth]

On 08.11.2016 02:14, David Gibson wrote:
> On Fri, Nov 04, 2016 at 02:10:17PM +0100, Thomas Huth wrote:
>> qemu_savevm_state_iterate() expects the iterators to return 1
>> when they are done, and 0 if there is still something left to do.
>> However, ram_save_iterate() does not obey this rule and returns
>> the number of saved pages instead. This causes a fatal hang with
>> ppc64 guests when you run QEMU like this (also works with TCG):
>>
>>  qemu-img create -f qcow2  /tmp/test.qcow2 1M
>>  qemu-system-ppc64 -nographic -nodefaults -m 256 \
>>                    -hda /tmp/test.qcow2 -serial mon:stdio
>>
>> ... then switch to the monitor by pressing CTRL-a c and try to
>> save a snapshot with "savevm test1" for example.
>>
>> After the first iteration, ram_save_iterate() always returns 0 here,
>> so that qemu_savevm_state_iterate() hangs in an endless loop and you
>> can only "kill -9" the QEMU process.
>> Fix it by using proper return values in ram_save_iterate().
>>
>> Signed-off-by: Thomas Huth <address@hidden>
> 
> Hmm.  I think the change is technically correct, but I'm uneasy with
> this approach to the solution.  The whole reason this wasn't caught
> earlier is that almost nothing looks at the return value.  Without
> changing that I think it's very likely someone will mess this up
> again.
> 
> I think it would be preferable to change the return type to void to
> make it explicit that this function is not directly returning the
> "completion" status, but instead that's calculated from the other
> progress variables it updates.

Not sure how such a patch should finally look like. Could you propose a
patch?

Anyway, we're in soft freeze already ... do we still want such a major
change of the logic at this point in time? If not, we should maybe go
with fixing the return type only for 2.8, and do the major change for
2.9 instead?

 Thomas

signature.asc
Description: OpenPGP digital signature