Re: [PATCH v7 43/65] qom: replace object_property_add_tm with StructTm QAPI type

[Markus Armbruster <armbru@redhat.com>]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Define a StructTm QAPI struct in common.json matching the existing
> "struct tm" property wire format (tm_year, tm_mon, tm_mday, tm_hour,
> tm_min, tm_sec as int32).
>
> Convert the two callers (mc146818rtc and spapr_rtc) to use
> object_property_add_qapi() with a standard ObjectPropertyAccessor
> that populates a StructTm and calls the generated visitor.
>
> Remove object_property_add_tm(), object_class_property_add_tm(),
> the TMProperty type, and the property_get_tm() helper from
> qom/object.c, along with their declarations in object.h.
>
> Reviewed-by: Amit Machhiwal <amachhiw@linux.ibm.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  hw/ppc/spapr_rtc.c   | 24 +++++++++++++++---
>  hw/rtc/mc146818rtc.c | 22 ++++++++++++++---
>  include/qom/object.h | 29 ----------------------
>  qapi/common.json     | 28 +++++++++++++++++++++
>  qapi/pragma.json     |  1 +
>  qom/object.c         | 69 ----------------------------------------------------
>  6 files changed, 69 insertions(+), 104 deletions(-)
>
> diff --git a/hw/ppc/spapr_rtc.c b/hw/ppc/spapr_rtc.c
> index 1f7d2d8f898b..5105cf538f64 100644
> --- a/hw/ppc/spapr_rtc.c
> +++ b/hw/ppc/spapr_rtc.c
> @@ -33,6 +33,9 @@
>  #include "migration/vmstate.h"
>  #include "qapi/error.h"
>  #include "qapi/qapi-events-misc.h"
> +#include "qapi/qapi-type-infos-common.h"
> +#include "qapi/qapi-visit-common.h"
> +#include "qapi/visitor.h"
>  #include "qemu/cutils.h"
>  #include "qemu/module.h"
>  
> @@ -131,9 +134,23 @@ static void rtas_set_time_of_day(PowerPCCPU *cpu, SpaprMachineState *spapr,
>      rtas_st(rets, 0, RTAS_OUT_SUCCESS);
>  }
>  
> -static void spapr_rtc_qom_date(Object *obj, struct tm *current_tm, Error **errp)
> +static void spapr_rtc_qom_date(Object *obj, Visitor *v, const char *name,
> +                               void *opaque, Error **errp)
>  {
> -    spapr_rtc_read(SPAPR_RTC(obj), current_tm, NULL);
> +    struct tm value;
> +    StructTm tm, *tmp = &tm;
> +
> +    spapr_rtc_read(SPAPR_RTC(obj), &value, NULL);
> +
> +    tm = (StructTm) {
> +        .tm_year = value.tm_year,
> +        .tm_mon = value.tm_mon,
> +        .tm_mday = value.tm_mday,
> +        .tm_hour = value.tm_hour,
> +        .tm_min = value.tm_min,
> +        .tm_sec = value.tm_sec,
> +    };
> +    visit_type_StructTm(v, name, &tmp, errp);
>  }
>  
>  static void spapr_rtc_realize(DeviceState *dev, Error **errp)
> @@ -150,7 +167,8 @@ static void spapr_rtc_realize(DeviceState *dev, Error **errp)
>      rtc_ns = qemu_clock_get_ns(rtc_clock);
>      rtc->ns_offset = host_s * NANOSECONDS_PER_SECOND - rtc_ns;
>  
> -    object_property_add_tm(OBJECT(rtc), "date", spapr_rtc_qom_date);
> +    object_property_add_qapi(OBJECT(rtc), "date", &StructTm_type_info,
> +                             spapr_rtc_qom_date, NULL, NULL, NULL);

The @type reported by qom-list & friends changes from "struct tm"
(visible at the end of this patch) to to "StuctTm".  Such changes can be
compatibility breaks.

Same "this is okay" argument as for the previous patch, and same
request: please note the change in the commit message.

