[PATCH v5 0/8] Configurable policy for handling deprecated interfaces

Markus Armbruster posted 8 patches 3 years, 7 months ago
Test docker-quick@centos7 failed
Test docker-mingw@fedora failed
Test checkpatch failed
Test FreeBSD passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20200914084802.4185028-1-armbru@redhat.com
Maintainers: Eric Blake <eblake@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, "Dr. David Alan Gilbert" <dgilbert@redhat.com>, Michael Roth <mdroth@linux.vnet.ibm.com>, Markus Armbruster <armbru@redhat.com>
There is a newer version of this series
qapi/compat.json                        |  52 ++++++++++++
qapi/introspect.json                    |   2 +-
qapi/qapi-schema.json                   |   1 +
include/qapi/compat-policy.h            |  20 +++++
include/qapi/qmp/dispatch.h             |   1 +
include/qapi/qobject-input-visitor.h    |   9 +++
include/qapi/qobject-output-visitor.h   |   9 +++
include/qapi/visitor-impl.h             |   6 ++
include/qapi/visitor.h                  |  18 +++++
monitor/monitor-internal.h              |   3 -
monitor/misc.c                          |   2 -
monitor/qmp-cmds-control.c              | 100 +++++++++++++++++++++---
qapi/qapi-visit-core.c                  |  18 +++++
qapi/qmp-dispatch.c                     |  17 ++++
qapi/qobject-input-visitor.c            |  29 +++++++
qapi/qobject-output-visitor.c           |  19 +++++
softmmu/vl.c                            |  17 ++++
storage-daemon/qemu-storage-daemon.c    |   2 -
tests/test-qmp-cmds.c                   |  91 +++++++++++++++++++--
tests/test-qmp-event.c                  |  41 ++++++++++
qapi/meson.build                        |   1 +
qapi/trace-events                       |   2 +
qemu-options.hx                         |  22 ++++++
scripts/qapi/commands.py                |  14 ++--
scripts/qapi/events.py                  |  22 +++++-
scripts/qapi/visit.py                   |  15 ++++
tests/qapi-schema/qapi-schema-test.json |  20 +++--
tests/qapi-schema/qapi-schema-test.out  |  20 ++---
28 files changed, 522 insertions(+), 51 deletions(-)
create mode 100644 qapi/compat.json
create mode 100644 include/qapi/compat-policy.h
[PATCH v5 0/8] Configurable policy for handling deprecated interfaces
Posted by Markus Armbruster 3 years, 7 months ago
New option -compat lets you configure what to do when deprecated
interfaces get used.  This is intended for testing users of the
management interfaces.  It is experimental.

-compat deprecated-input=<in-policy> configures what to do when
deprecated input is received.  Available policies:

* accept: Accept deprecated commands and arguments (default)
* reject: Reject them
* crash: Crash

-compat deprecated-output=<out-policy> configures what to do when
deprecated output is sent.  Available output policies:

* accept: Emit deprecated command results and events (default)
* hide: Suppress them

For now, -compat covers only deprecated syntactic aspects of QMP.  We
may want to extend it to cover semantic aspects, CLI, and experimental
features.

v5:
* Old PATCH 01-26 merged in commit f57587c7d47.
* Rebased, non-trivial conflicts in PATCH 1 due to Meson, and in PATCH
  7 due to visitor changes
* PATCH 1: Comments updated for 5.2 [Eric]
* PATCH 2: Harmless missing initialization fixed [Eric]
* PATCH 3+4: Harmless missing has_FOO = true fixed [Eric]
* PATCH 6+7: Commit message tweaked

v4:
* PATCH 05+07: Temporary memory leak plugged [Marc-André]
* PATCH 23: Rewritten [Marc-André]
* PATCH 24: Comment typo [Marc-André]
* PATCH 30: Memory leaks plugged

v3:
* Rebased, non-trivial conflicts in PATCH 01+26+27+34 due to RST
  conversion and code motion
* PATCH 28-29: Old PATCH 28 split up to ease review
* PATCH 30-31: New
* PATCH 32-33: Old PATCH 29 split up to ease review

Comparison to RFC (24 Oct 2019):
* Cover arguments and results in addition to commands and events
* Half-baked "[RFC PATCH 18/19] qapi: Include a warning in the
  response to a deprecated command" dropped

See also last item of
    Subject: Minutes of KVM Forum BoF on deprecating stuff
    Date: Fri, 26 Oct 2018 16:03:51 +0200
    Message-ID: <87mur0ls8o.fsf@dusky.pond.sub.org>
    https://lists.nongnu.org/archive/html/qemu-devel/2018-10/msg05828.html

