From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40255) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YGMvg-0001mr-UZ for qemu-devel@nongnu.org; Wed, 28 Jan 2015 02:20:50 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YGMvd-000325-MM for qemu-devel@nongnu.org; Wed, 28 Jan 2015 02:20:48 -0500 Received: from mx1.redhat.com ([209.132.183.28]:53762) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YGMvd-00031d-ED for qemu-devel@nongnu.org; Wed, 28 Jan 2015 02:20:45 -0500 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t0S7KitY000701 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Wed, 28 Jan 2015 02:20:44 -0500 Date: Wed, 28 Jan 2015 08:18:56 +0100 From: Peter Krempa Message-ID: <20150128071856.GC5003@andariel.home> References: <54C7D5CD.5010406@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="CblX+4bnyfN0pR09" Content-Disposition: inline In-Reply-To: <54C7D5CD.5010406@redhat.com> Subject: Re: [Qemu-devel] [PATCH 1/2] vl.c: Fix error messages when parsing maxmem parameters List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: Igor Mammedov , qemu-devel@nongnu.org --CblX+4bnyfN0pR09 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Jan 27, 2015 at 11:15:41 -0700, Eric Blake wrote: > On 01/26/2015 08:31 AM, Peter Krempa wrote: > > Produce more human readable error messages and fix few spelling > > mistakes. > >=20 > > Also remove a redundant check for the max memory size. > >=20 > > Signed-off-by: Peter Krempa > > --- > > vl.c | 22 +++++++--------------- > > 1 file changed, 7 insertions(+), 15 deletions(-) > >=20 > > diff --git a/vl.c b/vl.c > > index 983259b..cdc920c 100644 > > --- a/vl.c > > +++ b/vl.c > > @@ -2694,29 +2694,21 @@ static void set_memory_options(uint64_t *ram_sl= ots, ram_addr_t *maxram_size) > > uint64_t slots; > >=20 > > sz =3D qemu_opt_get_size(opts, "maxmem", 0); > > - if (sz < ram_size) { > > - error_report("invalid -m option value: maxmem " > > - "(0x%" PRIx64 ") <=3D initial memory (0x" > > - RAM_ADDR_FMT ")", sz, ram_size); > > + if (sz <=3D ram_size) { >=20 > Why are we changing from '<' to '<=3D'? I think the error was in the > message, not in the code, and that setting max =3D=3D size should be > allowed. [1] >=20 > > + error_report("invalid value of -m option maxmem: " > > + "maximum memory size (0x%" PRIx64 ") must be = greater " > > + "than initial memory size (0x" RAM_ADDR_FMT = ")", >=20 > Why two spaces? If I'm correct that we want '<' in the condition, then > the wording 'must be at least the initial memory size' would be better. >=20 > > + sz, ram_size); > > exit(EXIT_FAILURE); > > } > >=20 > > slots =3D qemu_opt_get_number(opts, "slots", 0); > > if ((sz > ram_size) && !slots) { > > - error_report("invalid -m option value: maxmem " > > - "(0x%" PRIx64 ") more than initial memory (0x" > > - RAM_ADDR_FMT ") but no hotplug slots where " > > - "specified", sz, ram_size); > > + error_report("invalid value of -m option: maxmem was speci= fied, " > > + "but no hotplug slots were specified"); > > exit(EXIT_FAILURE); > > } > >=20 > > - if ((sz <=3D ram_size) && slots) { > > - error_report("invalid -m option value: %" > > - PRIu64 " hotplug slots where specified but " > > - "maxmem (0x%" PRIx64 ") <=3D initial memory (0x" > > - RAM_ADDR_FMT ")", slots, sz, ram_size); > > - exit(EXIT_FAILURE); > > - } >=20 > Okay, I see. This is dead if condition [1] is changed to '<=3D', but > still reachable (sz =3D=3D ram_size) if condition [1] is left at '<'. Ma= ybe > better logic would be: >=20 > if (sz < ram_size) { > max cannot be less than memory > } > if (sz > ram_size) { > if (!slots) { > max cannot be larger than size without hotplug slots > } > } else if (slots) { > max must be larger than size to support hotplug slots > } >=20 > to allow max=3D=3Dram_size when slots is not present. This will only allow to specify ram_size equal to max if the slot parameter _is_ present and set to 0. If the string is not present a different branch is taken and the config will be rejected, as the two parameters have to be specified together. I'll post an updated version though, the control flow in your suggestion seems more clear. Thanks. Peter --CblX+4bnyfN0pR09 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBAgAGBQJUyI1gAAoJEDbsFqzQGGgrEd0QAKmB/gDTelZtkx3UE9aKvqpu 5o+5VoAiyQ/yrQVr4d2eDXmxf9JAymt3SuCSb3mJ9WLdkDFJhPcsRmsbZKp98lL3 R43t1DprFs9cW1bOSUM7w2YKsihhtzdWFdqf5gm6x+IxNAc1wUDNoGOoCDk8IsPc Kr7NFOhKA7iZQM1X35V7bgSgiFYa+17OELrXUCXc4lkuxDhRW2GWo/JoAw+y56OR PBRuxaLmIf84jCAWLAwM4d4SG4ZtvDycICfs+4C3N3SCnOgBh6SwY9WJjebfwJF0 q6qVAYAMCR3SDYzyAZA3Gp22Aq4H5ztAJqhcL5YAvrGx/2r/WnmFG1oHX3E0ZuPK qPAZhphgwYQgglMnUrx815PzrCnMXvU1STqrNYZ9U+479/sKu0PIbDJq6syMukld LW3P0YLS4jE27TM52UG7zNJxx1E3yaMNsYjATly56HQP1eg5D1KTrQdk5e3kheFe W1n4Muprfu8tFfm/JF1U5h+6ek9dqr4QaQDtLJBv6EBsTAcIPoojCmVwdtF5ZtB2 WZq6VlX3zpcE5B3NoGHqjt7nm4H9BKJ8HLn+j7OlPhw/aDSkAJ5nWEtT5d8QiElj ge+orH7LyUpq8Kh2ihUYnCbzfng4D527OHhq//gxL98HHHznN9GBbDCSNWT4eVvQ FyZVT9q6DxlE46GuiMSS =hTkL -----END PGP SIGNATURE----- --CblX+4bnyfN0pR09--