* Re: [Qemu-devel] we allow passing -machine type=foo more than once but are confused about which to use...
2017-11-01 6:22 ` Thomas Huth
@ 2017-11-08 7:42 ` Markus Armbruster
0 siblings, 0 replies; 3+ messages in thread
From: Markus Armbruster @ 2017-11-08 7:42 UTC (permalink / raw)
To: Thomas Huth; +Cc: Peter Maydell, QEMU Developers
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 ;)
^ permalink raw reply [flat|nested] 3+ messages in thread