Re: [Qemu-devel] [PATCH 1/2] vl.c: Fix error messages when parsing maxmem parameters
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.
> >
> > Also remove a redundant check for the max memory size.
> >
> > Signed-off-by: Peter Krempa
> > ---
> > vl.c | 22 +++---
> > 1 file changed, 7 insertions(+), 15 deletions(-)
> >
> > 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_slots,
> > ram_addr_t *maxram_size)
> > uint64_t slots;
> >
> > sz = qemu_opt_get_size(opts, "maxmem", 0);
> > -if (sz < ram_size) {
> > -error_report("invalid -m option value: maxmem "
> > -"(0x%" PRIx64 ") <= initial memory (0x"
> > -RAM_ADDR_FMT ")", sz, ram_size);
> > +if (sz <= ram_size) {
>
> Why are we changing from '<' to '<='? I think the error was in the
> message, not in the code, and that setting max == size should be
> allowed. [1]
>
> > +error_report("invalid value of -m option maxmem: "
> > + "maximum memory size (0x%" PRIx64 ") must be
> > greater "
> > + "than initial memory size (0x" RAM_ADDR_FMT ")",
>
> 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.
>
> > + sz, ram_size);
> > exit(EXIT_FAILURE);
> > }
> >
> > slots = 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
> > specified, "
> > + "but no hotplug slots were specified");
> > exit(EXIT_FAILURE);
> > }
> >
> > -if ((sz <= ram_size) && slots) {
> > -error_report("invalid -m option value: %"
> > -PRIu64 " hotplug slots where specified but "
> > -"maxmem (0x%" PRIx64 ") <= initial memory (0x"
> > -RAM_ADDR_FMT ")", slots, sz, ram_size);
> > -exit(EXIT_FAILURE);
> > -}
>
> Okay, I see. This is dead if condition [1] is changed to '<=', but
> still reachable (sz == ram_size) if condition [1] is left at '<'. Maybe
> better logic would be:
>
> 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
> }
>
> to allow max==ram_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
signature.asc
Description: Digital signature
Re: [Qemu-devel] [PATCH 1/2] vl.c: Fix error messages when parsing maxmem parameters
On 01/26/2015 08:31 AM, Peter Krempa wrote:
> Produce more human readable error messages and fix few spelling
> mistakes.
>
> Also remove a redundant check for the max memory size.
>
> Signed-off-by: Peter Krempa
> ---
> vl.c | 22 +++---
> 1 file changed, 7 insertions(+), 15 deletions(-)
>
> 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_slots,
> ram_addr_t *maxram_size)
> uint64_t slots;
>
> sz = qemu_opt_get_size(opts, "maxmem", 0);
> -if (sz < ram_size) {
> -error_report("invalid -m option value: maxmem "
> -"(0x%" PRIx64 ") <= initial memory (0x"
> -RAM_ADDR_FMT ")", sz, ram_size);
> +if (sz <= ram_size) {
Why are we changing from '<' to '<='? I think the error was in the
message, not in the code, and that setting max == size should be
allowed. [1]
> +error_report("invalid value of -m option maxmem: "
> + "maximum memory size (0x%" PRIx64 ") must be
> greater "
> + "than initial memory size (0x" RAM_ADDR_FMT ")",
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.
> + sz, ram_size);
> exit(EXIT_FAILURE);
> }
>
> slots = 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 specified, "
> + "but no hotplug slots were specified");
> exit(EXIT_FAILURE);
> }
>
> -if ((sz <= ram_size) && slots) {
> -error_report("invalid -m option value: %"
> -PRIu64 " hotplug slots where specified but "
> -"maxmem (0x%" PRIx64 ") <= initial memory (0x"
> -RAM_ADDR_FMT ")", slots, sz, ram_size);
> -exit(EXIT_FAILURE);
> -}
Okay, I see. This is dead if condition [1] is changed to '<=', but
still reachable (sz == ram_size) if condition [1] is left at '<'. Maybe
better logic would be:
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
}
to allow max==ram_size when slots is not present.
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
[Qemu-devel] [PATCH 1/2] vl.c: Fix error messages when parsing maxmem parameters
Produce more human readable error messages and fix few spelling
mistakes.
Also remove a redundant check for the max memory size.
Signed-off-by: Peter Krempa
---
vl.c | 22 +++---
1 file changed, 7 insertions(+), 15 deletions(-)
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_slots,
ram_addr_t *maxram_size)
uint64_t slots;
sz = qemu_opt_get_size(opts, "maxmem", 0);
-if (sz < ram_size) {
-error_report("invalid -m option value: maxmem "
-"(0x%" PRIx64 ") <= initial memory (0x"
-RAM_ADDR_FMT ")", sz, ram_size);
+if (sz <= ram_size) {
+error_report("invalid value of -m option maxmem: "
+ "maximum memory size (0x%" PRIx64 ") must be greater "
+ "than initial memory size (0x" RAM_ADDR_FMT ")",
+ sz, ram_size);
exit(EXIT_FAILURE);
}
slots = 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 specified, "
+ "but no hotplug slots were specified");
exit(EXIT_FAILURE);
}
-if ((sz <= ram_size) && slots) {
-error_report("invalid -m option value: %"
-PRIu64 " hotplug slots where specified but "
-"maxmem (0x%" PRIx64 ") <= initial memory (0x"
-RAM_ADDR_FMT ")", slots, sz, ram_size);
-exit(EXIT_FAILURE);
-}
*maxram_size = sz;
*ram_slots = slots;
} else if ((!maxmem_str && slots_str) ||
--
2.2.1
