[RFC PATCH 0/4] qom: Introduce object_class_property_deprecate()

[RFC PATCH 0/4] qom: Introduce object_class_property_deprecate()

[Philippe Mathieu-Daudé]

Hi,

There will always be a need to deprecate things. Here I'm
tackling the QOM (class) properties, since they can be set
from some CLI options (-object -device -global ...).

As an experiment, we add object_class_property_deprecate()
to register a class property as deprecated (since some version),
then we deprecate the TYPE_PFLASH_CFI02 'width' property, and
finally as a bonus we emit a warning when the deprecation period
is over, as a reminder. (For that we introduce few 'versions'
helpers).

Output example:

$ qemu-system-arm -M musicpal,accel=qtest -S \
  -drive if=pflash,driver=null-co,read-zeroes=on,size=8M \
  -global driver=cfi.pflash02,property=width,value=2
qemu-system-arm: warning: Property 'cfi.pflash02.width' is deprecated (renamed as 'cfi.pflash02.device-width').

$ qemu-system-arm -M musicpal,accel=qtest -S \
  -drive if=pflash,driver=null-co,read-zeroes=on,size=8M \
  -global driver=cfi.pflash02,property=device-width,value=2
qemu-system-arm: warning: Property 'cfi.pflash02.width' has been deprecated in release v8.0 and can be removed.

Thought?

Regards,

Phil.

