On Fri, 08 Feb 2013 20:34:20 +0100
Markus Armbruster <arm...@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).
