softmmu/vl.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-)
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
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.
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
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()?
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 :|
© 2016 - 2024 Red Hat, Inc.