Re: [PATCH for-5.2 4/4] qemu-option: warn for short-form boolean options

[Igor Mammedov]

On Tue, 3 Nov 2020 16:08:43 +0000
Daniel P. Berrangé <berrange@redhat.com> wrote:

> On Tue, Nov 03, 2020 at 10:14:52AM -0500, Paolo Bonzini wrote:
> > Options such as "server" or "nowait", that are commonly found in -chardev,
> > are sugar for "server=on" and "wait=off".  This is quite surprising and
> > also does not have any notion of typing attached.  It is even possible to
> > do "-device e1000,noid" and get a device with "id=off".
> > 
> > Deprecate all this, except for -chardev and -spice where it is in
> > wide use.
> > 
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >  chardev/char.c             |  1 +
> >  docs/system/deprecated.rst |  7 +++++++
> >  include/qemu/option.h      |  1 +
> >  tests/test-qemu-opts.c     |  1 +
> >  ui/spice-core.c            |  1 +
> >  util/qemu-option.c         | 22 +++++++++++++++-------
> >  6 files changed, 26 insertions(+), 7 deletions(-)
> > 
> > diff --git a/chardev/char.c b/chardev/char.c
> > index 78553125d3..108da615df 100644
> > --- a/chardev/char.c
> > +++ b/chardev/char.c
> > @@ -829,6 +829,7 @@ Chardev *qemu_chr_find(const char *name)
> >  
> >  QemuOptsList qemu_chardev_opts = {
> >      .name = "chardev",
> > +    .allow_flag_options = true, /* server, nowait, etc. */
> >      .implied_opt_name = "backend",
> >      .head = QTAILQ_HEAD_INITIALIZER(qemu_chardev_opts.head),
> >      .desc = {
> > diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
> > index 32a0e620db..0e7edf7e56 100644
> > --- a/docs/system/deprecated.rst
> > +++ b/docs/system/deprecated.rst
> > @@ -146,6 +146,13 @@ Drives with interface types other than ``if=none`` are 
> > for onboard
> >  devices.  It is possible to use drives the board doesn't pick up with
> >  -device.  This usage is now deprecated.  Use ``if=none`` instead.
> >  
> > +Short-form boolean options (since 5.2)
> > +''''''''''''''''''''''''''''''''''''''
> > +
> > +Boolean options such as ``share=on``/``share=off`` can be written
> > +in short form as ``share`` and ``noshare``.  This is deprecated
> > +for all command-line options except ``-chardev` and ``-spice``, for
> > +which the short form was in wide use.  
> 
> So IIUC, the short form was possible to use for absolutely /any/
> boolean property ?
> 
> IMHO if we're going to deprecate short forms, we should do it
> universally including chardev and spice. Arguably spice/chardev
> are the most important ones to give an explicit warning about
> precisely because their widespread usage means a heads up is
> important to users.  For chardev in particular it is possible
> we might end up wanting to wait longer than the usual 2 cycles
> before removal. So if we're serious about removing the short
> forms long term, the sooner we deprecate & warn the better
> for chardev.

shall we also deprecate short forms for -cpu model,[feat|+feat|-feat]
and in the end allow only -device compatible form i.e. -cpu type,feat=[on|off]

that would let us drop custom
  x86_cpu_parse_featurestr,
  ppc_cpu_parse_featurestr,
  sparc_cpu_parse_features

and a bunch of cpu_class_by_name, where almost each target does its
magic conversion of cpu_model to the type (which ranges from various
prefix/suffix shuffling to completely ignoring cpu_model and returning
a fixed cpu type)


> Regards,
> Daniel