Cc: Lukáš Doktor <ldoktor@redhat.com>
Cc: libguestfs@redhat.com
Cc: libvir-list@redhat.com
Cc: Daniel P. Berrange <berrange@redhat.com>
Cc: Peter Krempa <pkrempa@redhat.com>
Cc: Kevin Wolf <kwolf@redhat.com>

Markus Armbruster (8):
  qemu-options: New -compat to set policy for deprecated interfaces
  qapi: Implement deprecated-output=hide for QMP command results
  qapi: Implement deprecated-output=hide for QMP events
  qapi: Implement deprecated-output=hide for QMP event data
  qapi: Implement deprecated-output=hide for QMP introspection
  qapi: Implement deprecated-input=reject for QMP commands
  qapi: Implement deprecated-input=reject for QMP command arguments
  qapi: New -compat deprecated-input=crash

 qapi/compat.json                        |  52 ++++++++++++
 qapi/introspect.json                    |   2 +-
 qapi/qapi-schema.json                   |   1 +
 include/qapi/compat-policy.h            |  20 +++++
 include/qapi/qmp/dispatch.h             |   1 +
 include/qapi/qobject-input-visitor.h    |   9 +++
 include/qapi/qobject-output-visitor.h   |   9 +++
 include/qapi/visitor-impl.h             |   6 ++
 include/qapi/visitor.h                  |  18 +++++
 monitor/monitor-internal.h              |   3 -
 monitor/misc.c                          |   2 -
 monitor/qmp-cmds-control.c              | 100 +++++++++++++++++++++---
 qapi/qapi-visit-core.c                  |  18 +++++
 qapi/qmp-dispatch.c                     |  17 ++++
 qapi/qobject-input-visitor.c            |  29 +++++++
 qapi/qobject-output-visitor.c           |  19 +++++
 softmmu/vl.c                            |  17 ++++
 storage-daemon/qemu-storage-daemon.c    |   2 -
 tests/test-qmp-cmds.c                   |  91 +++++++++++++++++++--
 tests/test-qmp-event.c                  |  41 ++++++++++
 qapi/meson.build                        |   1 +
 qapi/trace-events                       |   2 +
 qemu-options.hx                         |  22 ++++++
 scripts/qapi/commands.py                |  14 ++--
 scripts/qapi/events.py                  |  22 +++++-
 scripts/qapi/visit.py                   |  15 ++++
 tests/qapi-schema/qapi-schema-test.json |  20 +++--
 tests/qapi-schema/qapi-schema-test.out  |  20 ++---
 28 files changed, 522 insertions(+), 51 deletions(-)
 create mode 100644 qapi/compat.json
 create mode 100644 include/qapi/compat-policy.h

-- 
2.26.2

Re: [PATCH v5 0/8] Configurable policy for handling deprecated interfaces
Posted by Richard W.M. Jones 3 years, 7 months ago
Some general comments on using the patch:

* For libguestfs I chose to add

  -compat deprecated-input=reject,deprecated-output=hide

  This is only enabled in developer builds of libguestfs when we
  are running qemu directly (not via libvirt).  The patch for
  this is attached.

* What's the point/difference in having reject vs crash?

* I hope that by hiding deprecated QAPI fields we may detect
  errors in libguestfs, but I suspect what'll happen is it'll
  cause fall-back behaviour which might be harder to detect.

* Be *really* good to have this for command line parameters!

Notes on the attached patch:

* https://libguestfs.org/guestfs-building.1.html

* Simple test:

LIBGUESTFS_BACKEND=direct \
LIBGUESTFS_HV=~/d/qemu/build/x86_64-softmmu/qemu-system-x86_64 \
./run libguestfs-test-tool

* Run the full test suite:

LIBGUESTFS_HV=~/d/qemu/build/x86_64-softmmu/qemu-system-x86_64 \
make -k check-direct

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW
>From 90df6dc8a3278800f9f9dc23f626df5fa00b5950 Mon Sep 17 00:00:00 2001
From: "Richard W.M. Jones" <rjones@redhat.com>
Date: Mon, 21 Sep 2020 13:18:05 +0100
Subject: [PATCH] lib: direct: Pass qemu -compat to detect deprecated features.

In developer versions of libguestfs only, pass the qemu -compat option
which will reject deprecated qemu features, giving us early warning if
we are using something that may be removed in future.  This does not
affect stable branch builds or old versions of qemu which did not have
this flag.
---
 lib/guestfs-internal.h |  3 +++
 lib/launch-direct.c    | 11 +++++++++++
 2 files changed, 14 insertions(+)

