Skip to contentSkip to conversation

Introducing QemuSupportState

PATCH In review Gerd Hoffmann 7 participants 18 messages Nov 6, 2018 – Nov 29, 2018

lore
Share & tools
lore
#0 lore raw

v2 cover Introducing QemuSupportState

Trying to fill the need to be more finegrained on support status.

v2:
- reduce the number of support states, add documentation for them.
- move QemuSupportState to ObjectClass, to simplify introspection integration (not done yet).
- add UsageHints.

Gerd Hoffmann (6):
move ObjectClass to typedefs.h
add QemuSupportState
Use QemuSupportState for machine types.
Warn on obsolete and deprecated devices.
tag cirrus as obsolete
add UsageHints to QemuSupportState

 include/hw/boards.h          |  3 ---
 include/qemu/support-state.h | 18 +++++++++++++
 include/qemu/typedefs.h      |  1 +
 include/qom/object.h         |  4 ++-
 hw/core/qdev.c               |  8 +++++-
 hw/display/cirrus_vga.c      |  3 +++
 hw/display/cirrus_vga_isa.c  |  3 +++
 hw/i386/pc_piix.c            |  5 +++-
 hw/ppc/prep.c                |  5 +++-
 qdev-monitor.c               |  9 +++++++
 util/support-state.c         | 26 +++++++++++++++++++
 vl.c                         |  7 ++---
 qapi/common.json             | 61 ++++++++++++++++++++++++++++++++++++++++++++
 util/Makefile.objs           |  1 +
 14 files changed, 144 insertions(+), 10 deletions(-)
 create mode 100644 include/qemu/support-state.h
 create mode 100644 util/support-state.c
Signature · 1 line
2.9.3
#1 lore raw

PATCH v2 1/6 move ObjectClass to typedefs.h

---
 include/qemu/typedefs.h | 1 +
 include/qom/object.h    | 1 -
 2 files changed, 1 insertion(+), 1 deletion(-)
include/qemu/typedefs.h +1 -0
index 3ec0e13a96..fed53f6de2 100644
@@ -62,6 +62,7 @@ typedef struct NetClientState NetClientState;
 typedef struct NetFilterState NetFilterState;
 typedef struct NICInfo NICInfo;
 typedef struct NumaNodeMem NumaNodeMem;
+typedef struct ObjectClass ObjectClass;
 typedef struct PCIBridge PCIBridge;
 typedef struct PCIBus PCIBus;
 typedef struct PCIDevice PCIDevice;
include/qom/object.h +0 -1
index f0b0bf39cc..499e1fd8b7 100644
@@ -20,7 +20,6 @@
 struct TypeImpl;
 typedef struct TypeImpl *Type;
 
-typedef struct ObjectClass ObjectClass;
 typedef struct Object Object;
 
 typedef struct TypeInfo TypeInfo;
Signature · 1 line
2.9.3
Reviewed-by on v2 1/6
#2 lore raw

Re: [Qemu-devel] [PATCH v2 1/6] move ObjectClass to typedefs.h

Quoted 1 line · Gerd Hoffmann
Quoted 29 lines
---
 include/qemu/typedefs.h | 1 +
 include/qom/object.h    | 1 -
 2 files changed, 1 insertion(+), 1 deletion(-)
include/qemu/typedefs.h +1 -0
index 3ec0e13a96..fed53f6de2 100644
@@ -62,6 +62,7 @@ typedef struct NetClientState NetClientState;
 typedef struct NetFilterState NetFilterState;
 typedef struct NICInfo NICInfo;
 typedef struct NumaNodeMem NumaNodeMem;
+typedef struct ObjectClass ObjectClass;
 typedef struct PCIBridge PCIBridge;
 typedef struct PCIBus PCIBus;
 typedef struct PCIDevice PCIDevice;
include/qom/object.h +0 -1
index f0b0bf39cc..499e1fd8b7 100644
@@ -20,7 +20,6 @@
 struct TypeImpl;
 typedef struct TypeImpl *Type;
 
-typedef struct ObjectClass ObjectClass;
 typedef struct Object Object;
 
 typedef struct TypeInfo TypeInfo;
Signature · 4 lines
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
Reviewed-by Tested-by on v2 1/6
#3 lore raw

Re: [Qemu-devel] [PATCH v2 1/6] move ObjectClass to typedefs.h

In reply to Gerd Hoffmann
Quoted 1 line · Gerd Hoffmann
Quoted 30 lines
---
  include/qemu/typedefs.h | 1 +
  include/qom/object.h    | 1 -
  2 files changed, 1 insertion(+), 1 deletion(-)