>  }
>  
>  static const VMStateDescription vmstate_spapr_rtc = {
> diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
> index ba396435d1af..6010f8587ce0 100644
> --- a/hw/rtc/mc146818rtc.c
> +++ b/hw/rtc/mc146818rtc.c
> @@ -42,6 +42,8 @@
>  #include "migration/vmstate.h"
>  #include "qapi/error.h"
>  #include "qapi/qapi-events-misc.h"
> +#include "qapi/qapi-type-infos-common.h"
> +#include "qapi/qapi-visit-common.h"
>  #include "qapi/visitor.h"
>  #include "trace.h"
>  
> @@ -854,12 +856,25 @@ static const MemoryRegionOps cmos_ops = {
>      .endianness = DEVICE_LITTLE_ENDIAN,
>  };
>  
> -static void rtc_get_date(Object *obj, struct tm *current_tm, Error **errp)
> +static void rtc_get_date(Object *obj, Visitor *v, const char *name,
> +                         void *opaque, Error **errp)
>  {
>      MC146818RtcState *s = MC146818_RTC(obj);
> +    struct tm value;
> +    StructTm tm, *tmp = &tm;
>  
>      rtc_update_time(s);
> -    rtc_get_time(s, current_tm);
> +    rtc_get_time(s, &value);
> +
> +    tm = (StructTm) {
> +        .tm_year = value.tm_year,
> +        .tm_mon = value.tm_mon,
> +        .tm_mday = value.tm_mday,
> +        .tm_hour = value.tm_hour,
> +        .tm_min = value.tm_min,
> +        .tm_sec = value.tm_sec,
> +    };
> +    visit_type_StructTm(v, name, &tmp, errp);
>  }
>  
>  static void rtc_realizefn(DeviceState *dev, Error **errp)
> @@ -1019,7 +1034,8 @@ static void rtc_class_initfn(ObjectClass *klass, const void *data)
>      device_class_set_props(dc, mc146818rtc_properties);
>      set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>  
> -    object_class_property_add_tm(klass, "date", rtc_get_date);
> +    object_class_property_add_qapi(klass, "date", &StructTm_type_info,
> +                                   rtc_get_date, NULL, NULL, NULL);
>  }
>  
>  static const TypeInfo mc146818rtc_info = {
> diff --git a/include/qom/object.h b/include/qom/object.h
> index e04be4caed57..29cbd84b8d03 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -2156,35 +2156,6 @@ object_class_property_add_qapi(ObjectClass *klass,
>                                 ObjectPropertyRelease *release,
>                                 void *opaque);
>  
> -/**
> - * object_property_add_tm:
> - * @obj: the object to add a property to
> - * @name: the name of the property
> - * @get: the getter or NULL if the property is write-only.
> - *
> - * Add a read-only struct tm valued property using a getter function.
> - * This function will add a property of type 'struct tm'.
> - *
> - * Returns: The newly added property on success, or %NULL on failure.
> - */
> -ObjectProperty *object_property_add_tm(Object *obj, const char *name,
> -                            void (*get)(Object *, struct tm *, Error **));
> -
> -/**
> - * object_class_property_add_tm:
> - * @klass: the object class to add a property to
> - * @name: the name of the property
> - * @get: the getter or NULL if the property is write-only.
> - *
> - * Add a read-only struct tm valued property using a getter function.
> - * This function will add a property of type 'struct tm'.
> - *
> - * Returns: The newly added property on success, or %NULL on failure.
> - */
> -ObjectProperty *object_class_property_add_tm(ObjectClass *klass,
> -                            const char *name,
> -                            void (*get)(Object *, struct tm *, Error **));
> -
>  typedef enum {
>      /* Automatically add a getter to the property */
>      OBJ_PROP_FLAG_READ = 1 << 0,
> diff --git a/qapi/common.json b/qapi/common.json
> index 1352e664b23e..222037ea0b32 100644
> --- a/qapi/common.json
> +++ b/qapi/common.json
> @@ -218,3 +218,31 @@
>  ##
>  { 'enum': 'EndianMode',
>    'data': [ 'unspecified', 'little', 'big' ] }
> +
> +##
> +# @StructTm:
> +#
> +# Broken-down time.  Field semantics match C ``struct tm``.

Again, please use the revised syntax for introductions.

> +#
> +# @tm_year: years since 1900
> +#
> +# @tm_mon: months since January (0-11)
> +#
> +# @tm_mday: day of the month (1-31)
> +#
> +# @tm_hour: hours since midnight (0-23)
> +#
> +# @tm_min: minutes after the hour (0-59)
> +#
> +# @tm_sec: seconds after the minute (0-59, 60-61 for leap seconds)
> +#
> +# Since: 11.2

Same "do we care?" as for the previous patch.

> +##
> +{ 'struct': 'StructTm',
> +  'data': {
> +    'tm_year': 'int32',
> +    'tm_mon': 'int32',
> +    'tm_mday': 'int32',
> +    'tm_hour': 'int32',
> +    'tm_min': 'int32',
> +    'tm_sec': 'int32' } }
> diff --git a/qapi/pragma.json b/qapi/pragma.json
> index 342cedc42e76..9be2819a46bd 100644
> --- a/qapi/pragma.json
> +++ b/qapi/pragma.json
> @@ -109,6 +109,7 @@
       # Externally visible types whose member names may use uppercase

Not this patch's fault, but here goes anyway: the comment should be "may
use uppercase or '_'".  I'll fix it.

       'member-name-exceptions': [     # visible in:
           'ACPISlotType',             # query-acpi-ospm-status
           [...]
>          'QKeyCode',                 # send-key, input-sent-event
>          'QapiErrorClass',           # QMP error replies
>          'SshHostKeyCheckMode',      # blockdev-add, -blockdev
> +        'StructTm',                 # qom-get of RTC date property
>          'SysEmuTarget',             # query-cpu-fast, query-target
>          'UuidInfo',                 # query-uuid
>          'VncClientInfo',            # query-vnc, query-vnc-servers, ...

[...]