diff --git a/lib/guestfs-internal.h b/lib/guestfs-internal.h
index d7ec7215d..4ad1cd125 100644
--- a/lib/guestfs-internal.h
+++ b/lib/guestfs-internal.h
@@ -33,6 +33,9 @@
 
 #include <pcre.h>
 
+/* Is this a developer version of libguestfs? */
+#define IS_DEVELOPER_VERSION ((PACKAGE_VERSION_MINOR & 1) == 1)
+
 /* Minimum required version of libvirt for the libvirt backend.
  *
  * This is also checked at runtime because you can dynamically link
diff --git a/lib/launch-direct.c b/lib/launch-direct.c
index b6ed9766f..3e42609ff 100644
--- a/lib/launch-direct.c
+++ b/lib/launch-direct.c
@@ -501,6 +501,17 @@ launch_direct (guestfs_h *g, void *datav, const char *arg)
   if (guestfs_int_qemu_supports (g, data->qemu_data, "-enable-fips"))
     flag ("-enable-fips");
 
+  /* In non-stable versions of libguestfs, pass the -compat option to
+   * qemu so we can look for potentially deprecated features.
+   */
+  if (IS_DEVELOPER_VERSION &&
+      guestfs_int_qemu_supports (g, data->qemu_data, "-compat")) {
+    start_list ("-compat") {
+      append_list ("deprecated-input=reject");
+      append_list ("deprecated-output=hide");
+    } end_list ();
+  }
+
   /* Newer versions of qemu (from around 2009/12) changed the
    * behaviour of monitors so that an implicit '-monitor stdio' is
    * assumed if we are in -nographic mode and there is no other
-- 
2.28.0.rc2

Re: [PATCH v5 0/8] Configurable policy for handling deprecated interfaces
Posted by Peter Krempa 3 years, 7 months ago
On Mon, Sep 21, 2020 at 13:45:14 +0100, Richard W.M. Jones wrote:
> Some general comments on using the patch:
> 
> * For libguestfs I chose to add
> 
>   -compat deprecated-input=reject,deprecated-output=hide
> 
>   This is only enabled in developer builds of libguestfs when we
>   are running qemu directly (not via libvirt).  The patch for
>   this is attached.
> 
> * What's the point/difference in having reject vs crash?

I'll be adding the following documentation for the qemu.conf entry in
libvirt controling the feature:

+# The "reject" option is less harsh towards the VMs but some code paths ignore
+# errors reported by qemu and thus it may not be obvious that a deprecated
+# command/field was used, thus it's suggested to use the "crash" option instead.

Re: [PATCH v5 0/8] Configurable policy for handling deprecated interfaces
Posted by Richard W.M. Jones 3 years, 7 months ago
On Mon, Sep 21, 2020 at 02:54:15PM +0200, Peter Krempa wrote:
> On Mon, Sep 21, 2020 at 13:45:14 +0100, Richard W.M. Jones wrote:
> > Some general comments on using the patch:
> > 
> > * For libguestfs I chose to add
> > 
> >   -compat deprecated-input=reject,deprecated-output=hide
> > 
> >   This is only enabled in developer builds of libguestfs when we
> >   are running qemu directly (not via libvirt).  The patch for
> >   this is attached.
> > 
> > * What's the point/difference in having reject vs crash?
> 
> I'll be adding the following documentation for the qemu.conf entry in
> libvirt controling the feature:
> 
> +# The "reject" option is less harsh towards the VMs but some code paths ignore
> +# errors reported by qemu and thus it may not be obvious that a deprecated
> +# command/field was used, thus it's suggested to use the "crash" option instead.

I'm not sure if libguestfs should use reject or crash.  But since most
of the benefit of this is going to be in detecting deprecated CLI
parameters in future, reject should be sufficient for us.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

Re: [PATCH v5 0/8] Configurable policy for handling deprecated interfaces
Posted by Peter Maydell 3 years, 7 months ago
On Mon, 14 Sep 2020 at 09:55, Markus Armbruster <armbru@redhat.com> wrote:
>
> New option -compat lets you configure what to do when deprecated
> interfaces get used.  This is intended for testing users of the
> management interfaces.  It is experimental.
>
> -compat deprecated-input=<in-policy> configures what to do when
> deprecated input is received.  Available policies:
>
> * accept: Accept deprecated commands and arguments (default)
> * reject: Reject them
> * crash: Crash
>
> -compat deprecated-output=<out-policy> configures what to do when
> deprecated output is sent.  Available output policies:
>
> * accept: Emit deprecated command results and events (default)
> * hide: Suppress them
>
> For now, -compat covers only deprecated syntactic aspects of QMP.  We
> may want to extend it to cover semantic aspects, CLI, and experimental
> features.

Some bikeshedding on option naming...

If this only covers QMP, should we make the argument to -compat
have a name that expresses that? eg deprecated-qmp-input,
deprecated-qmp-output ?

Also, it seems a bit repetitive to say 'deprecated' here all
the time -- do you have a future use of -compat in mind which
would be to adjust something that is *not* deprecated ? If
not, maybe the 'deprecated' part should be in the option name
rather than in every argument, eg

  -deprecation-compat qmp-input=crash,qmp-output=hide,cli-option=reject

?

thanks
-- PMM

Re: [PATCH v5 0/8] Configurable policy for handling deprecated interfaces
Posted by Markus Armbruster 3 years, 7 months ago
Peter Maydell <peter.maydell@linaro.org> writes:

> On Mon, 14 Sep 2020 at 09:55, Markus Armbruster <armbru@redhat.com> wrote:
>>
>> New option -compat lets you configure what to do when deprecated
>> interfaces get used.  This is intended for testing users of the
>> management interfaces.  It is experimental.
>>
>> -compat deprecated-input=<in-policy> configures what to do when
>> deprecated input is received.  Available policies:
>>
>> * accept: Accept deprecated commands and arguments (default)
>> * reject: Reject them
>> * crash: Crash
>>
>> -compat deprecated-output=<out-policy> configures what to do when
>> deprecated output is sent.  Available output policies:
>>
>> * accept: Emit deprecated command results and events (default)
>> * hide: Suppress them
>>
>> For now, -compat covers only deprecated syntactic aspects of QMP.  We
>> may want to extend it to cover semantic aspects, CLI, and experimental
>> features.
>
> Some bikeshedding on option naming...
>
> If this only covers QMP, should we make the argument to -compat
> have a name that expresses that? eg deprecated-qmp-input,
> deprecated-qmp-output ?

It's only implemented for QMP so far.  But we really want it for all
external interfaces for use by machines.  Today, that's QMP and CLI.

Naming the parameters deprecated-qmp-{input,output} leads to separate
settings for QMP and CLI.

Naming them just deprecated-{input,output} leads to a single set of
settings common to all externeal interfaces, or to sugar for setting all
the deprecated-*-{input,output} we may have.

I don't think getting it wrong now would be a big deal.  No excuse for
getting it wrong unthinkingly :)

> Also, it seems a bit repetitive to say 'deprecated' here all
> the time -- do you have a future use of -compat in mind which
> would be to adjust something that is *not* deprecated ? If
> not, maybe the 'deprecated' part should be in the option name
> rather than in every argument, eg
>
>   -deprecation-compat qmp-input=crash,qmp-output=hide,cli-option=reject
>
> ?

My cover letter hints at such future uses: "We may want to extend it to
cover [...] experimental features."  Something like

    -compat experimental-input=reject,experimental-output=hide

Re: [PATCH v5 0/8] Configurable policy for handling deprecated interfaces
Posted by Peter Maydell 3 years, 7 months ago
On Mon, 21 Sep 2020 at 15:58, Markus Armbruster <armbru@redhat.com> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
> > If this only covers QMP, should we make the argument to -compat
> > have a name that expresses that? eg deprecated-qmp-input,
> > deprecated-qmp-output ?
>
> It's only implemented for QMP so far.  But we really want it for all
> external interfaces for use by machines.  Today, that's QMP and CLI.
>
> Naming the parameters deprecated-qmp-{input,output} leads to separate
> settings for QMP and CLI.

I think that's a good thing. I might have fixed up my handling
of QMP to avoid deprecated behaviours but not yet got round to
doing that for my command line option choices (or vice-versa).

> > Also, it seems a bit repetitive to say 'deprecated' here all
> > the time -- do you have a future use of -compat in mind which
> > would be to adjust something that is *not* deprecated ? If
> > not, maybe the 'deprecated' part should be in the option name
> > rather than in every argument, eg
> >
> >   -deprecation-compat qmp-input=crash,qmp-output=hide,cli-option=reject
> >
> > ?
>
> My cover letter hints at such future uses: "We may want to extend it to
> cover [...] experimental features."

Ah, I read "-compat covers only deprecated syntactic aspects of QMP.  We
may want to extend it to cover semantic aspects, CLI, and experimental
features." as implying "deprecated semantic aspects, deprecated CLI,
and deprecated experimental features"...

thanks
-- PMM