include/qemu/typedefs.h +1 -0
index 3ec0e13a96..fed53f6de2 100644
@@ -62,6 +62,7 @@ typedef struct NetClientState NetClientState;
  typedef struct NetFilterState NetFilterState;
  typedef struct NICInfo NICInfo;
  typedef struct NumaNodeMem NumaNodeMem;
+typedef struct ObjectClass ObjectClass;
  typedef struct PCIBridge PCIBridge;
  typedef struct PCIBus PCIBus;
  typedef struct PCIDevice PCIDevice;
include/qom/object.h +0 -1
index f0b0bf39cc..499e1fd8b7 100644
@@ -20,7 +20,6 @@
  struct TypeImpl;
  typedef struct TypeImpl *Type;
 
-typedef struct ObjectClass ObjectClass;
  typedef struct Object Object;
 
  typedef struct TypeInfo TypeInfo;
#4 lore raw

PATCH v2 2/6 add QemuSupportState

In reply to Gerd Hoffmann

Indicates support state for something (device, backend, subsystem, ...) in qemu. Add QemuSupportState field to ObjectClass. Add some support code.

TODO: wire up to qom-list-types

---
 include/qemu/support-state.h | 17 +++++++++++++++++
 include/qom/object.h         |  3 +++
 util/support-state.c         | 26 ++++++++++++++++++++++++++
 qapi/common.json             | 32 ++++++++++++++++++++++++++++++++
 util/Makefile.objs           |  1 +
 5 files changed, 79 insertions(+)
 create mode 100644 include/qemu/support-state.h
 create mode 100644 util/support-state.c
include/qemu/support-state.h new file +17 -0
new file mode 100644
index 0000000000..6567d8702b
@@ -0,0 +1,17 @@
+#ifndef QEMU_SUPPORT_STATE_H
+#define QEMU_SUPPORT_STATE_H
+
+#include "qapi/qapi-types-common.h"
+
+typedef struct QemuSupportState {
+    SupportState state;
+    const char   *help;
+} QemuSupportState;
+
+void qemu_warn_support_state(const char *type, const char *name,
+                             ObjectClass *oc);
+
+bool qemu_is_deprecated(ObjectClass *oc);
+bool qemu_is_obsolete(ObjectClass *oc);
+
+#endif /* QEMU_SUPPORT_STATE_H */
include/qom/object.h +3 -0
index 499e1fd8b7..2b5f27bf85 100644
@@ -15,6 +15,7 @@
 #define QEMU_OBJECT_H
 
 #include "qapi/qapi-builtin-types.h"
+#include "qemu/support-state.h"
 #include "qemu/queue.h"
 
 struct TypeImpl;
@@ -399,6 +400,8 @@ struct ObjectClass
     ObjectUnparent *unparent;
 
     GHashTable *properties;
