From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39158) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XTQb4-0001uG-H4 for qemu-devel@nongnu.org; Mon, 15 Sep 2014 03:21:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XTQaz-0003cp-9q for qemu-devel@nongnu.org; Mon, 15 Sep 2014 03:21:14 -0400 Received: from szxga01-in.huawei.com ([119.145.14.64]:54776) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XTQay-0003bF-23 for qemu-devel@nongnu.org; Mon, 15 Sep 2014 03:21:09 -0400 Message-ID: <5416920E.30803@huawei.com> Date: Mon, 15 Sep 2014 15:15:26 +0800 From: zhanghailiang MIME-Version: 1.0 References: <1410501536-1516-1-git-send-email-zhang.zhanghailiang@huawei.com> <20140912155209.22ffc535@nial.usersys.redhat.com> In-Reply-To: <20140912155209.22ffc535@nial.usersys.redhat.com> Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/2] vl: Fix the confused logic for '-m' option List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: luonengjun@huawei.com, qemu-devel@nongnu.org, aliguori@amazon.com, peter.huangpeng@huawei.com On 2014/9/12 21:52, Igor Mammedov wrote: > On Fri, 12 Sep 2014 13:58:55 +0800 > zhanghailiang wrote: > >> It should be valid for the follow configure: >> -m 256,slots=0 >> -m 256,maxmem=256M > Doc comment/help says that slots& maxmem must be in pair so above is not valid CLI > " Hmm, yes, you are right;), Sorry for the noise. > -m[emory] [size=]megs[,slots=n,maxmem=size] > configure guest RAM > size: initial amount of guest memory (default: 128MiB) > slots: number of hotplug slots (default: none) > maxmem: maximum amount of guest memory (default: none) > " > >> -m 256,slots=0,maxmem=256M >> -m 256,slots=x,maxmem=y where x> 0 and y> 256M > s/-m 256,slots=x,maxmem=y where x> 0 and y> 256M/-m z,slots=x,maxmem=y where x> 0 and y> z(M) > >> >> Fix the confused code logic and use error_report instead of fprintf. >> >> Printing the maxmem in hex, same with ram_size. >> >> Signed-off-by: zhanghailiang >> --- >> vl.c | 46 +++++++++++++++++++++++++++------------------- >> 1 file changed, 27 insertions(+), 19 deletions(-) >> >> diff --git a/vl.c b/vl.c >> index 9c9acf5..f547405 100644 >> --- a/vl.c >> +++ b/vl.c >> @@ -3306,6 +3306,7 @@ int main(int argc, char **argv, char **envp) >> break; >> case QEMU_OPTION_m: { >> uint64_t sz; >> + uint64_t slots; >> const char *mem_str; >> const char *maxmem_str, *slots_str; >> >> @@ -3353,40 +3354,47 @@ int main(int argc, char **argv, char **envp) >> >> maxmem_str = qemu_opt_get(opts, "maxmem"); >> slots_str = qemu_opt_get(opts, "slots"); >> - if (maxmem_str&& slots_str) { >> - uint64_t slots; >> - >> + if (maxmem_str) { >> sz = qemu_opt_get_size(opts, "maxmem", 0); >> + } >> + if (slots_str) { >> + slots = qemu_opt_get_number(opts, "slots", 0); >> + } >> + if (maxmem_str&& slots_str) { >> if (sz< ram_size) { >> - fprintf(stderr, "qemu: invalid -m option value: maxmem " >> - "(%" PRIu64 ")<= initial memory (" >> + error_report("qemu: invalid -m option value: maxmem " >> + "(%" PRIx64 ")< initial memory (" > it may be worth to add 0x before hex number, > maybe printing maxmem_str instead of number here would be better, since > user would able to match easily maxmem value and what is printed in error message. > Good idea, will fix it. > >> RAM_ADDR_FMT ")\n", sz, ram_size); >> exit(EXIT_FAILURE); >> } >> - >> - slots = qemu_opt_get_number(opts, "slots", 0); >> - if ((sz> ram_size)&& !slots) { >> - fprintf(stderr, "qemu: invalid -m option value: maxmem " >> - "(%" PRIu64 ") more than initial memory (" >> + if (!slots&& (sz != ram_size)) { > it would report wrong message in case of "-m 256,slots=0,maxmem=128M" > No, in this case, actually it will go into the above *if (sz< ram_size) {* branch statements and reports "qemu: invalid -m option value: maxmem (8000000) < initial memory (10000000)":) >> + error_report("qemu: invalid -m option value: maxmem " >> + "(%" PRIx64 ") more than initial memory (" >> RAM_ADDR_FMT ") but no hotplug slots where " >> "specified\n", sz, ram_size); >> exit(EXIT_FAILURE); >> } >> - >> - if ((sz<= ram_size)&& slots) { >> - fprintf(stderr, "qemu: invalid -m option value: %" >> + if (slots&& (sz == ram_size)) { > What takes care about the case "-m 256,slots=1,maxmem=128M"? > Same with the above explain. >> + error_report("qemu: invalid -m option value: %" >> PRIu64 " hotplug slots where specified but " >> - "maxmem (%" PRIu64 ")<= initial memory (" >> + "maxmem (%" PRIx64 ") = initial memory (" >> RAM_ADDR_FMT ")\n", slots, sz, ram_size); >> exit(EXIT_FAILURE); >> } >> maxram_size = sz; >> ram_slots = slots; >> - } else if ((!maxmem_str&& slots_str) || >> - (maxmem_str&& !slots_str)) { >> - fprintf(stderr, "qemu: invalid -m option value: missing " >> - "'%s' option\n", slots_str ? "maxmem" : "slots"); >> - exit(EXIT_FAILURE); >> + } else if (!maxmem_str&& slots_str) { >> + if (slots> 0) { >> + error_report("qemu: invalid -m option value: missing " >> + "'maxmem' option\n"); >> + exit(EXIT_FAILURE); >> + } >> + } else if (maxmem_str&& !slots_str) { >> + if (sz != ram_size) { >> + error_report("qemu: invalid -m option value: missing " >> + "'slot' option\n"); >> + exit(EXIT_FAILURE); >> + } >> } >> break; >> } > > > . >