Paolo Bonzini <pbonz...@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()?
