[PATCH for-7.1] vl: fix [memory] section with -readconfig

[PATCH for-7.1] vl: fix [memory] section with -readconfig

[Paolo Bonzini]

The -M memory.* options do not have magic applied to them than the -m
option, namely no "M" (for mebibytes) is tacked at the end of a
suffixless value for "-M memory.size".

This magic is performed by parse_memory_options, and we have to
do it for both "-m" and the [memory] section of a config file.
Storing [memory] sections directly to machine_opts_dict changed
the meaning of

    [memory]
      size = "1024"

in a -readconfig file from 1024MiB to 8KiB (1024 Bytes rounded up to
8KiB silently).  To avoid this, the [memory] section has to be
changed back to QemuOpts (combining [memory] and "-m" will work fine
thanks to .merge_lists being true).

Change parse_memory_options() so that, similar to the older function
set_memory_options(), it operates after command line parsing is done;
and also call it where set_memory_options() used to be.

Note, the parsing code uses exit(1) instead of exit(EXIT_FAILURE) to
match neighboring code.

Reported-by: Markus Armbruster <armbru@redhat.com>
Fixes: ce9d03fb3f ("machine: add mem compound property", 2022-05-12)
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 softmmu/vl.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/softmmu/vl.c b/softmmu/vl.c
index aabd82e09a..3c23f266e9 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -1947,17 +1947,20 @@ static void qemu_resolve_machine_memdev(void)
     }
 }
 
