[Qemu-devel] [PATCH 0/2] convert -m to QemuOpts

[Qemu-devel] [PATCH 0/2] convert -m to QemuOpts

[Igor Mammedov]

changes since RFC:
 - resolved conflict with error_abort change
   in qemu_get_machine_opts()

Igor Mammedov (1):
  vl: convert -m to QemuOpts

Paolo Bonzini (1):
  QemuOpts: introduce qemu_find_opts_singleton

 include/qemu/config-file.h |    2 +
 qemu-options.hx            |    7 +++-
 util/qemu-config.c         |   14 +++++++++
 vl.c                       |   64 +++++++++++++++++++++++++++++--------------
 4 files changed, 64 insertions(+), 23 deletions(-)

[Qemu-devel] [PATCH 1/2] QemuOpts: introduce qemu_find_opts_singleton

[Igor Mammedov]

From: Paolo Bonzini <pbonzini@redhat.com>

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 include/qemu/config-file.h |    2 ++
 util/qemu-config.c         |   14 ++++++++++++++
 vl.c                       |   11 +----------
 3 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/include/qemu/config-file.h b/include/qemu/config-file.h
index dbd97c4..d4ba20e 100644
--- a/include/qemu/config-file.h
+++ b/include/qemu/config-file.h
@@ -8,6 +8,8 @@
 
 QemuOptsList *qemu_find_opts(const char *group);
 QemuOptsList *qemu_find_opts_err(const char *group, Error **errp);
+QemuOpts *qemu_find_opts_singleton(const char *group);
+
 void qemu_add_opts(QemuOptsList *list);
 void qemu_add_drive_opts(QemuOptsList *list);
 int qemu_set_option(const char *str);
diff --git a/util/qemu-config.c b/util/qemu-config.c
index 9298f55..9dfacbc 100644
--- a/util/qemu-config.c
+++ b/util/qemu-config.c
@@ -39,6 +39,20 @@ QemuOptsList *qemu_find_opts(const char *group)
     return ret;
 }
 
