| Message ID | 20220805100635.493961-1-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [for-7.1] vl: fix [memory] section with -readconfig | expand |
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
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()?
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();
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(-)