-static void parse_memory_options(const char *arg)
+static void parse_memory_options(void)
 {
-    QemuOpts *opts;
+    QemuOpts *opts = qemu_find_opts_singleton("memory");
     QDict *dict, *prop;
     const char *mem_str;
+    Location loc;
 
-    opts = qemu_opts_parse_noisily(qemu_find_opts("memory"), arg, true);
     if (!opts) {
-        exit(EXIT_FAILURE);
+        return;
     }
 
+    loc_push_none(&loc);
+    qemu_opts_loc_restore(opts);
+
     prop = qdict_new();
 
     if (qemu_opt_get_size(opts, "size", 0) != 0) {
@@ -1987,6 +1990,7 @@ static void parse_memory_options(const char *arg)
     qdict_put(dict, "memory", prop);
     keyval_merge(machine_opts_dict, dict, &error_fatal);
     qobject_unref(dict);
+    loc_pop(&loc);
 }
 
 static void qemu_create_machine(QDict *qdict)
@@ -2053,8 +2057,7 @@ static bool is_qemuopts_group(const char *group)
     if (g_str_equal(group, "object") ||
         g_str_equal(group, "machine") ||
         g_str_equal(group, "smp-opts") ||
-        g_str_equal(group, "boot-opts") ||
-        g_str_equal(group, "memory")) {
+        g_str_equal(group, "boot-opts")) {
         return false;
     }
     return true;
@@ -2078,8 +2081,6 @@ static void qemu_record_config_group(const char *group, QDict *dict,
         machine_merge_property("smp", dict, &error_fatal);
     } else if (g_str_equal(group, "boot-opts")) {
         machine_merge_property("boot", dict, &error_fatal);
-    } else if (g_str_equal(group, "memory")) {
-        machine_merge_property("memory", dict, &error_fatal);
     } else {
         abort();
     }
@@ -2882,7 +2883,10 @@ void qemu_init(int argc, char **argv, char **envp)
                 exit(0);
                 break;
             case QEMU_OPTION_m:
-                parse_memory_options(optarg);
+                opts = qemu_opts_parse_noisily(qemu_find_opts("memory"), optarg, true);
+                if (opts == NULL) {
+                    exit(1);
+                }
                 break;
 #ifdef CONFIG_TPM
             case QEMU_OPTION_tpmdev:
@@ -3515,6 +3519,9 @@ void qemu_init(int argc, char **argv, char **envp)
 
     configure_rtc(qemu_find_opts_singleton("rtc"));
 
+    /* Transfer QemuOpts options into machine options */
+    parse_memory_options();
+
     qemu_create_machine(machine_opts_dict);
 
     suspend_mux_open();
-- 
2.37.1

Re: [PATCH for-7.1] vl: fix [memory] section with -readconfig

[Daniel P. Berrangé]

On Fri, Aug 05, 2022 at 12:06:35PM +0200, Paolo Bonzini wrote:
> The -M memory.* options do not have magic applied to them than the -m
> option, namely no "M" (for mebibytes) is tacked at the end of a
> suffixless value for "-M memory.size".
> 
> This magic is performed by parse_memory_options, and we have to
> do it for both "-m" and the [memory] section of a config file.
> Storing [memory] sections directly to machine_opts_dict changed
> the meaning of
> 
>     [memory]
>       size = "1024"
> 
> in a -readconfig file from 1024MiB to 8KiB (1024 Bytes rounded up to
> 8KiB silently).  To avoid this, the [memory] section has to be
> changed back to QemuOpts (combining [memory] and "-m" will work fine
> thanks to .merge_lists being true).
> 
> Change parse_memory_options() so that, similar to the older function
> set_memory_options(), it operates after command line parsing is done;
> and also call it where set_memory_options() used to be.
> 
> Note, the parsing code uses exit(1) instead of exit(EXIT_FAILURE) to
> match neighboring code.
> 
> Reported-by: Markus Armbruster <armbru@redhat.com>
> Fixes: ce9d03fb3f ("machine: add mem compound property", 2022-05-12)
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  softmmu/vl.c | 25 ++++++++++++++++---------
>  1 file changed, 16 insertions(+), 9 deletions(-)

I wrote a qtest (see cc'd separate mail) to validate -readconfig
handling of '[memory]' and this change makes the test pass, so
on that basis

Tested-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH for-7.1] vl: fix [memory] section with -readconfig

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> The -M memory.* options do not have magic applied to them than the -m
> option, namely no "M" (for mebibytes) is tacked at the end of a
> suffixless value for "-M memory.size".

This sentence is confusing.  Do you mean "like the -m option"?

> This magic is performed by parse_memory_options, and we have to
> do it for both "-m" and the [memory] section of a config file.
> Storing [memory] sections directly to machine_opts_dict changed
> the meaning of
>
>     [memory]
>       size = "1024"
>
> in a -readconfig file from 1024MiB to 8KiB (1024 Bytes rounded up to
> 8KiB silently).  To avoid this, the [memory] section has to be
> changed back to QemuOpts (combining [memory] and "-m" will work fine
> thanks to .merge_lists being true).
>
> Change parse_memory_options() so that, similar to the older function
> set_memory_options(), it operates after command line parsing is done;
> and also call it where set_memory_options() used to be.
>
> Note, the parsing code uses exit(1) instead of exit(EXIT_FAILURE) to
> match neighboring code.

Thanks for that.

> Reported-by: Markus Armbruster <armbru@redhat.com>
> Fixes: ce9d03fb3f ("machine: add mem compound property", 2022-05-12)
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  softmmu/vl.c | 25 ++++++++++++++++---------
>  1 file changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index aabd82e09a..3c23f266e9 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -1947,17 +1947,20 @@ static void qemu_resolve_machine_memdev(void)
>      }
>  }
>  
> -static void parse_memory_options(const char *arg)
> +static void parse_memory_options(void)
>  {
> -    QemuOpts *opts;
> +    QemuOpts *opts = qemu_find_opts_singleton("memory");
>      QDict *dict, *prop;
>      const char *mem_str;
> +    Location loc;
>  
> -    opts = qemu_opts_parse_noisily(qemu_find_opts("memory"), arg, true);
>      if (!opts) {
> -        exit(EXIT_FAILURE);
> +        return;
>      }

qemu_find_opts_singleton() never returns null.  Drop the null check,
please.

> +    loc_push_none(&loc);
> +    qemu_opts_loc_restore(opts);
> +
>      prop = qdict_new();
>  
>      if (qemu_opt_get_size(opts, "size", 0) != 0) {

This treats "size=0" like absent size.  Before commit ce9d03fb3f, we
instead checked

       mem_str = qemu_opt_get(opts, "size");
       if (mem_str) {

Makes more sense, doesn't it?

Also, with the new check above, the check below...

           mem_str = qemu_opt_get(opts, "size");
           if (!*mem_str) {
               error_report("missing 'size' option value");
               exit(EXIT_FAILURE);
           }

... looks dead.  We get there only when qemu_opt_get_size() returns
non-zero, which implies a non-blank string.

           /* Fix up legacy suffix-less format */
           if (g_ascii_isdigit(mem_str[strlen(mem_str) - 1])) {
               g_autofree char *mib_str = g_strdup_printf("%sM", mem_str);
               qdict_put_str(prop, "size", mib_str);
           } else {
               qdict_put_str(prop, "size", mem_str);
           }
       }

       if (qemu_opt_get(opts, "maxmem")) {
           qdict_put_str(prop, "max-size", qemu_opt_get(opts, "maxmem"));
       }
       if (qemu_opt_get(opts, "slots")) {
           qdict_put_str(prop, "slots", qemu_opt_get(opts, "slots"));
       }

       dict = qdict_new();
> @@ -1987,6 +1990,7 @@ static void parse_memory_options(const char *arg)
>      qdict_put(dict, "memory", prop);
>      keyval_merge(machine_opts_dict, dict, &error_fatal);
>      qobject_unref(dict);
> +    loc_pop(&loc);
>  }
>  
>  static void qemu_create_machine(QDict *qdict)

Commit ce9d03fb3f changed this function's purpose and renamed it from
set_memory_options() to parse_memory_options().

This commit is a partial revert.  It doesn't revert the change of name.
Intentional?

> @@ -2053,8 +2057,7 @@ static bool is_qemuopts_group(const char *group)
>      if (g_str_equal(group, "object") ||
>          g_str_equal(group, "machine") ||
>          g_str_equal(group, "smp-opts") ||
> -        g_str_equal(group, "boot-opts") ||
> -        g_str_equal(group, "memory")) {
> +        g_str_equal(group, "boot-opts")) {
>          return false;
>      }
>      return true;
> @@ -2078,8 +2081,6 @@ static void qemu_record_config_group(const char *group, QDict *dict,
>          machine_merge_property("smp", dict, &error_fatal);
>      } else if (g_str_equal(group, "boot-opts")) {
>          machine_merge_property("boot", dict, &error_fatal);
> -    } else if (g_str_equal(group, "memory")) {
> -        machine_merge_property("memory", dict, &error_fatal);
>      } else {
>          abort();
>      }
> @@ -2882,7 +2883,10 @@ void qemu_init(int argc, char **argv, char **envp)
>                  exit(0);
>                  break;
>              case QEMU_OPTION_m:
> -                parse_memory_options(optarg);
> +                opts = qemu_opts_parse_noisily(qemu_find_opts("memory"), optarg, true);
> +                if (opts == NULL) {
> +                    exit(1);
> +                }
>                  break;
>  #ifdef CONFIG_TPM
>              case QEMU_OPTION_tpmdev:

The previous three hunks revert commit ce9d03fb3f's switch from QemuOpts
to qemu_record_config_group().  Makes sense.

> @@ -3515,6 +3519,9 @@ void qemu_init(int argc, char **argv, char **envp)
>  
>      configure_rtc(qemu_find_opts_singleton("rtc"));
>  
> +    /* Transfer QemuOpts options into machine options */
> +    parse_memory_options();
> +
>      qemu_create_machine(machine_opts_dict);
>  
>      suspend_mux_open();

We used to call set_memory_options() early in qemu_create_machine().
Calling it here now should work, too.  Pointing out to make sure it's
not an accident.

Re: [PATCH for-7.1] vl: fix [memory] section with -readconfig

[Paolo Bonzini]

On 8/5/22 15:40, Markus Armbruster wrote:
>> +    loc_push_none(&loc);
>> +    qemu_opts_loc_restore(opts);
>> +
>>       prop = qdict_new();
>>   
>>       if (qemu_opt_get_size(opts, "size", 0) != 0) {
> This treats "size=0" like absent size.  Before commit ce9d03fb3f, we
> instead checked
> 
>         mem_str = qemu_opt_get(opts, "size");
>         if (mem_str) {
> 
> Makes more sense, doesn't it?

True, on the other hand before commit ce9d03fb3f we handled "-m 0" like 
this:

     sz = 0;
     mem_str = qemu_opt_get(opts, "size");
     if (mem_str) {
         ...
     }
     if (sz == 0) {
         sz = default_ram_size;
     }

Now instead, the "-m 0" case results in no qdict_put_str() call at all. 
  So the code flows better with qemu_opt_get_size(opts, "size", 0).

In addition, using qemu_opt_get_size() is what enables the dead code 
removal below, because it generates an error for empty size.

> Also, with the new check above, the check below...
> 
>             mem_str = qemu_opt_get(opts, "size");
>             if (!*mem_str) {
>                 error_report("missing 'size' option value");
>                 exit(EXIT_FAILURE);
>             }
> 
> ... looks dead.  We get there only when qemu_opt_get_size() returns
> non-zero, which implies a non-blank string.

Makes sense.  Separate patch?

>>   static void qemu_create_machine(QDict *qdict)
> Commit ce9d03fb3f changed this function's purpose and renamed it from
> set_memory_options() to parse_memory_options().
> 
> This commit is a partial revert.  It doesn't revert the change of name.
> Intentional?

Yes, though honestly both are pretty bad names.  set_memory_options() is 
bad because it's not setting anything, it's just putting the values in a 
QDict.  I kept parse_*() because it does do a limited amount of parsing 
to handle the suffix-less memory size.

Paolo

Re: [PATCH for-7.1] vl: fix [memory] section with -readconfig

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 8/5/22 15:40, Markus Armbruster wrote:
>>> +    loc_push_none(&loc);
>>> +    qemu_opts_loc_restore(opts);
>>> +
>>>       prop = qdict_new();
>>>         if (qemu_opt_get_size(opts, "size", 0) != 0) {
>>
>> This treats "size=0" like absent size.  Before commit ce9d03fb3f, we
>> instead checked
>>         mem_str = qemu_opt_get(opts, "size");
>>         if (mem_str) {
>> Makes more sense, doesn't it?
>
> True, on the other hand before commit ce9d03fb3f we handled "-m 0" like this:
>
>     sz = 0;
>     mem_str = qemu_opt_get(opts, "size");
>     if (mem_str) {
>         ...
>     }
>     if (sz == 0) {
>         sz = default_ram_size;
>     }
>
> Now instead, the "-m 0" case results in no qdict_put_str() call at all.   So the code flows better with qemu_opt_get_size(opts, "size", 0).

I see.

> In addition, using qemu_opt_get_size() is what enables the dead code removal below, because it generates an error for empty size.

My personal preference would be to default only absent size, but not an
explicit size=0.  But that's a change, and you patch's mission is fix,
not change, so okay.

>> Also, with the new check above, the check below...
>>             mem_str = qemu_opt_get(opts, "size");
>>             if (!*mem_str) {
>>                 error_report("missing 'size' option value");
>>                 exit(EXIT_FAILURE);
>>             }
>> ... looks dead.  We get there only when qemu_opt_get_size() returns
>> non-zero, which implies a non-blank string.
>
> Makes sense.  Separate patch?

Sure!

>>>   static void qemu_create_machine(QDict *qdict)
>> Commit ce9d03fb3f changed this function's purpose and renamed it from
>> set_memory_options() to parse_memory_options().
>> This commit is a partial revert.  It doesn't revert the change of name.
>> Intentional?
>
> Yes, though honestly both are pretty bad names.  set_memory_options() is bad because it's not setting anything, it's just putting the values in a 
> QDict.  I kept parse_*() because it does do a limited amount of parsing to handle the suffix-less memory size.

Intentionally keeping a moderately bad name is okay.

Okay need not stop us from looking for a better one, so: the function's
purpose is to merge the contents of QemuOpts (singleton) group "memory"
into machine options.  merge_memory_into_machine_options()?