Command line QAPIfication and -readconfig

Command line QAPIfication and -readconfig

[Kevin Wolf]

Hi,

while QAPIfying the chardev configuration, I noticed that dropping
QemuOpts completely in vl.c would break -readconfig. So for now I'm
stopping at the point where internal interfaces are fully QAPIfied and
the QemuOpts interfaces are only a thin wrapper around them.

But do we already have a plan what to do with -readconfig? Should we
just deprecate it in 5.2 so we can complete the work in 6.1 and leave
vl.c unconverted for now? Or should we rather convert half of vl.c and
keep both QAPI and QemuOpts around for the same option for now?

Kevin

Re: Command line QAPIfication and -readconfig

[Daniel P. Berrangé]

On Wed, Nov 11, 2020 at 10:24:40AM +0100, Kevin Wolf wrote:
> Hi,
> 
> while QAPIfying the chardev configuration, I noticed that dropping
> QemuOpts completely in vl.c would break -readconfig. So for now I'm
> stopping at the point where internal interfaces are fully QAPIfied and
> the QemuOpts interfaces are only a thin wrapper around them.
> 
> But do we already have a plan what to do with -readconfig? Should we
> just deprecate it in 5.2 so we can complete the work in 6.1 and leave
> vl.c unconverted for now? Or should we rather convert half of vl.c and
> keep both QAPI and QemuOpts around for the same option for now?

-readconfig is a little complex because we're not trying to remove the
ability to use a config file, instead we want to switch to a different
type of config file.

IOW, it is about replacement, rather than removal, of functionality.

Normally we would not mark something as deprecated unless its replacement
is ready, because telling people "stop using this but the replacement
doesn't exist yet" is not a nice message as there is no action users can
take to deal with the deprecation.

We might question whether -readconfig has any users but I would note
that our own documentation illustrates its usage, and provides several
example configs

$ git grep docs -- -readconfig
config/ich9-ehci-uhci.cfg:# You can pass this file directly to qemu using the -readconfig
config/mach-virt-graphical.cfg:#     -readconfig mach-virt-graphical.cfg \
config/mach-virt-serial.cfg:#     -readconfig mach-virt-serial.cfg \
config/q35-emulated.cfg:#     -readconfig q35-emulated.cfg
config/q35-virtio-graphical.cfg:#     -readconfig q35-virtio-graphical.cfg
config/q35-virtio-serial.cfg:#     -readconfig q35-virtio-serial.cfg \
devel/blkdebug.txt:follows the same .ini-like format used by QEMU's -readconfig option, and
system/qemu-block-drivers.rst.inc:in a configuration file provided via '-readconfig' or directly on the
system/qemu-block-drivers.rst.inc:    -readconfig iscsi.conf
usb2.txt:    qemu -readconfig docs/config/ich9-ehci-uhci.cfg


IIUC, even with just the internal interface conversion to QAPI we're able
to introduce our new config file functionality in whatever manner we like.

Thus removing QemuOpts from vl.c should not be a blocker for other work,
it is more of a "nice to have" from pov of cleaning up code.

IOW, I'd suggest we focus effort on introducing the new config file format
based on QAPI first, and once that exists, then convert these sample
config files in docs/config, and deprecate -readconfig.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: Command line QAPIfication and -readconfig

[Kevin Wolf]

Am 11.11.2020 um 11:14 hat Daniel P. Berrangé geschrieben:
> On Wed, Nov 11, 2020 at 10:24:40AM +0100, Kevin Wolf wrote:
> > Hi,
> > 
> > while QAPIfying the chardev configuration, I noticed that dropping
> > QemuOpts completely in vl.c would break -readconfig. So for now I'm
> > stopping at the point where internal interfaces are fully QAPIfied and
> > the QemuOpts interfaces are only a thin wrapper around them.
> > 
> > But do we already have a plan what to do with -readconfig? Should we
> > just deprecate it in 5.2 so we can complete the work in 6.1 and leave
> > vl.c unconverted for now? Or should we rather convert half of vl.c and
> > keep both QAPI and QemuOpts around for the same option for now?
> 
> -readconfig is a little complex because we're not trying to remove the
> ability to use a config file, instead we want to switch to a different
> type of config file.
> 
> IOW, it is about replacement, rather than removal, of functionality.

That's the long term plan, but we can only add that different type of
config file once all options are QAPIfied and QAPIfication has the
problem of having to deal with -readconfig.