[earlier inspiration: https://lore.kernel.org/qemu-devel/Y7wlnqwU+/auE0Jj@redhat.com/]

Philippe Mathieu-Daudé (4):
  qom: Introduce object_class_property_deprecate()
  hw/block: Rename TYPE_PFLASH_CFI02 'width' property as 'device-width'
  util: Introduce helpers to compare QEMU versions
  qom: Warn when deprecated class property can be removed

 hw/block/pflash_cfi02.c     |  8 +++++++-
 include/qemu/qemu-version.h | 36 ++++++++++++++++++++++++++++++++++++
 include/qom/object.h        | 17 +++++++++++++++++
 qom/object.c                | 30 ++++++++++++++++++++++++++++++
 util/meson.build            |  1 +
 util/qemu-version.c         | 37 +++++++++++++++++++++++++++++++++++++
 6 files changed, 128 insertions(+), 1 deletion(-)
 create mode 100644 include/qemu/qemu-version.h
 create mode 100644 util/qemu-version.c

-- 
2.38.1

[RFC PATCH 1/4] qom: Introduce object_class_property_deprecate()

[Philippe Mathieu-Daudé]

Introduce object_class_property_deprecate() to register
a QOM property as deprecated. When this property's getter /
setter is called, a deprecation warning is displayed on the
monitor.

Inspired-by: Daniel P. Berrange <berrange@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/qom/object.h | 17 +++++++++++++++++
 qom/object.c         | 23 +++++++++++++++++++++++
 2 files changed, 40 insertions(+)

diff --git a/include/qom/object.h b/include/qom/object.h
index ef7258a5e1..b76724292c 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -97,6 +97,7 @@ struct ObjectProperty
     ObjectPropertyInit *init;
     void *opaque;
     QObject *defval;
+    const char *deprecation_reason;
 };
 
 /**
@@ -1075,6 +1076,22 @@ ObjectProperty *object_class_property_add(ObjectClass *klass, const char *name,
                                           ObjectPropertyRelease *release,
                                           void *opaque);
 
+/**
+ * object_class_property_deprecate:
+ * @klass: the class to add a property to
+ * @name: the name of the property.  This can contain any character except for
+ *  a forward slash.  In general, you should use hyphens '-' instead of
+ *  underscores '_' when naming properties.
+ * @reason: the deprecation reason.
+ * @version_major: the major version since this property is deprecated.
+ * @version_minor: the minor version since this property is deprecated.
+ *
+ * Deprecate a class property.
+ */
+void object_class_property_deprecate(ObjectClass *klass,
+                                     const char *name, const char *reason,
+                                     int version_major, int version_minor);
+
 /**
  * object_property_set_default_bool:
  * @prop: the property to set
diff --git a/qom/object.c b/qom/object.c
index e25f1e96db..05b97cd424 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1293,6 +1293,16 @@ object_class_property_add(ObjectClass *klass,
     return prop;
 }
 
+void object_class_property_deprecate(ObjectClass *klass,
+                                     const char *name, const char *reason,
+                                     int version_major, int version_minor)
+{
+    ObjectProperty *prop = object_class_property_find(klass, name);
+
+    assert(prop);
+    prop->deprecation_reason = reason;
+}
+
 ObjectProperty *object_property_find(Object *obj, const char *name)
 {
     ObjectProperty *prop;
@@ -1382,6 +1392,17 @@ void object_property_del(Object *obj, const char *name)
     g_hash_table_remove(obj->properties, name);
 }
 
+static void object_property_check_deprecation(const Object *obj,
+                                              const char *name,
+                                              const ObjectProperty *prop)
+{
+    if (!prop->deprecation_reason) {
+        return;
+    }
+    warn_report("Property '%s.%s' is deprecated (%s).",
+                object_get_typename(obj), name, prop->deprecation_reason);
+}
+
 bool object_property_get(Object *obj, const char *name, Visitor *v,
                          Error **errp)
 {
@@ -1392,6 +1413,7 @@ bool object_property_get(Object *obj, const char *name, Visitor *v,
         return false;
     }
 
+    object_property_check_deprecation(obj, name, prop);
     if (!prop->get) {
         error_setg(errp, "Property '%s.%s' is not readable",
                    object_get_typename(obj), name);
@@ -1412,6 +1434,7 @@ bool object_property_set(Object *obj, const char *name, Visitor *v,
         return false;
     }
 
+    object_property_check_deprecation(obj, name, prop);
     if (!prop->set) {
         error_setg(errp, "Property '%s.%s' is not writable",
                    object_get_typename(obj), name);
-- 
2.38.1

[RFC PATCH 2/4] hw/block: Rename TYPE_PFLASH_CFI02 'width' property as 'device-width'

[Philippe Mathieu-Daudé]

Use the same property name than the TYPE_PFLASH_CFI01 model.

Deprecate the current 'width' property and add the 'device-width'
property pointing to the same field in PFlashCFI02.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/block/pflash_cfi02.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index 2a99b286b0..bbf78ad1e4 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -950,6 +950,7 @@ static Property pflash_cfi02_properties[] = {
     DEFINE_PROP_UINT32("num-blocks3", PFlashCFI02, nb_blocs[3], 0),
     DEFINE_PROP_UINT32("sector-length3", PFlashCFI02, sector_len[3], 0),
     DEFINE_PROP_UINT8("width", PFlashCFI02, width, 0),
+    DEFINE_PROP_UINT8("device-width", PFlashCFI02, width, 0),
     DEFINE_PROP_UINT8("mappings", PFlashCFI02, mappings, 0),
     DEFINE_PROP_UINT8("big-endian", PFlashCFI02, be, 0),
     DEFINE_PROP_UINT16("id0", PFlashCFI02, ident0, 0),
@@ -978,6 +979,11 @@ static void pflash_cfi02_class_init(ObjectClass *klass, void *data)
     dc->unrealize = pflash_cfi02_unrealize;
     device_class_set_props(dc, pflash_cfi02_properties);
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
+
+    object_class_property_deprecate(klass, "width",
+                                    "renamed as '"
+                                    TYPE_PFLASH_CFI02 ".device-width'",
+                                    8, 0);
 }
 
 static const TypeInfo pflash_cfi02_info = {
@@ -1014,7 +1020,7 @@ PFlashCFI02 *pflash_cfi02_register(hwaddr base,
     assert(QEMU_IS_ALIGNED(size, sector_len));
     qdev_prop_set_uint32(dev, "num-blocks", size / sector_len);
     qdev_prop_set_uint32(dev, "sector-length", sector_len);
-    qdev_prop_set_uint8(dev, "width", width);
+    qdev_prop_set_uint8(dev, "device-width", width);
     qdev_prop_set_uint8(dev, "mappings", nb_mappings);
     qdev_prop_set_uint8(dev, "big-endian", !!be);
     qdev_prop_set_uint16(dev, "id0", id0);
-- 
2.38.1

[RFC PATCH 3/4] util: Introduce helpers to compare QEMU versions

[Philippe Mathieu-Daudé]

Add qemu_version_delta() to compare 2 QEMU versions,
and qemu_version_delta_current() to compare with the
current QEMU version.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/qemu/qemu-version.h | 36 ++++++++++++++++++++++++++++++++++++
 util/meson.build            |  1 +
 util/qemu-version.c         | 37 +++++++++++++++++++++++++++++++++++++
 3 files changed, 74 insertions(+)
 create mode 100644 include/qemu/qemu-version.h
 create mode 100644 util/qemu-version.c

diff --git a/include/qemu/qemu-version.h b/include/qemu/qemu-version.h
new file mode 100644
index 0000000000..c9274bfaf0
--- /dev/null
+++ b/include/qemu/qemu-version.h
@@ -0,0 +1,36 @@
+/*
+ * Utility function around QEMU release version
+ *
+ * Copyright (c) 2023 Linaro Ltd
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#ifndef QEMU_UTIL_VERSION_H
+#define QEMU_UTIL_VERSION_H
+
+/**
+ * qemu_version_delta - Return delta between two release versions ('A' and 'B').
+ * @version_major_a: Version 'A' major number
+ * @version_minor_a: Version 'A' minor number
+ * @version_major_b: Version 'B' major number
+ * @version_minor_b: Version 'B' minor number
+ *
+ * Returns a negative number is returned if 'A' is older than 'B', or positive
+ * if 'A' is newer than 'B'. The number represents the number of minor versions.
+ */
+int qemu_version_delta(unsigned version_major_a, unsigned version_minor_a,
+                       unsigned version_major_b, unsigned version_minor_b);
+
+/**
+ * qemu_version_delta_current - Return delta with current QEMU release version.
+ * @version_major: The major version
+ * @version_minor: The minor version
+ *
+ * Returns the number of minor versions between the current released
+ * version and the requested $major.$minor. A negative number is returned
+ * for older versions and positive for newer.
+ */
+int qemu_version_delta_current(unsigned version_major, unsigned version_minor);
+
+#endif
diff --git a/util/meson.build b/util/meson.build
index d8d109ff84..655debeec1 100644
--- a/util/meson.build
+++ b/util/meson.build
@@ -58,6 +58,7 @@ util_ss.add(files('yank.c'))
 util_ss.add(files('int128.c'))
 util_ss.add(files('memalign.c'))
 util_ss.add(files('interval-tree.c'))
