Paolo Bonzini <pbonz...@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 <arm...@redhat.com>
> Fixes: ce9d03fb3f ("machine: add mem compound property", 2022-05-12)
> Signed-off-by: Paolo Bonzini <pbonz...@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.
