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

[Markus Armbruster <armbru@redhat.com>]

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()?