[2/2] vl: convert -m to QemuOpts

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>
---
 qemu-options.hx |    7 +++++--
 vl.c            |   53 ++++++++++++++++++++++++++++++++++++++++++-----------
 2 files changed, 47 insertions(+), 13 deletions(-)

comments below

On 02/06/14 09:16, 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>
> ---
>  qemu-options.hx |    7 +++++--
>  vl.c            |   53 ++++++++++++++++++++++++++++++++++++++++++-----------
>  2 files changed, 47 insertions(+), 13 deletions(-)
> 
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 56e5fdf..4d7ef52 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -210,8 +210,11 @@ use is discouraged as it may be removed from future versions.
>  ETEXI
>  
>  DEF("m", HAS_ARG, QEMU_OPTION_m,
> -    "-m megs         set virtual RAM size to megs MB [default="
> -    stringify(DEFAULT_RAM_SIZE) "]\n", QEMU_ARCH_ALL)
> +    "-m [mem=]megs\n"
> +    "                configure guest RAM\n"

"configure guest RAM size"

> +    "                mem: initial amount of guest memory (default: "
> +    stringify(DEFAULT_RAM_SIZE) "Mb)\n",

I wonder if it should rather say "MB" -- small "b" has this "bits"
connotation for me. But I could be wrong.

Also, again, I believe explaining the default used to mean something
else, but I'm OK with that part as-is.

> +    QEMU_ARCH_ALL)
>  STEXI
>  @item -m @var{megs}
>  @findex -m
> diff --git a/vl.c b/vl.c
> index 7f2595c..fe5dae3 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -532,6 +532,20 @@ static QemuOptsList qemu_msg_opts = {
>      },
>  };