+util_ss.add(files('qemu-version.c'))
 
 if have_user
   util_ss.add(files('selfmap.c'))
diff --git a/util/qemu-version.c b/util/qemu-version.c
new file mode 100644
index 0000000000..d409a6e574
--- /dev/null
+++ b/util/qemu-version.c
@@ -0,0 +1,37 @@
+/*
+ * Utility function around QEMU release version
+ *
+ * Copyright (c) 2023 Linaro Ltd
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/qemu-version.h"
+#include "config-host.h"
+
+#define QEMU_FIRST_MAJOR_VERSION_SUPPORTED 4
+#define QEMU_MINOR_VERSIONS_PER_MAJOR 3
+
+int qemu_version_delta(unsigned version_major_a, unsigned version_minor_a,
+                       unsigned version_major_b, unsigned version_minor_b)
+{
+    int delta;
+
+    assert(version_major_a >= QEMU_FIRST_MAJOR_VERSION_SUPPORTED);
+    assert(version_major_b >= QEMU_FIRST_MAJOR_VERSION_SUPPORTED);
+    assert(version_minor_a < QEMU_MINOR_VERSIONS_PER_MAJOR);
+    assert(version_minor_b < QEMU_MINOR_VERSIONS_PER_MAJOR);
+
+    delta = version_major_b - version_major_a;
+    delta *= QEMU_MINOR_VERSIONS_PER_MAJOR;
+    delta += version_minor_b - version_minor_a;
+
+    return delta;
+}
+
+int qemu_version_delta_current(unsigned version_major, unsigned version_minor)
+{
+    return qemu_version_delta(QEMU_VERSION_MAJOR, QEMU_VERSION_MINOR,
+                              version_major, version_minor);
+}
-- 
2.38.1

[RFC PATCH 4/4] qom: Warn when deprecated class property can be removed

[Philippe Mathieu-Daudé]

Per docs/system/deprecated.rst, a deprecated feature can be
removed after 2 releases. Since we commit when a class property
is deprecated, we can warn when the deprecation period is over.

See also commit ef1f5b0a96 ("docs: clarify deprecation schedule").

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 qom/object.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/qom/object.c b/qom/object.c
index 05b97cd424..cb829f1e44 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -17,6 +17,7 @@
 #include "qom/object_interfaces.h"
 #include "qemu/cutils.h"
 #include "qemu/memalign.h"
+#include "qemu/qemu-version.h"
 #include "qapi/visitor.h"
 #include "qapi/string-input-visitor.h"
 #include "qapi/string-output-visitor.h"
@@ -1300,6 +1301,12 @@ void object_class_property_deprecate(ObjectClass *klass,
     ObjectProperty *prop = object_class_property_find(klass, name);
 
     assert(prop);
+    if (qemu_version_delta_current(version_major, version_minor) <= 2) {
+        warn_report_once("Property '%s.%s' has been deprecated in release"
+                         " v%u.%u and can be removed.",
+                         object_class_get_name(klass), name,
+                         version_major, version_minor);
+    }
     prop->deprecation_reason = reason;
 }
 
-- 
2.38.1

Re: [RFC PATCH 4/4] qom: Warn when deprecated class property can be removed

[Bernhard Beschow]

Am 9. Januar 2023 22:54:19 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>Per docs/system/deprecated.rst, a deprecated feature can be
>removed after 2 releases. Since we commit when a class property
>is deprecated, we can warn when the deprecation period is over.
>
>See also commit ef1f5b0a96 ("docs: clarify deprecation schedule").
>
>Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>---
> qom/object.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
>diff --git a/qom/object.c b/qom/object.c
>index 05b97cd424..cb829f1e44 100644
>--- a/qom/object.c
>+++ b/qom/object.c
>@@ -17,6 +17,7 @@
> #include "qom/object_interfaces.h"
> #include "qemu/cutils.h"
> #include "qemu/memalign.h"
>+#include "qemu/qemu-version.h"
> #include "qapi/visitor.h"
> #include "qapi/string-input-visitor.h"
> #include "qapi/string-output-visitor.h"
>@@ -1300,6 +1301,12 @@ void object_class_property_deprecate(ObjectClass *klass,
>     ObjectProperty *prop = object_class_property_find(klass, name);
> 
>     assert(prop);
>+    if (qemu_version_delta_current(version_major, version_minor) <= 2) {
>+        warn_report_once("Property '%s.%s' has been deprecated in release"
>+                         " v%u.%u and can be removed.",
>+                         object_class_get_name(klass), name,
>+                         version_major, version_minor);
>+    }
>     prop->deprecation_reason = reason;
> }
> 

Great series!

Perhaps turn object_class_property_deprecate() into a macro and warn at compile time already? That way we won't miss removing any properties and users won't see a warning they can't do anything about.

Best regards,
Bernhard

Re: [RFC PATCH 4/4] qom: Warn when deprecated class property can be removed

[Daniel P. Berrangé]

On Mon, Jan 09, 2023 at 11:54:19PM +0100, Philippe Mathieu-Daudé wrote:
> Per docs/system/deprecated.rst, a deprecated feature can be
> removed after 2 releases. Since we commit when a class property
> is deprecated, we can warn when the deprecation period is over.
> 
> See also commit ef1f5b0a96 ("docs: clarify deprecation schedule").
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  qom/object.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/qom/object.c b/qom/object.c
> index 05b97cd424..cb829f1e44 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -17,6 +17,7 @@
>  #include "qom/object_interfaces.h"
>  #include "qemu/cutils.h"
>  #include "qemu/memalign.h"
> +#include "qemu/qemu-version.h"
>  #include "qapi/visitor.h"
>  #include "qapi/string-input-visitor.h"
>  #include "qapi/string-output-visitor.h"
> @@ -1300,6 +1301,12 @@ void object_class_property_deprecate(ObjectClass *klass,
>      ObjectProperty *prop = object_class_property_find(klass, name);
>  
>      assert(prop);
> +    if (qemu_version_delta_current(version_major, version_minor) <= 2) {
> +        warn_report_once("Property '%s.%s' has been deprecated in release"
> +                         " v%u.%u and can be removed.",
> +                         object_class_get_name(klass), name,
> +                         version_major, version_minor);
> +    }

IMHO this is not really appropriate. Removal of deprecated features
is a job for maintainers, but this is printing a message at users.

Maintainers have a record of deprecations in the docs that they can
periodically review to identify stuff that can now be deleted.


With 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: [RFC PATCH 2/4] hw/block: Rename TYPE_PFLASH_CFI02 'width' property as 'device-width'

[Daniel P. Berrangé]

On Mon, Jan 09, 2023 at 11:54:17PM +0100, Philippe Mathieu-Daudé wrote:
> Use the same property name than the TYPE_PFLASH_CFI01 model.
> 
> Deprecate the current 'width' property and add the 'device-width'
> property pointing to the same field in PFlashCFI02.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/block/pflash_cfi02.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)

docs/about/deprecated.rst must be updated for any deprecations
to be valid.

With 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: [RFC PATCH 1/4] qom: Introduce object_class_property_deprecate()

[Daniel P. Berrangé]

On Mon, Jan 09, 2023 at 11:54:16PM +0100, Philippe Mathieu-Daudé wrote:
> Introduce object_class_property_deprecate() to register
> a QOM property as deprecated. When this property's getter /
> setter is called, a deprecation warning is displayed on the
> monitor.
> 
> Inspired-by: Daniel P. Berrange <berrange@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  include/qom/object.h | 17 +++++++++++++++++
>  qom/object.c         | 23 +++++++++++++++++++++++
>  2 files changed, 40 insertions(+)
> 
> diff --git a/include/qom/object.h b/include/qom/object.h
> index ef7258a5e1..b76724292c 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -97,6 +97,7 @@ struct ObjectProperty
>      ObjectPropertyInit *init;
>      void *opaque;
>      QObject *defval;
> +    const char *deprecation_reason;
>  };
>  
>  /**
> @@ -1075,6 +1076,22 @@ ObjectProperty *object_class_property_add(ObjectClass *klass, const char *name,
>                                            ObjectPropertyRelease *release,
>                                            void *opaque);
>  
> +/**
> + * object_class_property_deprecate:
> + * @klass: the class to add a property to
> + * @name: the name of the property.  This can contain any character except for
> + *  a forward slash.  In general, you should use hyphens '-' instead of
> + *  underscores '_' when naming properties.
> + * @reason: the deprecation reason.
> + * @version_major: the major version since this property is deprecated.
> + * @version_minor: the minor version since this property is deprecated.
> + *
> + * Deprecate a class property.
> + */
> +void object_class_property_deprecate(ObjectClass *klass,
> +                                     const char *name, const char *reason,
> +                                     int version_major, int version_minor);
> +
>  /**
>   * object_property_set_default_bool:
>   * @prop: the property to set
> diff --git a/qom/object.c b/qom/object.c
> index e25f1e96db..05b97cd424 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -1293,6 +1293,16 @@ object_class_property_add(ObjectClass *klass,
>      return prop;
>  }
>  
> +void object_class_property_deprecate(ObjectClass *klass,
> +                                     const char *name, const char *reason,
> +                                     int version_major, int version_minor)
> +{
> +    ObjectProperty *prop = object_class_property_find(klass, name);
> +
> +    assert(prop);
> +    prop->deprecation_reason = reason;
> +}

Nothing is using the 'version_major' / 'version_minor' parameters so
they look redundant.

> @@ -1392,6 +1413,7 @@ bool object_property_get(Object *obj, const char *name, Visitor *v,
>          return false;
>      }
>  
> +    object_property_check_deprecation(obj, name, prop);
>      if (!prop->get) {
>          error_setg(errp, "Property '%s.%s' is not readable",
>                     object_get_typename(obj), name);
> @@ -1412,6 +1434,7 @@ bool object_property_set(Object *obj, const char *name, Visitor *v,
>          return false;
>      }
>  
> +    object_property_check_deprecation(obj, name, prop);
>      if (!prop->set) {
>          error_setg(errp, "Property '%s.%s' is not writable",
>                     object_get_typename(obj), name);
> -- 
> 2.38.1
> 

With 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: [RFC PATCH 1/4] qom: Introduce object_class_property_deprecate()

[Daniel P. Berrangé]

On Mon, Jan 09, 2023 at 11:54:16PM +0100, Philippe Mathieu-Daudé wrote:
> Introduce object_class_property_deprecate() to register
> a QOM property as deprecated. When this property's getter /
> setter is called, a deprecation warning is displayed on the
> monitor.
> 
> Inspired-by: Daniel P. Berrange <berrange@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  include/qom/object.h | 17 +++++++++++++++++
>  qom/object.c         | 23 +++++++++++++++++++++++
>  2 files changed, 40 insertions(+)
> 
> diff --git a/include/qom/object.h b/include/qom/object.h
> index ef7258a5e1..b76724292c 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -97,6 +97,7 @@ struct ObjectProperty
>      ObjectPropertyInit *init;
>      void *opaque;
>      QObject *defval;
> +    const char *deprecation_reason;
>  };

qapi/qom.list should change ObjectPropertyInfo type to
include a 'deprecated': 'bool'  field, as we've done
for machine types and CPU models. Then qmp_qom_list
should set this flag  "deprecated = !!deprecation_reason"


With 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: [RFC PATCH 0/4] qom: Introduce object_class_property_deprecate()

[Kevin Wolf]

Am 09.01.2023 um 23:54 hat Philippe Mathieu-Daudé geschrieben:
> Hi,
> 
> There will always be a need to deprecate things. Here I'm
> tackling the QOM (class) properties, since they can be set
> from some CLI options (-object -device -global ...).
> 
> As an experiment, we add object_class_property_deprecate()
> to register a class property as deprecated (since some version),
> then we deprecate the TYPE_PFLASH_CFI02 'width' property, and
> finally as a bonus we emit a warning when the deprecation period
> is over, as a reminder. (For that we introduce few 'versions'
> helpers).

The last part means that increasing the version number (i.e. the commit
that opens the development tree for the next release) can change the
output, and this is turn can break test cases.

If we are happy to introduce breakage with a version number change that
will require future commits to open the development tree less trivial
than they are today because they need to fix the breakage, too, why not
make it a build error instead of a different warning message at runtime?

Kevin

Re: [RFC PATCH 0/4] qom: Introduce object_class_property_deprecate()

[Philippe Mathieu-Daudé]

On 10/1/23 14:02, Kevin Wolf wrote:
> Am 09.01.2023 um 23:54 hat Philippe Mathieu-Daudé geschrieben:
>> Hi,
>>
>> There will always be a need to deprecate things. Here I'm
>> tackling the QOM (class) properties, since they can be set
>> from some CLI options (-object -device -global ...).
>>
>> As an experiment, we add object_class_property_deprecate()
>> to register a class property as deprecated (since some version),
>> then we deprecate the TYPE_PFLASH_CFI02 'width' property, and
>> finally as a bonus we emit a warning when the deprecation period
>> is over, as a reminder. (For that we introduce few 'versions'
>> helpers).
> 
> The last part means that increasing the version number (i.e. the commit
> that opens the development tree for the next release) can change the
> output, and this is turn can break test cases.
> 
> If we are happy to introduce breakage with a version number change that
> will require future commits to open the development tree less trivial
> than they are today because they need to fix the breakage, too, why not
> make it a build error instead of a different warning message at runtime?

To avoid build breakages, maybe it is clever is to store the deprecation
version in ObjectPropertyInfo and let QAPI inspection scripts enumerate
/ report deprecated features?

Re: [RFC PATCH 0/4] qom: Introduce object_class_property_deprecate()

[Daniel P. Berrangé]

On Wed, Jan 11, 2023 at 10:55:47AM +0100, Philippe Mathieu-Daudé wrote:
> On 10/1/23 14:02, Kevin Wolf wrote:
> > Am 09.01.2023 um 23:54 hat Philippe Mathieu-Daudé geschrieben:
> > > Hi,
> > > 
> > > There will always be a need to deprecate things. Here I'm
> > > tackling the QOM (class) properties, since they can be set
> > > from some CLI options (-object -device -global ...).
> > > 
> > > As an experiment, we add object_class_property_deprecate()
> > > to register a class property as deprecated (since some version),
> > > then we deprecate the TYPE_PFLASH_CFI02 'width' property, and
> > > finally as a bonus we emit a warning when the deprecation period
> > > is over, as a reminder. (For that we introduce few 'versions'
> > > helpers).
> > 
> > The last part means that increasing the version number (i.e. the commit
> > that opens the development tree for the next release) can change the
> > output, and this is turn can break test cases.
> > 
> > If we are happy to introduce breakage with a version number change that
> > will require future commits to open the development tree less trivial
> > than they are today because they need to fix the breakage, too, why not
> > make it a build error instead of a different warning message at runtime?
> 
> To avoid build breakages, maybe it is clever is to store the deprecation
> version in ObjectPropertyInfo and let QAPI inspection scripts enumerate
> / report deprecated features?

I don't think we want the version information in the code nor
introspectable at all.

We want applications to only apply logic based off features that are
actually available, not predicted future versions where something may
or may not be removed. This is why we exposed only a plain 'deprecated'
boolean field in QAPI schema for other deprecations.  This is just a
warning to be ready for something to change in future. If an application
has not been updated they are fine to carry on using the deprecated
feature. If an application has been updated, they should probe for
existance of the new feature and use that if available, in preference
to the deprecated feature. There's no reason for an application to
consider version numbers.

With 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: [RFC PATCH 0/4] qom: Introduce object_class_property_deprecate()

[Philippe Mathieu-Daudé]

On 11/1/23 10:59, Daniel P. Berrangé wrote:
> On Wed, Jan 11, 2023 at 10:55:47AM +0100, Philippe Mathieu-Daudé wrote:
>> On 10/1/23 14:02, Kevin Wolf wrote:
>>> Am 09.01.2023 um 23:54 hat Philippe Mathieu-Daudé geschrieben:
>>>> Hi,
>>>>
>>>> There will always be a need to deprecate things. Here I'm
>>>> tackling the QOM (class) properties, since they can be set
>>>> from some CLI options (-object -device -global ...).
>>>>
>>>> As an experiment, we add object_class_property_deprecate()
>>>> to register a class property as deprecated (since some version),
>>>> then we deprecate the TYPE_PFLASH_CFI02 'width' property, and
>>>> finally as a bonus we emit a warning when the deprecation period
>>>> is over, as a reminder. (For that we introduce few 'versions'
>>>> helpers).
>>>
>>> The last part means that increasing the version number (i.e. the commit
>>> that opens the development tree for the next release) can change the
>>> output, and this is turn can break test cases.
>>>
>>> If we are happy to introduce breakage with a version number change that
>>> will require future commits to open the development tree less trivial
>>> than they are today because they need to fix the breakage, too, why not
>>> make it a build error instead of a different warning message at runtime?
>>
>> To avoid build breakages, maybe it is clever is to store the deprecation
>> version in ObjectPropertyInfo and let QAPI inspection scripts enumerate
>> / report deprecated features?
> 
> I don't think we want the version information in the code nor
> introspectable at all.
> 
> We want applications to only apply logic based off features that are
> actually available, not predicted future versions where something may
> or may not be removed. This is why we exposed only a plain 'deprecated'
> boolean field in QAPI schema for other deprecations.  This is just a
> warning to be ready for something to change in future. If an application
> has not been updated they are fine to carry on using the deprecated
> feature. If an application has been updated, they should probe for
> existance of the new feature and use that if available, in preference
> to the deprecated feature. There's no reason for an application to
> consider version numbers.

Right, but "applications" can also be developer scripts right? Not
only user / sysadmin.

In particular, some HMP commands are only useful for developers, and
they are implemented over QMP -> QAPI. So we already expose extra
developer information via QAPI.

Re: [RFC PATCH 0/4] qom: Introduce object_class_property_deprecate()

[Daniel P. Berrangé]

On Wed, Jan 11, 2023 at 11:08:05AM +0100, Philippe Mathieu-Daudé wrote:
> On 11/1/23 10:59, Daniel P. Berrangé wrote:
> > On Wed, Jan 11, 2023 at 10:55:47AM +0100, Philippe Mathieu-Daudé wrote:
> > > On 10/1/23 14:02, Kevin Wolf wrote:
> > > > Am 09.01.2023 um 23:54 hat Philippe Mathieu-Daudé geschrieben:
> > > > > Hi,
> > > > > 
> > > > > There will always be a need to deprecate things. Here I'm
> > > > > tackling the QOM (class) properties, since they can be set
> > > > > from some CLI options (-object -device -global ...).
> > > > > 
> > > > > As an experiment, we add object_class_property_deprecate()
> > > > > to register a class property as deprecated (since some version),
> > > > > then we deprecate the TYPE_PFLASH_CFI02 'width' property, and
> > > > > finally as a bonus we emit a warning when the deprecation period
> > > > > is over, as a reminder. (For that we introduce few 'versions'
> > > > > helpers).
> > > > 
> > > > The last part means that increasing the version number (i.e. the commit
> > > > that opens the development tree for the next release) can change the
> > > > output, and this is turn can break test cases.
> > > > 
> > > > If we are happy to introduce breakage with a version number change that
> > > > will require future commits to open the development tree less trivial
> > > > than they are today because they need to fix the breakage, too, why not
> > > > make it a build error instead of a different warning message at runtime?
> > > 
> > > To avoid build breakages, maybe it is clever is to store the deprecation
> > > version in ObjectPropertyInfo and let QAPI inspection scripts enumerate
> > > / report deprecated features?
> > 
> > I don't think we want the version information in the code nor
> > introspectable at all.
> > 
> > We want applications to only apply logic based off features that are
> > actually available, not predicted future versions where something may
> > or may not be removed. This is why we exposed only a plain 'deprecated'
> > boolean field in QAPI schema for other deprecations.  This is just a
> > warning to be ready for something to change in future. If an application
> > has not been updated they are fine to carry on using the deprecated
> > feature. If an application has been updated, they should probe for
> > existance of the new feature and use that if available, in preference
> > to the deprecated feature. There's no reason for an application to
> > consider version numbers.
> 
> Right, but "applications" can also be developer scripts right? Not
> only user / sysadmin.
> 
> In particular, some HMP commands are only useful for developers, and
> they are implemented over QMP -> QAPI. So we already expose extra
> developer information via QAPI.

Sure, but I still don't think we should expose any version info there.
A deprecated feature isn't gone until it is gone. In the deprecations
doc we only mention the release where it is first deprecated, don't
explicitly state when it will be removed. The 2 cycle timeframe is
a minimum, not an exact removal date, so it would be misleading to
claim we'll remove things in exactly 2 cycles.

With 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: [RFC PATCH 0/4] qom: Introduce object_class_property_deprecate()

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Wed, Jan 11, 2023 at 11:08:05AM +0100, Philippe Mathieu-Daudé wrote:
>> On 11/1/23 10:59, Daniel P. Berrangé wrote:
>> > On Wed, Jan 11, 2023 at 10:55:47AM +0100, Philippe Mathieu-Daudé wrote:
>> > > On 10/1/23 14:02, Kevin Wolf wrote:
>> > > > Am 09.01.2023 um 23:54 hat Philippe Mathieu-Daudé geschrieben:
>> > > > > Hi,
>> > > > > 
>> > > > > There will always be a need to deprecate things. Here I'm
>> > > > > tackling the QOM (class) properties, since they can be set
>> > > > > from some CLI options (-object -device -global ...).
>> > > > > 
>> > > > > As an experiment, we add object_class_property_deprecate()
>> > > > > to register a class property as deprecated (since some version),
>> > > > > then we deprecate the TYPE_PFLASH_CFI02 'width' property, and
>> > > > > finally as a bonus we emit a warning when the deprecation period
>> > > > > is over, as a reminder. (For that we introduce few 'versions'
>> > > > > helpers).
>> > > > 
>> > > > The last part means that increasing the version number (i.e. the commit
>> > > > that opens the development tree for the next release) can change the
>> > > > output, and this is turn can break test cases.
>> > > > 
>> > > > If we are happy to introduce breakage with a version number change that
>> > > > will require future commits to open the development tree less trivial
>> > > > than they are today because they need to fix the breakage, too, why not
>> > > > make it a build error instead of a different warning message at runtime?
>> > > 
>> > > To avoid build breakages, maybe it is clever is to store the deprecation
>> > > version in ObjectPropertyInfo and let QAPI inspection scripts enumerate
>> > > / report deprecated features?
>> > 
>> > I don't think we want the version information in the code nor
>> > introspectable at all.
>> > 
>> > We want applications to only apply logic based off features that are
>> > actually available, not predicted future versions where something may
>> > or may not be removed. This is why we exposed only a plain 'deprecated'
>> > boolean field in QAPI schema for other deprecations.  This is just a
>> > warning to be ready for something to change in future. If an application
>> > has not been updated they are fine to carry on using the deprecated
>> > feature. If an application has been updated, they should probe for
>> > existance of the new feature and use that if available, in preference
>> > to the deprecated feature. There's no reason for an application to
>> > consider version numbers.
>> 
>> Right, but "applications" can also be developer scripts right? Not
>> only user / sysadmin.
>> 
>> In particular, some HMP commands are only useful for developers, and
>> they are implemented over QMP -> QAPI. So we already expose extra
>> developer information via QAPI.
>
> Sure, but I still don't think we should expose any version info there.
> A deprecated feature isn't gone until it is gone. In the deprecations
> doc we only mention the release where it is first deprecated, don't
> explicitly state when it will be removed. The 2 cycle timeframe is
> a minimum, not an exact removal date, so it would be misleading to
> claim we'll remove things in exactly 2 cycles.

I agree with Daniel.

I understand the motivation for making developers aware of expired grace
periods.

A warning is one way to make aware.  It creates another problem, though:
since the grace period is flexible, we need a way to extend the period,
and we need to decide right at the beginning of the development cycle.

I think the existing process for getting rid of deprecated stuff in a
timely manner is good enough: document all deprecations in
docs/about/deprecated.rst, and check the file periodically.

I'd recommend to follow QAPI's lead and add a "deprecated" flag to QOM.

We may want to follow QAPI some more and add an "unstable" flag, too.
See commit a3c45b3e62 'qapi: New special feature flag "unstable"' for
rationale.

Re: [RFC PATCH 0/4] qom: Introduce object_class_property_deprecate()

[Philippe Mathieu-Daudé]

On 11/1/23 12:29, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
>> On Wed, Jan 11, 2023 at 11:08:05AM +0100, Philippe Mathieu-Daudé wrote:
>>> On 11/1/23 10:59, Daniel P. Berrangé wrote:
>>>> On Wed, Jan 11, 2023 at 10:55:47AM +0100, Philippe Mathieu-Daudé wrote:
>>>>> On 10/1/23 14:02, Kevin Wolf wrote:
>>>>>> Am 09.01.2023 um 23:54 hat Philippe Mathieu-Daudé geschrieben:
>>>>>>> Hi,
>>>>>>>
>>>>>>> There will always be a need to deprecate things. Here I'm
>>>>>>> tackling the QOM (class) properties, since they can be set
>>>>>>> from some CLI options (-object -device -global ...).
>>>>>>>
>>>>>>> As an experiment, we add object_class_property_deprecate()
>>>>>>> to register a class property as deprecated (since some version),
>>>>>>> then we deprecate the TYPE_PFLASH_CFI02 'width' property, and
>>>>>>> finally as a bonus we emit a warning when the deprecation period
>>>>>>> is over, as a reminder. (For that we introduce few 'versions'
>>>>>>> helpers).
>>>>>>
>>>>>> The last part means that increasing the version number (i.e. the commit
>>>>>> that opens the development tree for the next release) can change the
>>>>>> output, and this is turn can break test cases.
>>>>>>
>>>>>> If we are happy to introduce breakage with a version number change that
>>>>>> will require future commits to open the development tree less trivial
>>>>>> than they are today because they need to fix the breakage, too, why not
>>>>>> make it a build error instead of a different warning message at runtime?
>>>>>
>>>>> To avoid build breakages, maybe it is clever is to store the deprecation
>>>>> version in ObjectPropertyInfo and let QAPI inspection scripts enumerate
>>>>> / report deprecated features?
>>>>
>>>> I don't think we want the version information in the code nor
>>>> introspectable at all.
>>>>
>>>> We want applications to only apply logic based off features that are
>>>> actually available, not predicted future versions where something may
>>>> or may not be removed. This is why we exposed only a plain 'deprecated'
>>>> boolean field in QAPI schema for other deprecations.  This is just a
>>>> warning to be ready for something to change in future. If an application
>>>> has not been updated they are fine to carry on using the deprecated
>>>> feature. If an application has been updated, they should probe for
>>>> existance of the new feature and use that if available, in preference
>>>> to the deprecated feature. There's no reason for an application to
>>>> consider version numbers.
>>>
>>> Right, but "applications" can also be developer scripts right? Not
>>> only user / sysadmin.
>>>
>>> In particular, some HMP commands are only useful for developers, and
>>> they are implemented over QMP -> QAPI. So we already expose extra
>>> developer information via QAPI.
>>
>> Sure, but I still don't think we should expose any version info there.
>> A deprecated feature isn't gone until it is gone. In the deprecations
>> doc we only mention the release where it is first deprecated, don't
>> explicitly state when it will be removed. The 2 cycle timeframe is
>> a minimum, not an exact removal date, so it would be misleading to
>> claim we'll remove things in exactly 2 cycles.
> 
> I agree with Daniel.
> 
> I understand the motivation for making developers aware of expired grace
> periods.
> 
> A warning is one way to make aware.  It creates another problem, though:
> since the grace period is flexible, we need a way to extend the period,
> and we need to decide right at the beginning of the development cycle.
> 
> I think the existing process for getting rid of deprecated stuff in a
> timely manner is good enough: document all deprecations in
> docs/about/deprecated.rst, and check the file periodically.
> 
> I'd recommend to follow QAPI's lead and add a "deprecated" flag to QOM.
> 
> We may want to follow QAPI some more and add an "unstable" flag, too.
> See commit a3c45b3e62 'qapi: New special feature flag "unstable"' for
> rationale.

I understand breaking the build system is not a welcomed idea and that
the deprecation grace period can be flexible.

I don't understand why storing version information isn't helpful, as
I'd like to run some ./qapi-dump-deprecated-stuff script at least once
between releases, and being able to display this info directly seems
simpler than going to match in deprecated.rst.

But overall the discussion was enlightening, so thanks!

Phil.