From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40727) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wx042-0006pC-3o for qemu-devel@nongnu.org; Tue, 17 Jun 2014 16:33:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Wx03x-0007nu-5E for qemu-devel@nongnu.org; Tue, 17 Jun 2014 16:33:06 -0400 Received: from mx1.redhat.com ([209.132.183.28]:43170) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wx03w-0007no-Tr for qemu-devel@nongnu.org; Tue, 17 Jun 2014 16:33:01 -0400 Message-ID: <53A0A5F5.4060505@redhat.com> Date: Tue, 17 Jun 2014 14:32:53 -0600 From: Eric Blake MIME-Version: 1.0 References: <1402888539-14961-1-git-send-email-quintela@redhat.com> <1402888539-14961-7-git-send-email-quintela@redhat.com> <539F2A0A.40401@redhat.com> <53A00AEF.2090304@kamp.de> In-Reply-To: <53A00AEF.2090304@kamp.de> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="WauOl3nS1EMsCiqhw50DAf3rPLxSubPDi" Subject: Re: [Qemu-devel] [PATCH 6/6] migration: catch unknown flags in ram_load List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Lieven , Juan Quintela , qemu-devel@nongnu.org, Amit Shah This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --WauOl3nS1EMsCiqhw50DAf3rPLxSubPDi Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 06/17/2014 03:31 AM, Peter Lieven wrote: >> Among other things, switching from a chain of if-else to a switch migh= t >> make it easier to document explicit supported combinations of flags an= d >> reject other values from an invalid stream. >> >=20 > Is this what you have in mind? >=20 > diff --git a/arch_init.c b/arch_init.c > index 8ddaf35..925cc66 100644 > --- a/arch_init.c > +++ b/arch_init.c > @@ -1039,7 +1039,7 @@ void ram_handle_compressed(void *host, uint8_t ch= , > uint64_t size) > static int ram_load(QEMUFile *f, void *opaque, int version_id) > { > ram_addr_t addr; > - int flags, ret =3D 0; > + int flags =3D 0, ret =3D 0; > static uint64_t seq_iter; >=20 > seq_iter++; > @@ -1048,97 +1048,96 @@ static int ram_load(QEMUFile *f, void *opaque, > int version_id) > ret =3D -EINVAL; > } >=20 > - while (!ret) { > + while (!ret && !(flags & RAM_SAVE_FLAG_EOS)) { > - ret =3D -EINVAL; > + if (flags & RAM_SAVE_FLAG_HOOK) { > + ram_control_load_hook(f, flags); Can RAM_SAVE_FLAG_HOOK | RAM_SAVE_FLAG_EOS ever be true? If so, you abort the loop early. (One possibility: solve it by adding 'flags =3D 0;'= here.) Also, this patch was harder to read than necessary, because it reindented everything, and threw off git's diff algorithm. Use 'git diff --patience' to make it a bit more legible as a bulk delete/replace instead of dividing into hunks around common but unrelated context lines. Or even rewrite it slightly so that the bulk of the code is still at the same indentation, perhaps by rearranging this if statement to instead appear inside the default label of the switch: while (!ret && !(flags & RAM_SAVE_FLAG_EOS)) { flags =3D ...; switch (flags & ~RAM_SAVE_FLAG_CONTINUE) { ... default: if (flags & RAM_SAVE_FLAG_HOOK) { ram_control_load_hook(f, flags); flags =3D 0; } else { report error on unknown flags } break; } ret =3D qemu_file_get_error(f); } But yes, this is what I had in mind - propose it as a formal patch with S-o-B, and I'll review it. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --WauOl3nS1EMsCiqhw50DAf3rPLxSubPDi Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJToKX1AAoJEKeha0olJ0NqJDEIAJ4bSUvyh5AA0KOeomOtxhia 5Xwy/+vxfATPDv840Bv+4z82IG/Qf3/cOOqOAdg9gwhpc9+PdZQYcI0o37ORw3Lh PB9KtHmVqSB1XpdLOZBSL8BmEbkNKZLwVQcPSViukJ23T4R9G59pILd/d08WcYwQ jV0NIWw2ClisC6lXuJS06Rt3LH0sPtM+VcuCLsu9eKf/v00vLitxITjzeimTmJ8j aYQedC1Hx9BWn0Z9utBz3hGT4d0yMQnzRkJx+J5J9iT37BI3HB2AR5bhOHyX0Y5W 91roJFa/sIVnVS7g60q1jSk3cJkaoRk0P4Jr21cOL5Yw4h4G1VWX/qjKhNvdUlo= =zVDq -----END PGP SIGNATURE----- --WauOl3nS1EMsCiqhw50DAf3rPLxSubPDi--