+
+    QemuSupportState supported;
 };
 
 /**
util/support-state.c new file +26 -0
new file mode 100644
index 0000000000..3170dbd15d
@@ -0,0 +1,26 @@
+#include "qemu/osdep.h"
+#include "qemu/error-report.h"
+#include "qemu/support-state.h"
+#include "qom/object.h"
+
+void qemu_warn_support_state(const char *type, const char *name,
+                             ObjectClass *oc)
+{
+    const char *help = oc->supported.help;
+
+    warn_report("%s %s is %s%s%s%s", type, name,
+                SupportState_str(oc->supported.state),
+                help ? " (" : "",
+                help ? help : "",
+                help ? ")"  : "");
+}
+
+bool qemu_is_deprecated(ObjectClass *oc)
+{
+    return oc->supported.state == SUPPORT_STATE_DEPRECATED;
+}
+
+bool qemu_is_obsolete(ObjectClass *oc)
+{
+    return oc->supported.state == SUPPORT_STATE_OBSOLETE;
+}
qapi/common.json +32 -0
index 021174f04e..00374127b8 100644
@@ -151,3 +151,35 @@
              'ppc64', 'riscv32', 'riscv64', 's390x', 'sh4',
              'sh4eb', 'sparc', 'sparc64', 'tricore', 'unicore32',
              'x86_64', 'xtensa', 'xtensaeb' ] }
+
+##
+# @SupportState:
+#
+# Indicate Support level of qemu devices, backends, subsystems, ...
+#
+# @unspecified: not specified (zero-initialized).
+#
+# @experimental: in development, can be unstable or incomplete.
+#
+# @supported: works stable and is fully supported.
+#             (supported + maintained in MAINTAINERS).
+#
+# @unsupported: should work, support is weak or not present.
+#               (odd-fixes + orphan in MAINTAINERS).
+#
+# @obsolete: is obsolete, still present for compatibility reasons,
+#            will likely be removed at some point in the future.
+#            Not deprecated (yet).
+#            (obsolete in MAINTAINERS).
+#
+# @deprecated: is deprecated, according to qemu deprecation policy.
+#
+# Since: 3.2
+##
+{ 'enum': 'SupportState',
+  'data': [ 'unspecified',
+            'experimental',
+            'supported',
+            'unsupported',
+            'obsolete',
+            'deprecated' ] }
util/Makefile.objs +1 -0
index 0820923c18..6e5f8faf82 100644
@@ -50,5 +50,6 @@ util-obj-y += range.o
 util-obj-y += stats64.o
 util-obj-y += systemd.o
 util-obj-y += iova-tree.o
+util-obj-y += support-state.o
 util-obj-$(CONFIG_LINUX) += vfio-helpers.o
 util-obj-$(CONFIG_OPENGL) += drm.o
Signature · 1 line
2.9.3
#5 lore raw

Re: [Qemu-devel] [PATCH v2 2/6] add QemuSupportState

Quoted 8 lines · Gerd Hoffmann

Indicates support state for something (device, backend, subsystem, ...) in qemu. Add QemuSupportState field to ObjectClass. Add some support code.

TODO: wire up to qom-list-types

---

[...]

qapi/common.json line 162

Quoted 8 lines
qapi/common.json +8 -0
+##
+# @SupportState:
+#
+# Indicate Support level of qemu devices, backends, subsystems, ...
+#
+# @unspecified: not specified (zero-initialized).
+#
+# @experimental: in development, can be unstable or incomplete.

People reading this document would ask: what would appear on MAINTAINERS if SupportState is experimental?

qapi/common.json line 168

Quoted 6 lines
qapi/common.json +6 -0
+#
+# @supported: works stable and is fully supported.
+#             (supported + maintained in MAINTAINERS).
+#
+# @unsupported: should work, support is weak or not present.
+#               (odd-fixes + orphan in MAINTAINERS).

What's the difference in practice between unsupported and experimental?

qapi/common.json line 175

Quoted 7 lines
qapi/common.json +7 -0
+#
+# @obsolete: is obsolete, still present for compatibility reasons,
+#            will likely be removed at some point in the future.
+#            Not deprecated (yet).
+#            (obsolete in MAINTAINERS).
+#
+# @deprecated: is deprecated, according to qemu deprecation policy.

I believe we want to differentiate "deprecated, but still safe to use in production if you have a migration plan" from "deprecated, and also unstable and unsafe for production".

I expect enterprise distributions to have a strict policy of not allowing unsupported and experimental devices to be enabled, but still allow deprecated devices to be enabled (but only if they are stable/supported).

Signature · 1 line
Eduardo
#6 lore raw

Re: [Qemu-devel] [PATCH v2 2/6] add QemuSupportState

Quoted 10 lines · Eduardo Habkost
Quoted 8 lines · Gerd Hoffmann

Indicates support state for something (device, backend, subsystem, ...) in qemu. Add QemuSupportState field to ObjectClass. Add some support code.

TODO: wire up to qom-list-types

---

[...]

Quoted 11 lines

qapi/common.json line 162

Quoted 8 lines
qapi/common.json +8 -0
+##
+# @SupportState:
+#
+# Indicate Support level of qemu devices, backends, subsystems, ...
+#
+# @unspecified: not specified (zero-initialized).
+#
+# @experimental: in development, can be unstable or incomplete.

People reading this document would ask: what would appear on MAINTAINERS if SupportState is experimental?

Probably Maintained. It's something that is on its way towards becoming "supported", but still too immature ("unstable or incomplete").

Quoted 10 lines

qapi/common.json line 168

Quoted 6 lines
qapi/common.json +6 -0
+#
+# @supported: works stable and is fully supported.
+#             (supported + maintained in MAINTAINERS).
+#
+# @unsupported: should work, support is weak or not present.
+#               (odd-fixes + orphan in MAINTAINERS).

What's the difference in practice between unsupported and experimental?

Quoted 3 lines

qapi/common.json line 171

Quoted 3 lines
qapi/common.json +3 -0
+#
+# @obsolete: is obsolete, still present for compatibility reasons,
+#            will likely be removed at some point in the future.

I am not sure this is necessarily true. I don't see Cirrus or adlib or pcnet disappearing anytime soon.

Paolo

Quoted 14 lines

qapi/common.json line 175

Quoted 4 lines
qapi/common.json +4 -0
+#            Not deprecated (yet).
+#            (obsolete in MAINTAINERS).
+#
+# @deprecated: is deprecated, according to qemu deprecation policy.

I believe we want to differentiate "deprecated, but still safe to use in production if you have a migration plan" from "deprecated, and also unstable and unsafe for production".

I expect enterprise distributions to have a strict policy of not allowing unsupported and experimental devices to be enabled, but still allow deprecated devices to be enabled (but only if they are stable/supported).

#7 lore raw

Re: [Qemu-devel] [PATCH v2 2/6] add QemuSupportState

Hi,

Quoted 7 lines
Quoted 4 lines
Quoted 1 line

+# @experimental: in development, can be unstable or incomplete.

People reading this document would ask: what would appear on MAINTAINERS if SupportState is experimental?

Probably Maintained. It's something that is on its way towards becoming "supported", but still too immature ("unstable or incomplete").

Yep. For the user there isn't much of a difference between "experimental" and "unsupported" though, except that "experimental" indicates that someone is working on it and the chances are higher that it'll transition to "supported" at some point in the future. So I'm not fully sure it makes sense to list that as separate category.

Quoted 5 lines
Quoted 2 lines

qapi/common.json line 171

Quoted 2 lines
qapi/common.json +2 -0
+# @obsolete: is obsolete, still present for compatibility reasons,
+#            will likely be removed at some point in the future.

I am not sure this is necessarily true. I don't see Cirrus or adlib or pcnet disappearing anytime soon.

So s/will likely/might/ ?

cheers,
Gerd

#8 lore raw

PATCH v2 3/6 Use QemuSupportState for machine types.

In reply to Gerd Hoffmann

Switch over the current deprecation_reason users to use the QemuSupportState field in ObjectClass instead.

---
 include/hw/boards.h | 3 ---
 hw/i386/pc_piix.c   | 5 ++++-
 hw/ppc/prep.c       | 5 ++++-
 vl.c                | 7 ++++---
 4 files changed, 12 insertions(+), 8 deletions(-)
include/hw/boards.h +0 -3
index f82f28468b..b7bb181fc8 100644
@@ -106,8 +106,6 @@ typedef struct {
 
 /**
  * MachineClass:
- * @deprecation_reason: If set, the machine is marked as deprecated. The
- *    string should provide some clear information about what to use instead.
  * @max_cpus: maximum number of CPUs supported. Default: 1
  * @min_cpus: minimum number of CPUs supported. Default: 1
  * @default_cpus: number of CPUs instantiated if none are specified. Default: 1
@@ -167,7 +165,6 @@ struct MachineClass {
     char *name;
     const char *alias;
     const char *desc;
-    const char *deprecation_reason;
 
     void (*init)(MachineState *state);
     void (*reset)(void);
hw/i386/pc_piix.c +4 -1
index dc09466b3e..407f4503a3 100644
@@ -955,9 +955,12 @@ DEFINE_I440FX_MACHINE(v0_12, "pc-0.12", pc_compat_0_13,
 
 static void pc_i440fx_0_11_machine_options(MachineClass *m)
 {
+    ObjectClass *oc = OBJECT_CLASS(m);
+
     pc_i440fx_0_12_machine_options(m);
     m->hw_version = "0.11";
-    m->deprecation_reason = "use a newer machine type instead";
+    oc->supported.state = SUPPORT_STATE_DEPRECATED;
+    oc->supported.help = "use a newer machine type instead";
     SET_MACHINE_COMPAT(m, PC_COMPAT_0_11);
 }
 
hw/ppc/prep.c +4 -1
index 2afb7f437e..260700847b 100644
@@ -588,7 +588,10 @@ static void ppc_prep_init(MachineState *machine)
 
 static void prep_machine_init(MachineClass *mc)
 {
-    mc->deprecation_reason = "use 40p machine type instead";
+    ObjectClass *oc = OBJECT_CLASS(mc);
+
+    oc->supported.state = SUPPORT_STATE_DEPRECATED;
+    oc->supported.help = "use 40p machine type instead";
     mc->desc = "PowerPC PREP platform";
     mc->init = ppc_prep_init;
     mc->block_default_type = IF_IDE;
vl.c +4 -3
index 1fcacc5caa..ba81fefb66 100644
@@ -2573,7 +2573,7 @@ static gint machine_class_cmp(gconstpointer a, gconstpointer b)
             }
             printf("%-20s %s%s%s\n", mc->name, mc->desc,
                    mc->is_default ? " (default)" : "",
-                   mc->deprecation_reason ? " (deprecated)" : "");
+                   qemu_is_deprecated(OBJECT_CLASS(mc)) ? " (deprecated)" : "");
         }
     }
 
@@ -4290,9 +4290,10 @@ int main(int argc, char **argv, char **envp)
 
     configure_accelerator(current_machine);
 
-    if (!qtest_enabled() && machine_class->deprecation_reason) {
+    if (!qtest_enabled() && qemu_is_deprecated(OBJECT_CLASS(machine_class))) {
         error_report("Machine type '%s' is deprecated: %s",
-                     machine_class->name, machine_class->deprecation_reason);
+                     machine_class->name,
+                     OBJECT_CLASS(machine_class)->supported.help);
     }
 
     /*
Signature · 1 line
2.9.3
#9 lore raw

Re: [Qemu-devel] [PATCH v2 3/6] Use QemuSupportState for machine types.

Quoted 82 lines · Gerd Hoffmann

Switch over the current deprecation_reason users to use the QemuSupportState field in ObjectClass instead.

---
 include/hw/boards.h | 3 ---
 hw/i386/pc_piix.c   | 5 ++++-
 hw/ppc/prep.c       | 5 ++++-
 vl.c                | 7 ++++---
 4 files changed, 12 insertions(+), 8 deletions(-)
include/hw/boards.h +0 -3
index f82f28468b..b7bb181fc8 100644
@@ -106,8 +106,6 @@ typedef struct {
 
 /**
  * MachineClass:
- * @deprecation_reason: If set, the machine is marked as deprecated. The
- *    string should provide some clear information about what to use instead.
  * @max_cpus: maximum number of CPUs supported. Default: 1
  * @min_cpus: minimum number of CPUs supported. Default: 1
  * @default_cpus: number of CPUs instantiated if none are specified. Default: 1
@@ -167,7 +165,6 @@ struct MachineClass {
     char *name;
     const char *alias;
     const char *desc;
-    const char *deprecation_reason;
 
     void (*init)(MachineState *state);
     void (*reset)(void);
hw/i386/pc_piix.c +4 -1
index dc09466b3e..407f4503a3 100644
@@ -955,9 +955,12 @@ DEFINE_I440FX_MACHINE(v0_12, "pc-0.12", pc_compat_0_13,
 
 static void pc_i440fx_0_11_machine_options(MachineClass *m)
 {
+    ObjectClass *oc = OBJECT_CLASS(m);
+
     pc_i440fx_0_12_machine_options(m);
     m->hw_version = "0.11";
-    m->deprecation_reason = "use a newer machine type instead";
+    oc->supported.state = SUPPORT_STATE_DEPRECATED;
+    oc->supported.help = "use a newer machine type instead";
     SET_MACHINE_COMPAT(m, PC_COMPAT_0_11);
 }
 
hw/ppc/prep.c +4 -1
index 2afb7f437e..260700847b 100644
@@ -588,7 +588,10 @@ static void ppc_prep_init(MachineState *machine)
 
 static void prep_machine_init(MachineClass *mc)
 {
-    mc->deprecation_reason = "use 40p machine type instead";
+    ObjectClass *oc = OBJECT_CLASS(mc);
+
+    oc->supported.state = SUPPORT_STATE_DEPRECATED;
+    oc->supported.help = "use 40p machine type instead";
     mc->desc = "PowerPC PREP platform";
     mc->init = ppc_prep_init;
     mc->block_default_type = IF_IDE;
vl.c +1 -1
index 1fcacc5caa..ba81fefb66 100644
@@ -2573,7 +2573,7 @@ static gint machine_class_cmp(gconstpointer a, gconstpointer b)
             }
             printf("%-20s %s%s%s\n", mc->name, mc->desc,
                    mc->is_default ? " (default)" : "",
-                   mc->deprecation_reason ? " (deprecated)" : "");
+                   qemu_is_deprecated(OBJECT_CLASS(mc)) ? " (deprecated)" : "");
         }
     }
 
@@ -4290,9 +4290,10 @@ int main(int argc, char **argv, char **envp)
 
     configure_accelerator(current_machine);

vl.c int main(int argc, char **argv, char **envp) · line 4299

Quoted 10 lines
vl.c +3 -2
 
-    if (!qtest_enabled() && machine_class->deprecation_reason) {
+    if (!qtest_enabled() && qemu_is_deprecated(OBJECT_CLASS(machine_class))) {
         error_report("Machine type '%s' is deprecated: %s",
-                     machine_class->name, machine_class->deprecation_reason);
+                     machine_class->name,
+                     OBJECT_CLASS(machine_class)->supported.help);
     }
 
     /*

Let's switch to warn_report() while we're touching it.

#10 lore raw

PATCH v2 4/6 Warn on obsolete and deprecated devices.

In reply to Gerd Hoffmann

Print a warning for deprecated and obsolete devices. Also add support state to device listing.

---
 hw/core/qdev.c | 8 +++++++-
 qdev-monitor.c | 9 +++++++++
 2 files changed, 16 insertions(+), 1 deletion(-)
hw/core/qdev.c +7 -1
index 6b3cc55b27..6205522c3e 100644
@@ -133,11 +133,17 @@ DeviceState *qdev_create(BusState *bus, const char *name)
 
 DeviceState *qdev_try_create(BusState *bus, const char *type)
 {
+    ObjectClass *oc;
     DeviceState *dev;
 
-    if (object_class_by_name(type) == NULL) {
+    oc = object_class_by_name(type);
+    if (oc == NULL) {
         return NULL;
     }
+    if (qemu_is_deprecated(oc) ||
+        qemu_is_obsolete(oc)) {
+        qemu_warn_support_state("device", type, oc);
+    }
     dev = DEVICE(object_new(type));
     if (!dev) {
         return NULL;
qdev-monitor.c +9 -0
index 802c18a74e..80370372f9 100644
@@ -115,6 +115,8 @@ static void out_printf(const char *fmt, ...)
 
 static void qdev_print_devinfo(DeviceClass *dc)
 {
+    ObjectClass *oc = OBJECT_CLASS(dc);
+
     out_printf("name \"%s\"", object_class_get_name(OBJECT_CLASS(dc)));
     if (dc->bus_type) {
         out_printf(", bus %s", dc->bus_type);
@@ -128,6 +130,9 @@ static void qdev_print_devinfo(DeviceClass *dc)
     if (!dc->user_creatable) {
         out_printf(", no-user");
     }
+    if (oc->supported.state != SUPPORT_STATE_UNSPECIFIED) {
+        out_printf(", %s", SupportState_str(oc->supported.state));
+    }
     out_printf("\n");
 }
 
@@ -579,6 +584,10 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
     if (!dc) {
         return NULL;
     }
+    if (qemu_is_deprecated(OBJECT_CLASS(dc)) ||
+        qemu_is_obsolete(OBJECT_CLASS(dc))) {
+        qemu_warn_support_state("device", driver, OBJECT_CLASS(dc));
+    }
 
     /* find bus */
     path = qemu_opt_get(opts, "bus");
