Re: [Qemu-devel] we allow passing -machine type=foo more than once but are confused about which to use...

[Markus Armbruster <armbru@redhat.com>]

Thomas Huth <thuth@redhat.com> writes:

> On 31.10.2017 19:33, Peter Maydell wrote:
>> (cc Markus because I know how much he likes weirdnesses in our
>> command line parsing :-))

Heh!

Let me give you a guided tour to this corner of the CLI swamp :)

>> https://stackoverflow.com/questions/46955244/qemu-run-arm-ubuntu-unsupported-machine-type/47042282
>> has a user who's run into a confusing error message, because
>> we allow the user to pass "-machine type=foo" more than once on
>> the command line. When we decide which one to use, we go with the
>> last one on the list. However if it's not valid, when we print the
>> "don't recognize that machine type" message, the name we use in
>> the message is the *first* one on the list :-)

As so often, this is a case of a perfectly decent initial design messed
up by a number of enhancements that all made sense in isolation, but
fall apart together.

QemuOpts was designed to collect all parsed option arguments of a
certain kind in one instance of QemuOptsList.  For example, the
arguments of all -mon options get collected in the QemuOptsList named
"mon".  Each of the -mon arguments must have a distinct value of "id".
Absent "id" counts as a distinct value, i.e. "id" may be absent at most
once.  Each argument is represented as an instance of QemuOpts.

Yours truly enhanced QemuOpts to track locations in member @loc (commit
ab5b027ee6..0f0bc3f1d5).  This is what enables error_report() to report
the location, both on the command line and in configuration files:

    $ qemu-system-x86_64 -chardev bogus,id=chr1
    qemu-system-x86_64: -chardev bogus,id=chr1: 'bogus' is not a valid char driver name
    $ qemu-system-x86_64 -readconfig example.cfg
    qemu-system-x86_64:example.cfg:4: Invalid parameter 'type'
    qemu-system-x86_64: -readconfig example.cfg: read config example.cfg: Invalid argument

Sometimes, it's convienient to build up configuration in multiple
places.  Say your example.cfg configures a QMP monitor on stdio like
this:

    # qemu config file

    [chardev "stdio"]
      backend = "stdio"

    [mon "qmp-stdio"]
      chardev = "stdio"
      mode = "control"

To set pretty=on just for a quick test run, you could add a line to the
file, run the test, then delete it again.  Or you can modify the test
run's configuration with -set:

    $ qemu-system-x86_64 -readconfig example.cfg -set mon.qmp-stdio.pretty=on

This is certainly one of the lesser known QemuOpts features.  It's
asymmetric: the -set is quite different from the -mon it modifies.
Location reporting works fine for parse errors:

    $ qemu-system-x86_64 -nodefaults -S -readconfig example.cfg -set mon.qmp-stdio.pretty=of
    qemu-system-x86_64: -set mon.qmp-stdio.pretty=of: Parameter 'pretty' expects 'on' or 'off'

It breaks down for errors detected after the -set was applied to the
QemuOpts:

    $ qemu-system-x86_64 -nodefaults -S -netdev user,id=net1 -device e1000,id=nic1 -set device.nic1.netev=net1
    qemu-system-x86_64: -device e1000,id=nic1: Property '.netev' not found

A certain Peter Maydell (*grin*) enhanced QemuOpts to permit symmetric
modifications (commit da93318), but only for certain options, currently
-machine, -accel, -boot, -name, -memory, -icount, -smp.  Example:

    $ qemu-system-x86_64 -machine accel=kvm -machine type=q35

Without this feature, the second -machine would be rejected as
duplicate.  With the feature, it is merged into the first -machine, so
the two together become equivalent to:

    $ qemu-system-x86_64 -machine accel=kvm,type=q35

However, there's still only *one* location, since there's just one
QemuOpts!

    $ qemu-system-x86_64 --machine accel=kvm -machine type=q36
    qemu-system-x86_64: --machine accel=kvm: unsupported machine type
    Use -machine help to list supported machines

>> Maybe we should just not allow users to pass the argument
>> more than once...?

Insufficient.  We'd have to revert the options merging wholesale, and
ditch -set.

To really fix this, we'd have to move the location information from
QemuOpts to QemuOpt (the thing representing a key=value within a
QemuOpt).  Lots of client code would have to be updated, too.

> I think we at least need that in some of our qtests - to override the
> default "-M accel=qtest" with "-M accel=tcg". So if we disallow to use
> the option more than once, we've got to find a different solution for
> the qtests.

I'm afraid this feature is used not just by qtest.

Of course, given how much I expect to suffer for CLI backward
compatibility during my CLI QAPIfication work, I'd be *delighted* to
have a nice precedent for breaking it some ;)