Re: [Qemu-trivial] [PATCH for-1.4 v2 4/6] qemu-option: Disable two helpful messages that got broken recently

[Luiz Capitulino <lcapitulino@redhat.com>]

On Fri, 08 Feb 2013 20:34:20 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> > The real problem here is that the k, M, G suffixes, for example, are not
> > good to be reported by QMP. So maybe we should refactor the code in a way
> > that we separate what's done in QMP from what is done in HMP/command-line.
> 
> Isn't it separated already?  parse_option_size() is used when parsing
> key=value,...  Such strings should not exist in QMP.  If they do, it's a
> design bug.

No and no. Such strings don't exist in QMP as far as can tell (see bugs
below though), but parse_option_size() is theoretically "present" in a
possible QMP call stack:

qemu_opts_from_dict_1()
  qemu_opt_set_err()
    opt_set()
      qemu_opt_paser()
        parse_option_size()

I can't tell if this will ever happen because qemu_opts_from_dict_1()
restricts the call to qemu_opt_set_err() to certain values, but the
fact that it's not clear is an indication that a better separation is
necessary.

Now, I think I've found at least two bugs. The first one is the
netdev_add doc in the schema, which states that we do accept key=value
strings. The problem is here is that that's about the C API, on the
wire we act as before (ie. accepting each key as a separate argument).
The qapi-schame.json is more or less format-independent, so I'm not
exactly sure what's the best way to describe commands using QemuOpts
the way QMP uses it.

The second bug is that I entirely ignored how set_option_paramater()
handles errors when doing parse_option_size() conversion to Error **
and also when converting bdrv_img_create(). The end result is that
we can report an error twice, once with error_set() and later with
qerror_report() (or vice-versa). Shouldn't hurt on QMP as it knows
how to deal with this, on HMP and command-line we get complementary
error messages if we're lucky.

I'm very surprised with my mistakes on the second bug (although some
of the mess with fprintf() was already there), but I honestly think we
should defer this to 1.5 (and I can do it myself next week).