Signature · 1 line
2.9.3
#11 lore raw

Re: [Qemu-devel] [PATCH v2 4/6] Warn on obsolete and deprecated devices.

Quoted 4 lines · Gerd Hoffmann

Print a warning for deprecated and obsolete devices. Also add support state to device listing.

Should we also add a runtime flag to block these?
E.g. I can see libvirt doing that and passing
the responsibility for what is reasonably
supported to qemu.

Quoted 64 lines
---
 hw/core/qdev.c | 8 +++++++-
 qdev-monitor.c | 9 +++++++++
 2 files changed, 16 insertions(+), 1 deletion(-)
hw/core/qdev.c +7 -1
index 6b3cc55b27..6205522c3e 100644
@@ -133,11 +133,17 @@ DeviceState *qdev_create(BusState *bus, const char *name)
 
 DeviceState *qdev_try_create(BusState *bus, const char *type)
 {
+    ObjectClass *oc;
     DeviceState *dev;
 
-    if (object_class_by_name(type) == NULL) {
+    oc = object_class_by_name(type);
+    if (oc == NULL) {
         return NULL;
     }
+    if (qemu_is_deprecated(oc) ||
+        qemu_is_obsolete(oc)) {
+        qemu_warn_support_state("device", type, oc);
+    }
     dev = DEVICE(object_new(type));
     if (!dev) {
         return NULL;
qdev-monitor.c +9 -0
index 802c18a74e..80370372f9 100644
@@ -115,6 +115,8 @@ static void out_printf(const char *fmt, ...)
 
 static void qdev_print_devinfo(DeviceClass *dc)
 {
+    ObjectClass *oc = OBJECT_CLASS(dc);
+
     out_printf("name \"%s\"", object_class_get_name(OBJECT_CLASS(dc)));
     if (dc->bus_type) {
         out_printf(", bus %s", dc->bus_type);
@@ -128,6 +130,9 @@ static void qdev_print_devinfo(DeviceClass *dc)
     if (!dc->user_creatable) {
         out_printf(", no-user");
     }
+    if (oc->supported.state != SUPPORT_STATE_UNSPECIFIED) {
+        out_printf(", %s", SupportState_str(oc->supported.state));
+    }
     out_printf("\n");
 }
 
@@ -579,6 +584,10 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
     if (!dc) {
         return NULL;
     }
+    if (qemu_is_deprecated(OBJECT_CLASS(dc)) ||
+        qemu_is_obsolete(OBJECT_CLASS(dc))) {
+        qemu_warn_support_state("device", driver, OBJECT_CLASS(dc));
+    }
 
     /* find bus */
     path = qemu_opt_get(opts, "bus");
Signature · 1 line
2.9.3
#12 lore raw

Re: [Qemu-devel] [PATCH v2 4/6] Warn on obsolete and deprecated devices.

Quoted 7 lines · Michael S. Tsirkin
Quoted 4 lines · Gerd Hoffmann

Print a warning for deprecated and obsolete devices. Also add support state to device listing.

Should we also add a runtime flag to block these?

Yes, probably. My priority is to hash out the classification first though, so it isn't in this series yet.

On the cmd line switches: How much flexibility do we want on this? We can go for a simple -future switch, as suggested by armbru, disallowing obsolete and deprecated devices. We could go for a full-blown ...

-policy {experimental,unsupported,...}={enable,warn,disable}

... or something inbetween.

Comments? Other ideas?

cheers,
Gerd

#13 lore raw

Re: [Qemu-devel] [PATCH v2 4/6] Warn on obsolete and deprecated devices.

In reply to Gerd Hoffmann
Quoted 18 lines · Gerd Hoffmann

Print a warning for deprecated and obsolete devices. Also add support state to device listing.

---
 hw/core/qdev.c | 8 +++++++-
 qdev-monitor.c | 9 +++++++++
 2 files changed, 16 insertions(+), 1 deletion(-)
hw/core/qdev.c +1 -0
index 6b3cc55b27..6205522c3e 100644
@@ -133,11 +133,17 @@ DeviceState *qdev_create(BusState *bus, const char *name)
 
 DeviceState *qdev_try_create(BusState *bus, const char *type)
 {
+    ObjectClass *oc;

hw/core/qdev.c DeviceState *qdev_create(BusState *bus, const char *name) · line 145

Quoted 10 lines
hw/core/qdev.c +5 -1
     DeviceState *dev;
 
-    if (object_class_by_name(type) == NULL) {
+    oc = object_class_by_name(type);
+    if (oc == NULL) {
         return NULL;
     }
+    if (qemu_is_deprecated(oc) ||
+        qemu_is_obsolete(oc)) {
+        qemu_warn_support_state("device", type, oc);

Looks like this:

    $ qemu-system-x86_64 -nodefaults -S -display none -device cirrus-vga
    qemu-system-x86_64: -device cirrus-vga: warning: device cirrus-vga is obsolete (use "-vga std" instead, see https://www.kraxel.org/blog/2014/10/qemu-using-cirrus-considered-harmful/)

I'd prefer:

    $ qemu-system-x86_64 -nodefaults -S -display none -device cirrus-vga
    qemu-system-x86_64: -device cirrus-vga: warning: device cirrus-vga is obsolete
    Use "-vga std" instead, see
    https://www.kraxel.org/blog/2014/10/qemu-using-cirrus-considered-harmful/

The obvious way to get it:

   void warn_support_state(const char *type, const char *name,
                           ObjectClass *oc)
   {
       const char *help = oc->supported.help;
       warn_report("%s %s is %s", type, name,
                   SupportState_str(oc->supported.state));
       if (help) {
           error_printf("%s\n", help);
       }

}

with the ->help suitably capitalized and formatted.

That should make qemu_warn_support_state() usable for the previous patch, too.

Note I scratched the qemu_ prefix. Matter of taste, I guess.

Quoted 4 lines
hw/core/qdev.c +1 -0
+    }
     dev = DEVICE(object_new(type));
     if (!dev) {
         return NULL;

[...]

#14 lore raw

PATCH v2 5/6 tag cirrus as obsolete

In reply to Gerd Hoffmann

Standard VGA should be used instead.

---
 hw/display/cirrus_vga.c     | 3 +++
 hw/display/cirrus_vga_isa.c | 3 +++
 2 files changed, 6 insertions(+)
hw/display/cirrus_vga.c +3 -0
index d9b854d74d..8474415687 100644
@@ -3024,6 +3024,9 @@ static void cirrus_vga_class_init(ObjectClass *klass, void *data)
     dc->vmsd = &vmstate_pci_cirrus_vga;
     dc->props = pci_vga_cirrus_properties;
     dc->hotpluggable = false;
+    klass->supported.state = SUPPORT_STATE_OBSOLETE;
+    klass->supported.help = "use \"-vga std\" instead, see "
+        "https://www.kraxel.org/blog/2014/10/qemu-using-cirrus-considered-harmful/";
 }
 
 static const TypeInfo cirrus_vga_info = {
hw/display/cirrus_vga_isa.c +3 -0
index fa10b74230..6bc83f7509 100644
@@ -81,6 +81,9 @@ static void isa_cirrus_vga_class_init(ObjectClass *klass, void *data)
     dc->realize = isa_cirrus_vga_realizefn;
     dc->props = isa_cirrus_vga_properties;
     set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
+    klass->supported.state = SUPPORT_STATE_OBSOLETE;
+    klass->supported.help = "use \"-vga std\" instead, see "
+        "https://www.kraxel.org/blog/2014/10/qemu-using-cirrus-considered-harmful/";
 }
 
 static const TypeInfo isa_cirrus_vga_info = {
Signature · 1 line
2.9.3
#15 lore raw

PATCH v2 6/6 add UsageHints to QemuSupportState

In reply to Gerd Hoffmann

So we can add device usage recommendations to devices, independant from support state.

---
 include/qemu/support-state.h |  1 +
 qapi/common.json             | 29 +++++++++++++++++++++++++++++
 2 files changed, 30 insertions(+)
include/qemu/support-state.h +1 -0
index 6567d8702b..6ea0d03bd5 100644
@@ -5,6 +5,7 @@
 
 typedef struct QemuSupportState {
     SupportState state;
+    UsageHints   hints;
     const char   *help;
 } QemuSupportState;
 
qapi/common.json +29 -0
index 00374127b8..6952ea2074 100644
@@ -183,3 +183,32 @@
             'unsupported',
             'obsolete',
             'deprecated' ] }
+
+##
+# @UsageHints:
+#
+# Usage recommendations.
+#
+# @unspecified: not specified (zero-initialized).
+#
+# @green: Best choice.  Typically paravirtual devices go into this
+#         category.
+#         Example (nic): virtio-net.
+#         Example (usb); xhci.
+#
+# @yellow: Reasonable choice.  Typically emulated devices with
+#          good performance go into this category.
+#          Example (nic): e1000, e1000e
+#
+# @red: Bad choice.  Avoid this unless you run an old guest which
+#       lacks support for something better.
+#       Example (nic): rtl8139, pcnet, ne2k, ...
+#       Example (usb): ehci, uhci, ohci
+#
+# Since: 3.2
+##
+{ 'enum': 'UsageHints',
+  'data': [ 'unspecified',
+            'green',
+            'yellow',
+            'red' ] }
Signature · 1 line
2.9.3
#16 lore raw

Re: [Qemu-devel] [PATCH v2 6/6] add UsageHints to QemuSupportState

Quoted 26 lines · Gerd Hoffmann

So we can add device usage recommendations to devices, independant from support state.

---
 include/qemu/support-state.h |  1 +
 qapi/common.json             | 29 +++++++++++++++++++++++++++++
 2 files changed, 30 insertions(+)
include/qemu/support-state.h +1 -0
index 6567d8702b..6ea0d03bd5 100644
@@ -5,6 +5,7 @@
 
 typedef struct QemuSupportState {
     SupportState state;
+    UsageHints   hints;
     const char   *help;
 } QemuSupportState;
 
qapi/common.json +0 -0
index 00374127b8..6952ea2074 100644

qapi/common.json line 192

Quoted 10 lines
qapi/common.json +7 -0
@@ -183,3 +183,32 @@
             'unsupported',
             'obsolete',
             'deprecated' ] }
+
+##
+# @UsageHints:
+#
+# Usage recommendations.
+#
+# @unspecified: not specified (zero-initialized).

(zero-initialized) is an implementation detail, which in my opinion doesn't belong here. Same in PATCH 2.

Quoted 12 lines
qapi/common.json +12 -0
+#
+# @green: Best choice.  Typically paravirtual devices go into this
+#         category.
+#         Example (nic): virtio-net.
+#         Example (usb); xhci.
+#
+# @yellow: Reasonable choice.  Typically emulated devices with
+#          good performance go into this category.
+#          Example (nic): e1000, e1000e
+#
+# @red: Bad choice.  Avoid this unless you run an old guest which
+#       lacks support for something better.

qapi/common.json line 214

Quoted 10 lines
qapi/common.json +10 -0
+#       Example (nic): rtl8139, pcnet, ne2k, ...
+#       Example (usb): ehci, uhci, ohci
+#
+# Since: 3.2
+##
+{ 'enum': 'UsageHints',
+  'data': [ 'unspecified',
+            'green',
+            'yellow',
+            'red' ] }

Simple. I like simple, as long as it's good enough. Hard to tell without actual users.

#17 lore raw
In reply to Gerd Hoffmann

This series is a step from treating deprecation completely ad hoc to a more structured approach. I like it. I guess we'll have to extend it. For instance, we deprecate not only objects, but also members and even values of members. Members should be straightforward, values probably not. But that's not an argument against this series.