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

[Amit Shah]

On (Fri) 04 Nov 2016 [14:10:17], 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):

"works with" -- does that mean reproduces with?

>  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>
> ---
>  migration/ram.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index fb9252d..a1c8089 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1987,7 +1987,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>      int ret;
>      int i;
>      int64_t t0;
> -    int pages_sent = 0;
> +    int done = 0;
>  
>      rcu_read_lock();
>      if (ram_list.version != last_version) {
> @@ -2007,9 +2007,9 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>          pages = ram_find_and_save_block(f, false, &bytes_transferred);
>          /* no more pages to sent */
>          if (pages == 0) {
> +            done = 1;
>              break;
>          }
> -        pages_sent += pages;
>          acct_info.iterations++;
>  
>          /* we want to check in the 1st loop, just in case it was the 1st time
> @@ -2044,7 +2044,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>          return ret;
>      }
>  
> -    return pages_sent;
> +    return done;
>  }

I agree with David, we can just remove the return value.  The first
patch of the series can do that; and this one could become the 2nd
patch.  Should be OK for the soft freeze.

                Amit