From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60459) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c3w10-0005eb-St for qemu-devel@nongnu.org; Mon, 07 Nov 2016 21:19:59 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c3w0x-0003hd-OO for qemu-devel@nongnu.org; Mon, 07 Nov 2016 21:19:58 -0500 Received: from ozlabs.org ([103.22.144.67]:58913) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1c3w0w-0003gy-WD for qemu-devel@nongnu.org; Mon, 07 Nov 2016 21:19:55 -0500 Date: Tue, 8 Nov 2016 12:14:04 +1100 From: David Gibson Message-ID: <20161108011404.GB28688@umbus.fritz.box> References: <1478265017-5700-1-git-send-email-thuth@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="0eh6TmSyL6TZE2Uz" Content-Disposition: inline In-Reply-To: <1478265017-5700-1-git-send-email-thuth@redhat.com> Subject: Re: [Qemu-devel] [PATCH for-2.8] migration: Fix return code of ram_save_iterate() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Thomas Huth Cc: Juan Quintela , Amit Shah , qemu-devel@nongnu.org, "Dr. David Alan Gilbert" --0eh6TmSyL6TZE2Uz Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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): >=20 > qemu-img create -f qcow2 /tmp/test.qcow2 1M > qemu-system-ppc64 -nographic -nodefaults -m 256 \ > -hda /tmp/test.qcow2 -serial mon:stdio >=20 > ... then switch to the monitor by pressing CTRL-a c and try to > save a snapshot with "savevm test1" for example. >=20 > 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(). >=20 > Signed-off-by: Thomas Huth 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(-) >=20 > 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 *opaq= ue) > int ret; > int i; > int64_t t0; > - int pages_sent =3D 0; > + int done =3D 0; > =20 > rcu_read_lock(); > if (ram_list.version !=3D last_version) { > @@ -2007,9 +2007,9 @@ static int ram_save_iterate(QEMUFile *f, void *opaq= ue) > pages =3D ram_find_and_save_block(f, false, &bytes_transferred); > /* no more pages to sent */ > if (pages =3D=3D 0) { > + done =3D 1; > break; > } > - pages_sent +=3D pages; > acct_info.iterations++; > =20 > /* 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 *opaq= ue) > return ret; > } > =20 > - return pages_sent; > + return done; > } > =20 > /* Called with iothread lock */ --=20 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 --0eh6TmSyL6TZE2Uz Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJYISbaAAoJEGw4ysog2bOS6T8P/A+9+DekSgh1JKHdfZl1lmG4 5yWLYGYM72+ad2VSmFJXKTBjMfD5JyJgbPkb9YLwTgaK8nQGrjMOPcTCOu+zw/Qd BWkVwhs9VEPjR0LWWemK0ntVkux2GMOEh1WGRd9LOTUaQsG+jp3BLNMPRBNHeCcF ctyGHbvXQoPFsxDBHFJv/ODs3GK/4rPKhYVTr3kG0wxMYZsx46ZxNfSP6qB3buvc Ibgn9Wvnlfds600Jt20G3/DyMP33c1JqhPzfN9SekU5x1gNUQDnkjAEXrGaXhWsS 1Bq1EGPtvPdSGwfi//O6VqGlP5x45vmizjsK5TVcYQXCmDEOAH6Nq/4QPJjteEnR pHlyqTQfOVcaahuko5i25sRcKVfZhe8rI6+iBtqVpvb7cExjwb2QU7BQ6T8UUsmH 92DhplBfiroWMYNqP7DfNGi8F50I7KpKPxVXr6/GfSUulUR84Rokv2FzINFJfyEw aEwaXEMu5tDdxQWz9D5/YgKSdCBklRRD6enEP64PAXDpdLDW96TPf4s3I1jASGnr nubIq2MQjyMSszP+ggSsIIAaJ216Ewm9fXfwhKXvWYmZA7RZ4/SNKn5nA4PqP/wU j0oVJ4REpkncXWCb+olFj5QiWdpZMD61yPbakraE+IJNsb3Fu4+Fsh2cQIOCQSFT sjwQzQ7ZgkAwDEortHKZ =h+bH -----END PGP SIGNATURE----- --0eh6TmSyL6TZE2Uz--