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

[Paolo Bonzini]

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