Re: [Qemu-devel] [PATCH 1/2] vl.c: Fix error messages when parsing maxmem parameters

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3019 bytes --]

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 <pkrempa@redhat.com>
> ---
>  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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]