Re: [Qemu-devel] [PATCH v2 2/2] vl: convert -m to QemuOpts

[Eric Blake <eblake@redhat.com>]

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

On 02/13/2014 06:50 AM, Igor Mammedov wrote:
> Adds option to -m
>  "mem" - startup memory amount
> 
> For compatibility with legacy CLI if suffix-less number is passed,
> it assumes amount in Mb.
> 
> Otherwise user is free to use suffixed number using suffixes b,k/K,M,G
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> v2:
>  - various fixes suggested by Laszlo Ersek <lersek@redhat.com>
>    - check for overflow
>    - check for empty mem="" case
>    - reshuffle code to avoid conflicts with another series on list
>    - some style/typo fixes
> ---
>  qemu-options.hx |   12 ++++++---
>  vl.c            |   68 ++++++++++++++++++++++++++++++++++++++++++++++---------
>  2 files changed, 65 insertions(+), 15 deletions(-)
> 

> +    "                configure guest RAM size\n"
> +    "                mem: initial amount of guest memory (default: "
> +    stringify(DEFAULT_RAM_SIZE) "MB)\n",

MB here...

> +    QEMU_ARCH_ALL)
>  STEXI
>  @item -m @var{megs}
>  @findex -m
> -Set virtual RAM size to @var{megs} megabytes. Default is 128 MiB.  Optionally,
> +Set virtual RAM size to @var{size} megabytes. Default is 128 MiB.  Optionally,
>  a suffix of ``M'' or ``G'' can be used to signify a value in megabytes or
> -gigabytes respectively.
> +gigabytes respectively. If @var{size} is provided without unit suffix then MB
> +is assumed.

...and inconsistent on MiB vs MB here.  If we're going to use MiB (which
is the correct term), it would be worth using it everywhere.


> +                opts = qemu_opts_parse(qemu_find_opts(MEMORY_OPTS),
> +                                       optarg, 1);
> +                if (!opts) {
> +                    exit(1);

I prefer EXIT_FAILURE, but that's a change for another patch (you're
just following conventions).

> +                if (!strlen(mem_str)) {

More efficient as 'if (!*mem_str)'...

> +                    fprintf(stderr, "qemu: missing 'mem' option value\n");
>                      exit(1);
>                  }
> -                sz = QEMU_ALIGN_UP((uint64_t)value, 8192);
> +
> +                sz = qemu_opt_get_size(opts, "mem", ram_size);
> +
> +                /* Fix up legacy suffix-less format */
> +                if (g_ascii_isdigit(mem_str[strlen(mem_str) - 1])) {

...either that, or cache your strlen() result so you only call it once.

> +                    uint64_t overflow_check = sz;
> +
> +                    sz <<= 20;
> +                    if ((sz >> 20) != overflow_check) {
> +                        fprintf(stderr, "qemu: too large 'mem' option "
> +                                "value\n");
> +                    }
> +                }

What? We print the warning, but then use the bogus value?  Missing exit().

-- 
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 --]