> Normally we would not mark something as deprecated unless its replacement
> is ready, because telling people "stop using this but the replacement
> doesn't exist yet" is not a nice message as there is no action users can
> take to deal with the deprecation.

But there is a replacement: Put everything back into the command line
and keep it in a shell script. Config files with -readconfig were never
complete enough to fully describe a VM, so it's not too unlikely that
you'll already have that script anyway.

> We might question whether -readconfig has any users but I would note
> that our own documentation illustrates its usage, and provides several
> example configs
> 
> $ git grep docs -- -readconfig
> config/ich9-ehci-uhci.cfg:# You can pass this file directly to qemu using the -readconfig
> config/mach-virt-graphical.cfg:#     -readconfig mach-virt-graphical.cfg \
> config/mach-virt-serial.cfg:#     -readconfig mach-virt-serial.cfg \
> config/q35-emulated.cfg:#     -readconfig q35-emulated.cfg
> config/q35-virtio-graphical.cfg:#     -readconfig q35-virtio-graphical.cfg
> config/q35-virtio-serial.cfg:#     -readconfig q35-virtio-serial.cfg \
> devel/blkdebug.txt:follows the same .ini-like format used by QEMU's -readconfig option, and
> system/qemu-block-drivers.rst.inc:in a configuration file provided via '-readconfig' or directly on the
> system/qemu-block-drivers.rst.inc:    -readconfig iscsi.conf
> usb2.txt:    qemu -readconfig docs/config/ich9-ehci-uhci.cfg
> 
> 
> IIUC, even with just the internal interface conversion to QAPI we're able
> to introduce our new config file functionality in whatever manner we like.
> 
> Thus removing QemuOpts from vl.c should not be a blocker for other work,
> it is more of a "nice to have" from pov of cleaning up code.
> 
> IOW, I'd suggest we focus effort on introducing the new config file format
> based on QAPI first, and once that exists, then convert these sample
> config files in docs/config, and deprecate -readconfig.

Fine with me. That would make introducing the new config file format a
priority, though, even if it can't support every option yet (similar to
-readconfig). I didn't have the impression so far that we are planning
to do that. Is anyone working on it?

Kevin

Re: Command line QAPIfication and -readconfig

[Paolo Bonzini]

On 11/11/20 11:35, Kevin Wolf wrote:
>> IOW, I'd suggest we focus effort on introducing the new config file format
>> based on QAPI first, and once that exists, then convert these sample
>> config files in docs/config, and deprecate -readconfig.
>
> Fine with me. That would make introducing the new config file format a
> priority, though, even if it can't support every option yet (similar to
> -readconfig). I didn't have the impression so far that we are planning
> to do that. Is anyone working on it?

Indeed, my plan was to focus on QMP-based configuration, not on 
configuration file formats.

However I hit the same snag, in that my patches broke -readconfig for 
-object, -M and -accel.  I'm thinking of decoupling config file parsing 
from QemuOpts, using QDicts instead and moving the QemuOpts part into 
softmmu/vl.c.

Paolo

Re: Command line QAPIfication and -readconfig

[Kevin Wolf]

Am 11.11.2020 um 12:07 hat Paolo Bonzini geschrieben:
> On 11/11/20 11:35, Kevin Wolf wrote:
> > > IOW, I'd suggest we focus effort on introducing the new config file format
> > > based on QAPI first, and once that exists, then convert these sample
> > > config files in docs/config, and deprecate -readconfig.
> > 
> > Fine with me. That would make introducing the new config file format a
> > priority, though, even if it can't support every option yet (similar to
> > -readconfig). I didn't have the impression so far that we are planning
> > to do that. Is anyone working on it?
> 
> Indeed, my plan was to focus on QMP-based configuration, not on
> configuration file formats.
> 
> However I hit the same snag, in that my patches broke -readconfig for
> -object, -M and -accel.  I'm thinking of decoupling config file parsing from
> QemuOpts, using QDicts instead and moving the QemuOpts part into
> softmmu/vl.c.

QDicts are one step closer to the final result, but would also have to
be processed separately as they need only half of the processing that
command line options need. Eventually, qobject_input_visitor_new_str()
is what we want to use to parse strings directly into QAPI objects, and
QDicts are only an internal intermediate result there.

So while it's even uglier, maybe what we should do with -readconfig is
convert it back to strings and then run the result through the normal
command line processing? This would get rid of the special cases.