+QemuOpts *qemu_find_opts_singleton(const char *group)
+{
+    QemuOptsList *list;
+    QemuOpts *opts;
+
+    list = qemu_find_opts(group);
+    assert(list);
+    opts = qemu_opts_find(list, NULL);
+    if (!opts) {
+        opts = qemu_opts_create(list, NULL, 0, &error_abort);
+    }
+    return opts;
+}
+
 static CommandLineParameterInfoList *query_option_descs(const QemuOptDesc *desc)
 {
     CommandLineParameterInfoList *param_list = NULL, *entry;
diff --git a/vl.c b/vl.c
index 383be1b..7f2595c 100644
--- a/vl.c
+++ b/vl.c
@@ -539,16 +539,7 @@ static QemuOptsList qemu_msg_opts = {
  */
 QemuOpts *qemu_get_machine_opts(void)
 {
-    QemuOptsList *list;
-    QemuOpts *opts;
-
-    list = qemu_find_opts("machine");
-    assert(list);
-    opts = qemu_opts_find(list, NULL);
-    if (!opts) {
-        opts = qemu_opts_create(list, NULL, 0, &error_abort);
-    }
-    return opts;
+    return qemu_find_opts_singleton("machine");
 }
 
 const char *qemu_get_vm_name(void)
-- 
1.7.1

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

[Igor Mammedov]

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"
+    "                mem: initial amount of guest memory (default: "
+    stringify(DEFAULT_RAM_SIZE) "Mb)\n",
+    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 = {
     },
 };
 
+static QemuOptsList qemu_mem_opts = {
+    .name = "memory-opts",
+    .implied_opt_name = "mem",
+    .head = QTAILQ_HEAD_INITIALIZER(qemu_mem_opts.head),
+    .merge_lists = true,
+    .desc = {
+        {
+            .name = "mem",
+            .type = QEMU_OPT_SIZE,
+        },
+        { /* 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;
 
     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);
+
+                mem_str = qemu_opt_get(opts, "mem");
+                if (!mem_str) {
+                    fprintf(stderr, "qemu: invalid -m option, missing "
+                            " 'mem' option\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])) {
+                    sz <<= 20;
+                }
+
+                /* 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);
 
     if (qemu_opts_foreach(qemu_find_opts("device"), device_help_func, NULL, 0)
         != 0) {
-- 
1.7.1

Re: [Qemu-devel] [PATCH 1/2] QemuOpts: introduce qemu_find_opts_singleton

[Laszlo Ersek]

comments below

On 02/06/14 09:16, Igor Mammedov wrote:
> From: Paolo Bonzini <pbonzini@redhat.com>
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  include/qemu/config-file.h |    2 ++
>  util/qemu-config.c         |   14 ++++++++++++++
>  vl.c                       |   11 +----------
>  3 files changed, 17 insertions(+), 10 deletions(-)
> 
> diff --git a/include/qemu/config-file.h b/include/qemu/config-file.h
> index dbd97c4..d4ba20e 100644
> --- a/include/qemu/config-file.h
> +++ b/include/qemu/config-file.h
> @@ -8,6 +8,8 @@
>  
>  QemuOptsList *qemu_find_opts(const char *group);
>  QemuOptsList *qemu_find_opts_err(const char *group, Error **errp);
> +QemuOpts *qemu_find_opts_singleton(const char *group);
> +
>  void qemu_add_opts(QemuOptsList *list);
>  void qemu_add_drive_opts(QemuOptsList *list);
>  int qemu_set_option(const char *str);
> diff --git a/util/qemu-config.c b/util/qemu-config.c
> index 9298f55..9dfacbc 100644
> --- a/util/qemu-config.c
> +++ b/util/qemu-config.c
> @@ -39,6 +39,20 @@ QemuOptsList *qemu_find_opts(const char *group)
>      return ret;
>  }
>  
> +QemuOpts *qemu_find_opts_singleton(const char *group)
> +{
> +    QemuOptsList *list;
> +    QemuOpts *opts;
> +
> +    list = qemu_find_opts(group);
> +    assert(list);
> +    opts = qemu_opts_find(list, NULL);
> +    if (!opts) {
> +        opts = qemu_opts_create(list, NULL, 0, &error_abort);
> +    }
> +    return opts;
> +}

First of all, the commit message body is empty, and the subject line
doesn't really say much, plus there are no comments, so I have no clue
what we're trying to implement here *in general*.

For example, if you look into qemu_opts_create(), you'll see that when
(id==NULL), then you don't actually need the

  qemu_opts_find(list, NULL)

call, because qemu_opts_create() calls that itself anyway. Of course,
this behavior *also* depends on merge_lists being "true", but in case of
"machine", that *is* a given.

Hence a specification for the function would help deciding if we can
take merge_lists for granted, or if we want something more general. In
the former case, the code above is unnecessarily pessimistic.

(Basically the qemu_find_opts_singleton() should be a *very* thin
wrapper around qemu_opts_create(), because the latter already has this
"O_CREAT without O_EXCL" semantics.)

> +
>  static CommandLineParameterInfoList *query_option_descs(const QemuOptDesc *desc)
>  {
>      CommandLineParameterInfoList *param_list = NULL, *entry;
> diff --git a/vl.c b/vl.c
> index 383be1b..7f2595c 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -539,16 +539,7 @@ static QemuOptsList qemu_msg_opts = {
>   */
>  QemuOpts *qemu_get_machine_opts(void)
>  {
> -    QemuOptsList *list;
> -    QemuOpts *opts;
> -
> -    list = qemu_find_opts("machine");
> -    assert(list);
> -    opts = qemu_opts_find(list, NULL);
> -    if (!opts) {
> -        opts = qemu_opts_create(list, NULL, 0, &error_abort);
> -    }
> -    return opts;
> +    return qemu_find_opts_singleton("machine");
>  }
>  
>  const char *qemu_get_vm_name(void)
> 

I also kinda hate that "error_abort" -- while used in several
option-specific parser functions (balloon_parse, virtcon_parse,
QEMU_OPTION_virtfs, QEMU_OPTION_virtfs_synth) -- now makes it into a
generic-looking options function. qemu_find_opts_singleton() should take
an errp.

Anyway I don't care enough to insist on a respin.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

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

[Laszlo Ersek]

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

Re: [Qemu-devel] [PATCH 1/2] QemuOpts: introduce qemu_find_opts_singleton

[Igor Mammedov]

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

> comments below
> 
> On 02/06/14 09:16, Igor Mammedov wrote:
> > From: Paolo Bonzini <pbonzini@redhat.com>
> > 
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  include/qemu/config-file.h |    2 ++
> >  util/qemu-config.c         |   14 ++++++++++++++
> >  vl.c                       |   11 +----------
> >  3 files changed, 17 insertions(+), 10 deletions(-)
> > 
> > diff --git a/include/qemu/config-file.h b/include/qemu/config-file.h
> > index dbd97c4..d4ba20e 100644
> > --- a/include/qemu/config-file.h
> > +++ b/include/qemu/config-file.h
> > @@ -8,6 +8,8 @@
> >  
> >  QemuOptsList *qemu_find_opts(const char *group);
> >  QemuOptsList *qemu_find_opts_err(const char *group, Error **errp);
> > +QemuOpts *qemu_find_opts_singleton(const char *group);
> > +
> >  void qemu_add_opts(QemuOptsList *list);
> >  void qemu_add_drive_opts(QemuOptsList *list);
> >  int qemu_set_option(const char *str);
> > diff --git a/util/qemu-config.c b/util/qemu-config.c
> > index 9298f55..9dfacbc 100644
> > --- a/util/qemu-config.c
> > +++ b/util/qemu-config.c
> > @@ -39,6 +39,20 @@ QemuOptsList *qemu_find_opts(const char *group)
> >      return ret;
> >  }
> >  
> > +QemuOpts *qemu_find_opts_singleton(const char *group)
> > +{
> > +    QemuOptsList *list;
> > +    QemuOpts *opts;
> > +
> > +    list = qemu_find_opts(group);
> > +    assert(list);
> > +    opts = qemu_opts_find(list, NULL);
> > +    if (!opts) {
> > +        opts = qemu_opts_create(list, NULL, 0, &error_abort);
> > +    }
> > +    return opts;
> > +}
> 
> First of all, the commit message body is empty, and the subject line
> doesn't really say much, plus there are no comments, so I have no clue
> what we're trying to implement here *in general*.
> 
> For example, if you look into qemu_opts_create(), you'll see that when
> (id==NULL), then you don't actually need the
> 
>   qemu_opts_find(list, NULL)
> 
> call, because qemu_opts_create() calls that itself anyway. Of course,
> this behavior *also* depends on merge_lists being "true", but in case of
> "machine", that *is* a given.
> 
> Hence a specification for the function would help deciding if we can
> take merge_lists for granted, or if we want something more general. In
> the former case, the code above is unnecessarily pessimistic.
> 
> (Basically the qemu_find_opts_singleton() should be a *very* thin
> wrapper around qemu_opts_create(), because the latter already has this
> "O_CREAT without O_EXCL" semantics.)
I'd guess function qemu_find_opts_singleton() is called so to express
common idiom and shouldn't have an expectation for merge_lists == true
being granted. So it could be reused with other options as well.

> 
> > +
> >  static CommandLineParameterInfoList *query_option_descs(const QemuOptDesc *desc)
> >  {
> >      CommandLineParameterInfoList *param_list = NULL, *entry;
> > diff --git a/vl.c b/vl.c
> > index 383be1b..7f2595c 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -539,16 +539,7 @@ static QemuOptsList qemu_msg_opts = {
> >   */
> >  QemuOpts *qemu_get_machine_opts(void)
> >  {
> > -    QemuOptsList *list;
> > -    QemuOpts *opts;
> > -
> > -    list = qemu_find_opts("machine");
> > -    assert(list);
> > -    opts = qemu_opts_find(list, NULL);
> > -    if (!opts) {
> > -        opts = qemu_opts_create(list, NULL, 0, &error_abort);
> > -    }
> > -    return opts;
> > +    return qemu_find_opts_singleton("machine");
> >  }
> >  
> >  const char *qemu_get_vm_name(void)
> > 
> 
> I also kinda hate that "error_abort" -- while used in several
> option-specific parser functions (balloon_parse, virtcon_parse,
> QEMU_OPTION_virtfs, QEMU_OPTION_virtfs_synth) -- now makes it into a
> generic-looking options function. qemu_find_opts_singleton() should take
> an errp.
looking at possible call paths qemu_opts_create() with given arguments should
never fail so assert like behavior here seems to be just fine, so we would
notice for certain if/when it breaks.

> 
> Anyway I don't care enough to insist on a respin.
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 

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

[Igor Mammedov]

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

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

[Paolo Bonzini]

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

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

[Laszlo Ersek]

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

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

[Paolo Bonzini]

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

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

[Igor Mammedov]

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

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

[Paolo Bonzini]

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