qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH for-2.8] migration: Fix return code of ram_save_


From: David Gibson
Subject: Re: [Qemu-devel] [PATCH for-2.8] migration: Fix return code of ram_save_iterate()
Date: Tue, 8 Nov 2016 12:14:04 +1100
User-agent: Mutt/1.7.1 (2016-10-04)

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.

> ---
>  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;
>  }
>  
>  /* Called with iothread lock */

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

Attachment: signature.asc
Description: PGP signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]