[RFC PATCH 0/3] Make use of deprecated QMP commands/fields more obvious
This is based on top of the recent capabilities update: https://www.redhat.com/archives/libvir-list/2020-April/msg01490.html Whole series can be fetched by: git fetch https://gitlab.com/pipo.sk/libvirt.git deprecated-crash and requires a qemu which includes Markus' patches adding the -compat feature: https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg05380.html Note that it was pushed partially, for convenience I'm providing a branch with the patches applied and rebased: git fetch https://gitlab.com/pipo.sk/qemu.git compat-deprecated The main idea is to add developers and other interested parties the ability to enable the -compat options to crash qemu in cases when deprecated QMP command is used and stop formating any deprecated response. The qemu code is not pushed yet and doesn't have any way to be detected but I want to query whether such feature makes sense for developers. Peter Krempa (3): qemu: capabilities: Introduce QEMU_CAPS_COMPAT_DEPRECATED qemu: conf: Add 'deprecated_debug' setting to qemu.conf qemu: command: Handle formatting of '-compat' options src/qemu/libvirtd_qemu.aug | 1 + src/qemu/qemu.conf | 11 +++++++++++ src/qemu/qemu_capabilities.c | 4 ++++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 16 ++++++++++++++++ src/qemu/qemu_conf.c | 2 ++ src/qemu/qemu_conf.h | 2 ++ src/qemu/test_libvirtd_qemu.aug.in | 1 + tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml | 1 + 9 files changed, 39 insertions(+) -- 2.26.2
The capability is asserted if qemu supports the -compat deprecated-input= and deprecated-output= settings to control what should happen if deprecated fields are used in QMP. This will be used for a developer-oriented setting which will aid us in catching use of deprecated settings sooner. --- src/qemu/qemu_capabilities.c | 4 ++++ src/qemu/qemu_capabilities.h | 1 + tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml | 1 + 3 files changed, 6 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 47f88481c8..cad65cca65 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -580,6 +580,7 @@ VIR_ENUM_IMPL(virQEMUCaps, "machine.pseries.cap-sbbc", "machine.pseries.cap-ibs", "tcg", + "compat-deprecated" ); @@ -4879,6 +4880,9 @@ virQEMUCapsInitQMPVersionCaps(virQEMUCapsPtr qemuCaps) /* TCG couldn't be disabled nor queried until QEMU 2.10 */ if (qemuCaps->version < 2010000) virQEMUCapsSet(qemuCaps, QEMU_CAPS_TCG); + + if (qemuCaps->version >= 5000050) + virQEMUCapsSet(qemuCaps, QEMU_CAPS_COMPAT_DEPRECATED); } diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index cdeaf09cce..c3e112e940 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -561,6 +561,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ QEMU_CAPS_MACHINE_PSERIES_CAP_SBBC, /* -machine pseries.cap-sbbc */ QEMU_CAPS_MACHINE_PSERIES_CAP_IBS, /* -machine pseries.cap-ibs */ QEMU_CAPS_TCG, /* QEMU does support TCG */ + QEMU_CAPS_COMPAT_DEPRECATED, /* -compat deprecated-(input|output) is supported */ QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml index 9611549bd7..8eafb036ea 100644 --- a/tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml @@ -232,6 +232,7 @@ <flag name='pcie-root-port.hotplug'/> <flag name='aio.io_uring'/> <flag name='tcg'/> + <flag name='compat-deprecated'/> <version>5000050</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100242</microcodeVersion> -- 2.26.2
On 4/30/20 9:13 AM, Peter Krempa wrote:
The capability is asserted if qemu supports the -compat deprecated-input= and deprecated-output= settings to control what should happen if deprecated fields are used in QMP.
This will be used for a developer-oriented setting which will aid us in catching use of deprecated settings sooner. --- src/qemu/qemu_capabilities.c | 4 ++++ src/qemu/qemu_capabilities.h | 1 + tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml | 1 + 3 files changed, 6 insertions(+)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 47f88481c8..cad65cca65 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -580,6 +580,7 @@ VIR_ENUM_IMPL(virQEMUCaps, "machine.pseries.cap-sbbc", "machine.pseries.cap-ibs", "tcg", + "compat-deprecated" );
Missing a trailing comma.
@@ -4879,6 +4880,9 @@ virQEMUCapsInitQMPVersionCaps(virQEMUCapsPtr qemuCaps) /* TCG couldn't be disabled nor queried until QEMU 2.10 */ if (qemuCaps->version < 2010000) virQEMUCapsSet(qemuCaps, QEMU_CAPS_TCG); + + if (qemuCaps->version >= 5000050) + virQEMUCapsSet(qemuCaps, QEMU_CAPS_COMPAT_DEPRECATED); }
And of course, this will be reworked if your request to the qemu list to make it introspectible is resolved. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
New QEMU supports an harsh, but hard to ignore way to notify that the QMP user used an deprecated command. This is useful e.g. for developers to see that something needs to be fixed. This patch introduces a qemu.conf option to enable the setting in cases when qemu supports it so that developers and continiuous integration efforts are notified about use of deprecated fields while it's not late. --- src/qemu/libvirtd_qemu.aug | 1 + src/qemu/qemu.conf | 11 +++++++++++ src/qemu/qemu_conf.c | 2 ++ src/qemu/qemu_conf.h | 2 ++ src/qemu/test_libvirtd_qemu.aug.in | 1 + 5 files changed, 17 insertions(+) diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug index 404498b611..4cf700346f 100644 --- a/src/qemu/libvirtd_qemu.aug +++ b/src/qemu/libvirtd_qemu.aug @@ -118,6 +118,7 @@ module Libvirtd_qemu = let debug_level_entry = int_entry "gluster_debug_level" | bool_entry "virtiofsd_debug" + | bool_entry "deprecated_debug" let memory_entry = str_entry "memory_backing_dir" diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index abdbf07fec..558731da0f 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -852,3 +852,14 @@ # may change across versions. # #capability_filters = [ "capname" ] + +# 'deprecated_debug' setting controls whether qemu should be instructed to crash +# when libvirt uses deprecated commands or arguments and at the same time to +# stop including deprecated fields in replies. This setting is meant for +# developers and continious-integration efforts to make it obvious when +# we rely on features which may go away. In cases when qemu doesn't support +# dealing with deprecated fields the setting is ignored. +# +# DO NOT use in production. +# +#deprecated_debug = 1 diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index c59824006c..34286bf388 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -846,6 +846,8 @@ virQEMUDriverConfigLoadDebugEntry(virQEMUDriverConfigPtr cfg, return -1; if (virConfGetValueBool(conf, "virtiofsd_debug", &cfg->virtiofsdDebug) < 0) return -1; + if (virConfGetValueBool(conf, "deprecated_debug", &cfg->deprecatedDebug) < 0) + return -1; return 0; } diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index b9ef4551a3..830e7f320d 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -218,6 +218,8 @@ struct _virQEMUDriverConfig { gid_t swtpm_group; char **capabilityfilters; + + bool deprecatedDebug; }; G_DEFINE_AUTOPTR_CLEANUP_FUNC(virQEMUDriverConfig, virObjectUnref); diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in index 19da591aae..a45003633f 100644 --- a/src/qemu/test_libvirtd_qemu.aug.in +++ b/src/qemu/test_libvirtd_qemu.aug.in @@ -111,3 +111,4 @@ module Test_libvirtd_qemu = { "capability_filters" { "1" = "capname" } } +{ "deprecated_debug" = "1" } -- 2.26.2
On Thu, Apr 30, 2020 at 04:13:43PM +0200, Peter Krempa wrote:
New QEMU supports an harsh, but hard to ignore way to notify that the QMP user used an deprecated command. This is useful e.g. for developers to see that something needs to be fixed.
This patch introduces a qemu.conf option to enable the setting in cases when qemu supports it so that developers and continiuous integration efforts are notified about use of deprecated fields while it's not late.
--- src/qemu/libvirtd_qemu.aug | 1 + src/qemu/qemu.conf | 11 +++++++++++ src/qemu/qemu_conf.c | 2 ++ src/qemu/qemu_conf.h | 2 ++ src/qemu/test_libvirtd_qemu.aug.in | 1 + 5 files changed, 17 insertions(+)
diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug index 404498b611..4cf700346f 100644 --- a/src/qemu/libvirtd_qemu.aug +++ b/src/qemu/libvirtd_qemu.aug @@ -118,6 +118,7 @@ module Libvirtd_qemu =
let debug_level_entry = int_entry "gluster_debug_level" | bool_entry "virtiofsd_debug" + | bool_entry "deprecated_debug"
let memory_entry = str_entry "memory_backing_dir"
diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index abdbf07fec..558731da0f 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -852,3 +852,14 @@ # may change across versions. # #capability_filters = [ "capname" ] + +# 'deprecated_debug' setting controls whether qemu should be instructed to crash +# when libvirt uses deprecated commands or arguments and at the same time to +# stop including deprecated fields in replies. This setting is meant for +# developers and continious-integration efforts to make it obvious when +# we rely on features which may go away. In cases when qemu doesn't support +# dealing with deprecated fields the setting is ignored. +# +# DO NOT use in production. +# +#deprecated_debug = 1
Calling it "debug" when the result is to "crash" is a bit misleading ! I think this behaviour is a bit too harsh. As a developer I would be happy to enable an option in my qemu.conf to causes log messages to printed to stderr for all my dev servers & VMs, or gracefully rejects a QMP command with an error. I'm very unlikely to ever enable an option that is going to hard crash my VMs, because that risks data loss to me. So I think we ought to make this a tri-state like "qemu_deprecations = ignore|error|crash" 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 :|
On Thu, Apr 30, 2020 at 15:27:58 +0100, Daniel Berrange wrote:
On Thu, Apr 30, 2020 at 04:13:43PM +0200, Peter Krempa wrote:
New QEMU supports an harsh, but hard to ignore way to notify that the QMP user used an deprecated command. This is useful e.g. for developers to see that something needs to be fixed.
This patch introduces a qemu.conf option to enable the setting in cases when qemu supports it so that developers and continiuous integration efforts are notified about use of deprecated fields while it's not late.
--- src/qemu/libvirtd_qemu.aug | 1 + src/qemu/qemu.conf | 11 +++++++++++ src/qemu/qemu_conf.c | 2 ++ src/qemu/qemu_conf.h | 2 ++ src/qemu/test_libvirtd_qemu.aug.in | 1 + 5 files changed, 17 insertions(+)
diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug index 404498b611..4cf700346f 100644 --- a/src/qemu/libvirtd_qemu.aug +++ b/src/qemu/libvirtd_qemu.aug @@ -118,6 +118,7 @@ module Libvirtd_qemu =
let debug_level_entry = int_entry "gluster_debug_level" | bool_entry "virtiofsd_debug" + | bool_entry "deprecated_debug"
let memory_entry = str_entry "memory_backing_dir"
diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index abdbf07fec..558731da0f 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -852,3 +852,14 @@ # may change across versions. # #capability_filters = [ "capname" ] + +# 'deprecated_debug' setting controls whether qemu should be instructed to crash +# when libvirt uses deprecated commands or arguments and at the same time to +# stop including deprecated fields in replies. This setting is meant for +# developers and continious-integration efforts to make it obvious when +# we rely on features which may go away. In cases when qemu doesn't support +# dealing with deprecated fields the setting is ignored. +# +# DO NOT use in production. +# +#deprecated_debug = 1
Calling it "debug" when the result is to "crash" is a bit misleading ! I think this behaviour is a bit too harsh. As a developer I would be happy to enable an option in my qemu.conf to causes log messages to printed to stderr for all my dev servers & VMs, or gracefully rejects a QMP command with an error. I'm very unlikely to ever enable an option that is going to hard crash my VMs, because that risks data loss to me.
Fair enough, qemu supports these options. It comes mostly from how we use VMs though. I mostly have VMs which boot from a CD or without any OS, because I simply don't need it for most cases so for me, crashing is something which I notice and don't risk anything. Reporting errors to logs though is similarly has it's drawbacks. You enable it, but if you don't chekc the log file you will not notice it. I'll probably ask for a possibility to return error and ignore the QMP command.
Enable '-compat' if requested in qemu.conf and supported by qemu to instruct qemu to crash when a deprecated command is used and stop returning deprecated fields. This setting is meant for libvirt developers and such. --- src/qemu/qemu_command.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 6acfc0b19d..5a4769e969 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9801,6 +9801,20 @@ qemuBuildVsockCommandLine(virCommandPtr cmd, } +static void +qemuBuildCompatDeprecatedCommandLine(virCommandPtr cmd, + virQEMUDriverConfigPtr cfg, + virQEMUCapsPtr qemuCaps) +{ + if (!cfg->deprecatedDebug || + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_COMPAT_DEPRECATED)) + return; + + virCommandAddArgList(cmd, "-compat", + "deprecated-input=crash,deprecated-output=hide", NULL); +} + + /* * Constructs a argv suitable for launching qemu with config defined * for a given virtual machine. @@ -9860,6 +9874,8 @@ qemuBuildCommandLine(virQEMUDriverPtr driver, if (qemuBuildNameCommandLine(cmd, cfg, def, qemuCaps) < 0) return NULL; + qemuBuildCompatDeprecatedCommandLine(cmd, cfg, qemuCaps); + if (!standalone) virCommandAddArg(cmd, "-S"); /* freeze CPU */ -- 2.26.2
participants (3)
-
Daniel P. Berrangé -
Eric Blake -
Peter Krempa