Re: [Qemu-devel] [PATCH V12 1/4] add def_value_str and use it in qemu_opts_print

[Markus Armbruster]

Dong Xu Wang <address@hidden> writes:

> qemu_opts_print has no user now, so can re-write the function safely.
>
> qemu_opts_print will be used while using "qemu-img create", it will
> produce the same output as previous code.
>
> The behavior of this function has changed:
>
> 1. Print every possible option, whether a value has been set or not.
> 2. Option descriptors may provide a default value.
> 3. Print to stdout instead of stderr.
>
> Previously the behavior was to print every option that has been set.
> Options that have not been set would be skipped.

The commit message needs updating for v12: def_value_str isn't just
about qemu_opts_print() anymore.

More serious, the default value job is only half done.  My fault,
because my description of what I want done in my review of v11 was
confused and misleading.  I apologize for that.  Let me try again.

You want qemu_opts_print() to print the effective value even for some
options that aren't set, because you don't want to change output of
qemu-img.  Fair enough.

Currently, what to do when an option isn't set is left to
qemu_opt_get_FOO() callers:

* qemu_opt_get() returns NULL when option isn't set.  Caller can fall
  back to a default value, or do something else entirely.

* qemu_opt_get_{bool,number,size}() let the caller supply a default
  value argument to be returned when the option isn't set.

Trouble is qemu_opts_print() can't know what these callers do, so it
can't show the effective value for options that aren't set.

Your v11 solved this problem by putting the default value in
QemuOptDesc, too, for use by qemu_opts_print().  Requires copying the
caller's real default value there.

You copied only the default values you actually need for your particular
use of qemu_opts_print().

Keeping the copies consistent is manual.  It's impossible when different
callers use different default values, but that isn't the case for any of
the ones you copied, as far as I can tell.

I dislike having the default value in multiple places.  Two ways to
avoid the duplication:

1. Keep the default value entirely in the callers

   In cases where you want qemu_opts_print() to show the effective
   value, change the caller to also set the value in opts when it falls
   back to a default.

   Keep qemu_opts_print() showing only options that are actually set.
   This is fine, because the previous paragraph makes sure all the
   values you want to see are actually set.

2. Move the default value entirely to QemuOptDesc

   When getting the value of an option that hasn't been set, and
   QemuOptDesc has a default value, return that.  Else, behave as
   before.

   Example: qemu_opt_get_number(opts, "foo", 42)

       If "foo" has been set in opts, return its value.

       Else, if opt's QemuOptDesc has a default value for "foo", return
       that.

       Else, return 42.

       Note that the last argument is useless when QemuOptDesc has a
       default value.  Ugly.  If it bothers us, assert that the argument
       equals the default from QemuOptDesc.

   Example: qemu_opt_get(opts, "bar")

       If "bar" has been set in opts, return its value.

       Else, if opt's QemuOptDesc has a default value for "bar", return
       that.

       Else, return NULL.

   Note that "no default in QemuOptDesc" does *not* mean "there is no
   default".  There may or may not be a default, depending on how the
   option's users get the value.

Hope I'm making sense this time :)