Both options are probably only hacks for the short term, so either way I
think I'd still prefer deprecating -readconfig now, in favour of command
line options as long as we don't have a QAPI based config file.

Kevin

Re: Command line QAPIfication and -readconfig

[Paolo Bonzini]

On 11/11/20 13:53, Kevin Wolf wrote:
> QDicts are one step closer to the final result, but would also have to
> be processed separately as they need only half of the processing that
> command line options need. Eventually, qobject_input_visitor_new_str()
> is what we want to use to parse strings directly into QAPI objects, and
> QDicts are only an internal intermediate result there.

To be clearer, the processing would look like this:

- both qemu_config_parse and qemu_read_config_file call a static 
function qemu_config_foreach

- qemu_config_foreach builds a QDict and passes it to a callback

qemu_read_config_file receives a callback argument from vl.c, while 
qemu_config_parse uses a default callback that does qemu_opts_create and 
qemu_opts_absorb_qdict.

The callback in vl.c does this:

1) for a keyval-based non-mergeable option (e.g. -object) -> do 
qdict_crumple and store the QDict for later processing.

2) for a keyval-based mergeable option (e.g. -M) -> do qdict_crumple and 
merge with previous command-line options using a new function keyval_merge

3) for non-keyval options -> forward to the default callback used by 
qemu_config_parse.


Later on, keyval-based options are parsed into QAPI objects or QOM 
properties using qobject_input_visitor_new_keyval.

> So while it's even uglier, maybe what we should do with -readconfig is
> convert it back to strings and then run the result through the normal
> command line processing? This would get rid of the special cases.

That would be more or less doable but there are two problems: 1) the 
-readconfig code is used for blkdebug too, so you would need some kind 
of callback anyway 2) converting to strings is not entirely trivial due 
to e.g. different spelling between the "-boot" command line option and 
the "boot-opts" group.

> Both options are probably only hacks for the short term, so either way I
> think I'd still prefer deprecating -readconfig now, in favour of command
> line options as long as we don't have a QAPI based config file.

"Now" is a bit too late for 5.2.  I would like to deprecate "-set" for 
that matter too, but it's so obscure that I'm thinking of just dropping 
it in 6.0.

Paolo

Re: Command line QAPIfication and -readconfig

[Kevin Wolf]

Am 11.11.2020 um 14:06 hat Paolo Bonzini geschrieben:
> On 11/11/20 13:53, Kevin Wolf wrote:
> > QDicts are one step closer to the final result, but would also have to
> > be processed separately as they need only half of the processing that
> > command line options need. Eventually, qobject_input_visitor_new_str()
> > is what we want to use to parse strings directly into QAPI objects, and
> > QDicts are only an internal intermediate result there.
> 
> To be clearer, the processing would look like this:
> 
> - both qemu_config_parse and qemu_read_config_file call a static function
> qemu_config_foreach
> 
> - qemu_config_foreach builds a QDict and passes it to a callback
> 
> qemu_read_config_file receives a callback argument from vl.c, while
> qemu_config_parse uses a default callback that does qemu_opts_create and
> qemu_opts_absorb_qdict.
> 
> The callback in vl.c does this:
> 
> 1) for a keyval-based non-mergeable option (e.g. -object) -> do
> qdict_crumple and store the QDict for later processing.
> 
> 2) for a keyval-based mergeable option (e.g. -M) -> do qdict_crumple and
> merge with previous command-line options using a new function keyval_merge

Ah, I wasn't even aware of this type of option, so thanks for mentioning
it here. (It makes sense to me.)

> 3) for non-keyval options -> forward to the default callback used by
> qemu_config_parse.
> 
> 
> Later on, keyval-based options are parsed into QAPI objects or QOM
> properties using qobject_input_visitor_new_keyval.

Yes, no question this is doable, it just requires some extra code for
each option instead of reusing something existing. But it's not too bad
as QDicts are at least slightly more natural for a QAPI based interface
than QemuOpts.

> > So while it's even uglier, maybe what we should do with -readconfig is
> > convert it back to strings and then run the result through the normal
> > command line processing? This would get rid of the special cases.
> 
> That would be more or less doable but there are two problems: 1) the
> -readconfig code is used for blkdebug too, so you would need some kind of
> callback anyway

Whether it's a callback or an iterator shouldn't make a big difference.
blkdebug can easily use either.

And whether the data is QemuOpts, QDict or a string shouldn't make much
of a difference either because it can just run a visitor on any of them
and get back the QAPI BlkdebugFoo types that it should be using anyway.