(this could conflict with Alan's series -- modifies the same spot)

> +static QemuOptsList qemu_mem_opts = {
> +    .name = "memory-opts",
> +    .implied_opt_name = "mem",
> +    .head = QTAILQ_HEAD_INITIALIZER(qemu_mem_opts.head),
> +    .merge_lists = true,

OK, so we've set merge_list to true here as well, same as for "machine".
Further support for simplifying qemu_find_opts_singleton(); see patch #1.

> +    .desc = {
> +        {
> +            .name = "mem",
> +            .type = QEMU_OPT_SIZE,

QEMU_OPT_SIZE implies (in parse_option_size()) that "no suffix" means
"unit==byte" (); I'll check lower down how that's solved.

> +        },
> +        { /* end of list */ }
> +    },
> +};
> +
>  /**
>   * Get machine options
>   *
> @@ -2868,6 +2882,7 @@ int main(int argc, char **argv, char **envp)
>      };
>      const char *trace_events = NULL;
>      const char *trace_file = NULL;
> +    const ram_addr_t default_ram_size = DEFAULT_RAM_SIZE * 1024 * 1024;

I'd feel safer if the multiplications were done in ram_addr_t. Currently
they are done in "int". It's unlikely that we'll make 2GB+ the default
ram size, but still.

>  
>      atexit(qemu_run_exit_notifiers);
>      error_set_progname(argv[0]);
> @@ -2906,6 +2921,7 @@ int main(int argc, char **argv, char **envp)
>      qemu_add_opts(&qemu_tpmdev_opts);
>      qemu_add_opts(&qemu_realtime_opts);
>      qemu_add_opts(&qemu_msg_opts);
> +    qemu_add_opts(&qemu_mem_opts);
>  
>      runstate_init();
>  
> @@ -2921,7 +2937,7 @@ int main(int argc, char **argv, char **envp)
>      module_call_init(MODULE_INIT_MACHINE);
>      machine = find_default_machine();
>      cpu_model = NULL;
> -    ram_size = 0;
> +    ram_size = default_ram_size;
>      snapshot = 0;
>      cyls = heads = secs = 0;
>      translation = BIOS_ATA_TRANSLATION_AUTO;
> @@ -3198,16 +3214,32 @@ int main(int argc, char **argv, char **envp)
>                  exit(0);
>                  break;
>              case QEMU_OPTION_m: {
> -                int64_t value;
>                  uint64_t sz;
> -                char *end;
> +                const char *mem_str;
>  
> -                value = strtosz(optarg, &end);
> -                if (value < 0 || *end) {
> -                    fprintf(stderr, "qemu: invalid ram size: %s\n", optarg);
> +                opts = qemu_opts_parse(qemu_find_opts("memory-opts"),
> +                                       optarg, 1);

This can set "opts" to NULL if parsing fails, and then the
qemu_opt_get() just below will SIGSEGV. You need to check if "opts"
becomes NULL here, and exit if so (see other calls to qemu_opts_parse()
in main()).

In particular, see commit f46e720a.

Also, unfortunately, this conversion kind of relaxes the error checking
that happens during parsing. The pre-patch version ends up in
strtosz_suffix_unit(), which rejects the empty string, for example. The
new version, which ends up in parse_option_size(), is not that smart
about strtod(). I think it will simply return zero for

  -m mem=""

However it's not the fault of this patch.

> +
> +                mem_str = qemu_opt_get(opts, "mem");
> +                if (!mem_str) {
> +                    fprintf(stderr, "qemu: invalid -m option, missing "
> +                            " 'mem' option\n");

Double space. (There's one at the end of the first string literal, and
another at the beginning of the second literal.)

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

Undefined behavior if mem_str is the emptry string. (I think it is
possible, but I didn't test it.)

> +                    sz <<= 20;
> +                }

We could check for overflow here, if we wanted.

> +
> +                /* backward compatibility behaviour for case "-m 0" */
> +                if (sz == 0) {
> +                    sz = default_ram_size;
> +                }
> +
> +                sz = QEMU_ALIGN_UP(sz, 8192);
>                  ram_size = sz;
>                  if (ram_size != sz) {
>                      fprintf(stderr, "qemu: ram size too large\n");
> @@ -4056,10 +4088,9 @@ int main(int argc, char **argv, char **envp)
>          exit(1);
>      }
>  
> -    /* init the memory */
> -    if (ram_size == 0) {
> -        ram_size = DEFAULT_RAM_SIZE * 1024 * 1024;
> -    }
> +    /* store value for the future use */
> +    qemu_opt_set_number(qemu_find_opts_singleton("memory-opts"),
> +                        "mem", ram_size);

Slight possibility here to overflow the int64_t "val" parameter with the
potentially uint64_t "ram_size" argument. I guess we don't care.

Also, I wonder what happens when we have passed a non-default memory
size on the command line. In that case, qemu_opt_set_number() seems to
create a second QemuOpt here. I guess that's maybe expected though?

>  
>      if (qemu_opts_foreach(qemu_find_opts("device"), device_help_func, NULL, 0)
>          != 0) {
> 

It's your call what you'd like to address from the above.

Thanks
Laszlo

On Mon, 10 Feb 2014 18:38:59 +0100
Laszlo Ersek <lersek@redhat.com> wrote:

> comments below
> 
> On 02/06/14 09:16, 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>
> > ---
> >  qemu-options.hx |    7 +++++--
> >  vl.c            |   53 ++++++++++++++++++++++++++++++++++++++++++-----------
> >  2 files changed, 47 insertions(+), 13 deletions(-)
> > 
> > diff --git a/qemu-options.hx b/qemu-options.hx
> > index 56e5fdf..4d7ef52 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -210,8 +210,11 @@ use is discouraged as it may be removed from future versions.
> >  ETEXI
> >  
> >  DEF("m", HAS_ARG, QEMU_OPTION_m,
> > -    "-m megs         set virtual RAM size to megs MB [default="
> > -    stringify(DEFAULT_RAM_SIZE) "]\n", QEMU_ARCH_ALL)
> > +    "-m [mem=]megs\n"
> > +    "                configure guest RAM\n"
> 
> "configure guest RAM size"
fixed

> 
> > +    "                mem: initial amount of guest memory (default: "
> > +    stringify(DEFAULT_RAM_SIZE) "Mb)\n",
> 
> I wonder if it should rather say "MB" -- small "b" has this "bits"
> connotation for me. But I could be wrong.
fixed

> 
> Also, again, I believe explaining the default used to mean something
> else, but I'm OK with that part as-is.
> 
> > +    QEMU_ARCH_ALL)
> >  STEXI
> >  @item -m @var{megs}
> >  @findex -m
> > diff --git a/vl.c b/vl.c
> > index 7f2595c..fe5dae3 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -532,6 +532,20 @@ static QemuOptsList qemu_msg_opts = {
> >      },
> >  };
> 
> (this could conflict with Alan's series -- modifies the same spot)
I'll put it in the beginning of list to avoid conflict.

> > +static QemuOptsList qemu_mem_opts = {
> > +    .name = "memory-opts",
> > +    .implied_opt_name = "mem",
> > +    .head = QTAILQ_HEAD_INITIALIZER(qemu_mem_opts.head),
> > +    .merge_lists = true,
> 
> OK, so we've set merge_list to true here as well, same as for "machine".
> Further support for simplifying qemu_find_opts_singleton(); see patch #1.
I believe it's not good for generic function to depend on merge_lists.

> 
> > +    .desc = {
> > +        {
> > +            .name = "mem",
> > +            .type = QEMU_OPT_SIZE,
> 
> QEMU_OPT_SIZE implies (in parse_option_size()) that "no suffix" means
> "unit==byte" (); I'll check lower down how that's solved.
it's(i.e. legacy behavior) taken care of later at -m parsing time

> 
> > +        },
> > +        { /* end of list */ }
> > +    },
> > +};
> > +
> >  /**
> >   * Get machine options
> >   *
> > @@ -2868,6 +2882,7 @@ int main(int argc, char **argv, char **envp)
> >      };
> >      const char *trace_events = NULL;
> >      const char *trace_file = NULL;
> > +    const ram_addr_t default_ram_size = DEFAULT_RAM_SIZE * 1024 * 1024;
> 
> I'd feel safer if the multiplications were done in ram_addr_t. Currently
> they are done in "int". It's unlikely that we'll make 2GB+ the default
> ram size, but still.
fixed

> 
> >  
> >      atexit(qemu_run_exit_notifiers);
> >      error_set_progname(argv[0]);
> > @@ -2906,6 +2921,7 @@ int main(int argc, char **argv, char **envp)
> >      qemu_add_opts(&qemu_tpmdev_opts);
> >      qemu_add_opts(&qemu_realtime_opts);
> >      qemu_add_opts(&qemu_msg_opts);
> > +    qemu_add_opts(&qemu_mem_opts);
> >  
> >      runstate_init();
> >  
> > @@ -2921,7 +2937,7 @@ int main(int argc, char **argv, char **envp)
> >      module_call_init(MODULE_INIT_MACHINE);
> >      machine = find_default_machine();
> >      cpu_model = NULL;
> > -    ram_size = 0;
> > +    ram_size = default_ram_size;
> >      snapshot = 0;
> >      cyls = heads = secs = 0;
> >      translation = BIOS_ATA_TRANSLATION_AUTO;
> > @@ -3198,16 +3214,32 @@ int main(int argc, char **argv, char **envp)
> >                  exit(0);
> >                  break;
> >              case QEMU_OPTION_m: {
> > -                int64_t value;
> >                  uint64_t sz;
> > -                char *end;
> > +                const char *mem_str;
> >  
> > -                value = strtosz(optarg, &end);
> > -                if (value < 0 || *end) {
> > -                    fprintf(stderr, "qemu: invalid ram size: %s\n", optarg);
> > +                opts = qemu_opts_parse(qemu_find_opts("memory-opts"),
> > +                                       optarg, 1);
> 
> This can set "opts" to NULL if parsing fails, and then the
> qemu_opt_get() just below will SIGSEGV. You need to check if "opts"
> becomes NULL here, and exit if so (see other calls to qemu_opts_parse()
> in main()).
fixed

> 
> In particular, see commit f46e720a.
> 
> Also, unfortunately, this conversion kind of relaxes the error checking
> that happens during parsing. The pre-patch version ends up in
> strtosz_suffix_unit(), which rejects the empty string, for example. The
> new version, which ends up in parse_option_size(), is not that smart
> about strtod(). I think it will simply return zero for
> 
>   -m mem=""
> 
> However it's not the fault of this patch.
parse_option_size() does return 0, but following code fixes it,
look for comment: /* backward compatibility behavior for case "-m 0" */
but as you pointed below empty mem option causes undefined behavior later
so fix by checking that string is not empty.

> 
> > +
> > +                mem_str = qemu_opt_get(opts, "mem");
> > +                if (!mem_str) {
> > +                    fprintf(stderr, "qemu: invalid -m option, missing "
> > +                            " 'mem' option\n");
> 
> Double space. (There's one at the end of the first string literal, and
> another at the beginning of the second literal.)
fixed

> 
> >                      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])) {
> 
> Undefined behavior if mem_str is the emptry string. (I think it is
> possible, but I didn't test it.)
indeed, fixed.

> 
> > +                    sz <<= 20;
> > +                }
> 
> We could check for overflow here, if we wanted.
fixed

> 
> > +
> > +                /* backward compatibility behaviour for case "-m 0" */
> > +                if (sz == 0) {
> > +                    sz = default_ram_size;
> > +                }
> > +
> > +                sz = QEMU_ALIGN_UP(sz, 8192);
> >                  ram_size = sz;
> >                  if (ram_size != sz) {
> >                      fprintf(stderr, "qemu: ram size too large\n");
> > @@ -4056,10 +4088,9 @@ int main(int argc, char **argv, char **envp)
> >          exit(1);
> >      }
> >  
> > -    /* init the memory */
> > -    if (ram_size == 0) {
> > -        ram_size = DEFAULT_RAM_SIZE * 1024 * 1024;
> > -    }
> > +    /* store value for the future use */
> > +    qemu_opt_set_number(qemu_find_opts_singleton("memory-opts"),
> > +                        "mem", ram_size);
> 
> Slight possibility here to overflow the int64_t "val" parameter with the
> potentially uint64_t "ram_size" argument. I guess we don't care.
yep, everywhere in API *_number is treated as uint64_t and only
qemu_opt_set_number() treats it as int64_t, perhaps this function should be
fixed.

> 
> Also, I wonder what happens when we have passed a non-default memory
> size on the command line. In that case, qemu_opt_set_number() seems to
> create a second QemuOpt here. I guess that's maybe expected though?
which appends it to the tail, and following lookup finds it first.

> 
> >  
> >      if (qemu_opts_foreach(qemu_find_opts("device"), device_help_func, NULL, 0)
> >          != 0) {
> > 
> 
> It's your call what you'd like to address from the above.
> 
> Thanks
> Laszlo

Il 10/02/2014 18:38, Laszlo Ersek ha scritto:
>> +    "                mem: initial amount of guest memory (default: "
>> > +    stringify(DEFAULT_RAM_SIZE) "Mb)\n",
> I wonder if it should rather say "MB" -- small "b" has this "bits"
> connotation for me. But I could be wrong.

Thanks, will fix this.

> Also, again, I believe explaining the default used to mean something
> else, but I'm OK with that part as-is.

What do you mean exactly?  I cannot parse this.

Paolo

On 02/26/14 14:29, Paolo Bonzini wrote:
> Il 10/02/2014 18:38, Laszlo Ersek ha scritto:
>>> +    "                mem: initial amount of guest memory (default: "
>>> > +    stringify(DEFAULT_RAM_SIZE) "Mb)\n",
>> I wonder if it should rather say "MB" -- small "b" has this "bits"
>> connotation for me. But I could be wrong.
> 
> Thanks, will fix this.
> 
>> Also, again, I believe explaining the default used to mean something
>> else, but I'm OK with that part as-is.
> 
> What do you mean exactly?  I cannot parse this.

Sorry, I was ambiguous.

When you have a QemuOpt that takes an argument, then the documentation
of the default behavior usually explains the case when the QemuOpt is
present, and the argument is absent. The documentation of the default
behavior usually doesn't concern the case when the QemuOpt*s* itself is
absent. Cf.

(1) -foo bar
(2) -foo bar=baz
(3) [nothing]

The "default" in the docs tends to explain case (1), not case (3).

In this case we have: -m [mem=]megs

(1) -m mem -- makes no sense
(2) -m mem=megs -- works, and well documented
(3) [nothing] -- is what the proposed docs describe as "default", but it
doesn't match "historical practice".

(1) in general can make sense, eg. for booleans.

Anyway I don't feel strongly about this in the least -- I just thought
I'd point it out. Feel free to ignore it; I have no good suggestion as
to how to make it consistent across all options.

Thanks
Laszlo

Il 26/02/2014 14:42, Laszlo Ersek ha scritto:
> When you have a QemuOpt that takes an argument, then the documentation
> of the default behavior usually explains the case when the QemuOpt is
> present, and the argument is absent. The documentation of the default
> behavior usually doesn't concern the case when the QemuOpt*s* itself is
> absent. Cf.
>
> (1) -foo bar
> (2) -foo bar=baz
> (3) [nothing]
>
> The "default" in the docs tends to explain case (1), not case (3).
>
> In this case we have: -m [mem=]megs
>
> (1) -m mem -- makes no sense
> (2) -m mem=megs -- works, and well documented
> (3) [nothing] -- is what the proposed docs describe as "default", but it
> doesn't match "historical practice".
>
> (1) in general can make sense, eg. for booleans.
>
> Anyway I don't feel strongly about this in the least -- I just thought
> I'd point it out.

Yeah, you're right.

> Feel free to ignore it; I have no good suggestion as
> to how to make it consistent across all options.

Neither do I, but at least "megs" is not marked as an optional element, 
so I think it's not that ambiguous.

Paolo

On Wed, 26 Feb 2014 14:29:54 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 10/02/2014 18:38, Laszlo Ersek ha scritto:
> >> +    "                mem: initial amount of guest memory (default: "
> >> > +    stringify(DEFAULT_RAM_SIZE) "Mb)\n",
> > I wonder if it should rather say "MB" -- small "b" has this "bits"
> > connotation for me. But I could be wrong.
> 
> Thanks, will fix this.
> 
> > Also, again, I believe explaining the default used to mean something
> > else, but I'm OK with that part as-is.
> 
> What do you mean exactly?  I cannot parse this.
> 
> Paolo
> 
> 

It should be fixed in v3 that you were going to apply to numa branch
https://lists.gnu.org/archive/html/qemu-devel/2014-02/msg02321.html

Il 26/02/2014 14:50, Igor Mammedov ha scritto:
> It should be fixed in v3 that you were going to apply to numa branch
> https://lists.gnu.org/archive/html/qemu-devel/2014-02/msg02321.html

Yes, applied now.

Thanks,

Paolo