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

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

[Peter Maydell]

(cc Markus because I know how much he likes weirdnesses in our
command line parsing :-))

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 :-)

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

thanks
-- PMM

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

[Thomas Huth]

On 31.10.2017 19:33, Peter Maydell wrote:
> (cc Markus because I know how much he likes weirdnesses in our
> command line parsing :-))
> 
> 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 :-)
> 
> Maybe we should just not allow users to pass the argument
> more than once...?

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.

 Thomas

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

[Markus Armbruster]

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 ;)