> 2) converting to strings is not entirely trivial due to e.g.
> different spelling between the "-boot" command line option and the
> "boot-opts" group.

Hm, where is the difference between both? The QEMU_OPTION_boot case
seems to just directly parse optarg into "boot-opts" with
qemu_opts_parse_noisily(), which should be the same as -readconfig does
with its input, no?

> > Both options are probably only hacks for the short term, so either way I
> > think I'd still prefer deprecating -readconfig now, in favour of command
> > line options as long as we don't have a QAPI based config file.
> 
> "Now" is a bit too late for 5.2.  I would like to deprecate "-set" for that
> matter too, but it's so obscure that I'm thinking of just dropping it in
> 6.0.

Even if we queue the -readconfig deprecation only for 6.0, that's still
fine. I just don't want to discuss one year from now how we should have
deprecated it long ago.

Kevin

Re: Command line QAPIfication and -readconfig

[Paolo Bonzini]

On 11/11/20 16:29, Kevin Wolf wrote:
> Yes, no question this is doable, it just requires some extra code for
> each option instead of reusing something existing. But it's not too bad
> as QDicts are at least slightly more natural for a QAPI based interface
> than QemuOpts.

Yeah, see the series I posted earlier.  There's some extra code but it's 
fairly well confined.

>> 2) converting to strings is not entirely trivial due to e.g.
>> different spelling between the "-boot" command line option and the
>> "boot-opts" group.
> 
> Hm, where is the difference between both? The QEMU_OPTION_boot case
> seems to just directly parse optarg into "boot-opts" with
> qemu_opts_parse_noisily(), which should be the same as -readconfig does
> with its input, no?

I mean that you could not easily turn

[boot-opts]
	order = "c"

into "-boot order=c" because the "boot-opts" -> "-boot" mapping is not 
written anywhere (except in code of course).

> Even if we queue the -readconfig deprecation only for 6.0, that's still
> fine. I just don't want to discuss one year from now how we should have
> deprecated it long ago.

As usual, the choice will be:

1) deprecate and give a firm removal date, knowing that there won't be a 
replacement ready in time

2) deprecate with no plan to remove; use deprecation to justify breaking 
whatever is too complicated to maintain

3) refactor to keep the code at least readable and as simple as 
possible, desugaring to something completely new and more maintainable

For -set I'm leaning towards (2), keeping only "-set device" working as 
it does have users.  For the rest of vl.c I'm betting on building (3) 
around a core consisting of -blockdev, -netdev, -machine, -object, 
-accel and -device.

Paolo

Re: Command line QAPIfication and -readconfig

[Andrea Bolognani]

On Wed, 2020-11-11 at 11:35 +0100, Kevin Wolf wrote:
> Am 11.11.2020 um 11:14 hat Daniel P. Berrangé geschrieben:
> > Normally we would not mark something as deprecated unless its replacement
> > is ready, because telling people "stop using this but the replacement
> > doesn't exist yet" is not a nice message as there is no action users can
> > take to deal with the deprecation.
> 
> But there is a replacement: Put everything back into the command line
> and keep it in a shell script. Config files with -readconfig were never
> complete enough to fully describe a VM, so it's not too unlikely that
> you'll already have that script anyway.

This is correct: in fact...

> > We might question whether -readconfig has any users but I would note
> > that our own documentation illustrates its usage, and provides several
> > example configs
> > 
[...]
> > config/mach-virt-graphical.cfg:#     -readconfig mach-virt-graphical.cfg \
> > config/mach-virt-serial.cfg:#     -readconfig mach-virt-serial.cfg \
> > config/q35-emulated.cfg:#     -readconfig q35-emulated.cfg
> > config/q35-virtio-graphical.cfg:#     -readconfig q35-virtio-graphical.cfg
> > config/q35-virtio-serial.cfg:#     -readconfig q35-virtio-serial.cfg \
[...]

... these configuration files, which I contributed some time ago, all
start with something along the lines of

  # Usage:
  #
  #   $ qemu-system-aarch64 \
  #     -nodefaults \
  #     -readconfig mach-virt-serial.cfg \
  #     -display none -serial mon:stdio \
  #     -cpu host

because it was simply impossible to provide QEMU with all the
settings necessary to obtain the desired virtual hardware using the
configuration file only.

-- 
Andrea Bolognani / Red Hat / Virtualization