[Qemu-devel] Is block_save_iterate() dead code? (was: migration: Fix return code of ram_save_iterate() )

[Thomas Huth]

On 18.11.2016 09:13, Thomas Huth wrote:
> On 17.11.2016 04:45, David Gibson wrote:
>> On Mon, Nov 14, 2016 at 07:34:59PM +0100, Juan Quintela wrote:
>>> Thomas Huth <address@hidden> 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>
>>>
>>> Reviewed-by: Juan Quintela <address@hidden>
>>>
>>> Applied.
>>>
>>> I don't know how we broked this so much.
>>
>> Note that block save iterate has the same bug...
> 
> I think you're right. Care to send a patch?

Looking at this issue again ... could it be that block_save_iterate() is
currently just dead code?
As far as I can see, the ->save_live_iterate() handlers are only called
from qemu_savevm_state_iterate(), right? And qemu_savevm_state_iterate()
only calls the handlers if se->ops->is_active(se->opaque) returns true.
But block_is_active() seems to only return 0 during savevm, most likely
because qemu_savevm_state() explicitly sets the "blk" and "shared"
MigrationParams to zero.
So to me, it looks like we could also just remove block_save_iterate()
completely ... or did I miss something here?

 Thomas

signature.asc
Description: OpenPGP digital signature