Re: [PATCH v3 05/10] qapi: make the vcpu parameters deprecated for 8.1

[Markus Armbruster <armbru@redhat.com>]

Alex Bennée <alex.bennee@linaro.org> writes:

> I don't think I can remove the parameters directly but certainly mark
> them as deprecated.
>
> Message-Id: <20230420150009.1675181-6-alex.bennee@linaro.org>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Message-Id: <20230503091756.1453057-6-alex.bennee@linaro.org>
> ---
>  qapi/trace.json | 22 +++++++---------------
>  1 file changed, 7 insertions(+), 15 deletions(-)
>
> diff --git a/qapi/trace.json b/qapi/trace.json
> index f425d10764..de6b1681aa 100644
> --- a/qapi/trace.json
> +++ b/qapi/trace.json
> @@ -33,9 +33,9 @@
>  #
>  # @name: Event name.
>  # @state: Tracing state.
> -# @vcpu: Whether this is a per-vCPU event (since 2.7).
> +# @vcpu: Whether this is a per-vCPU event (deprecated since 8.1).

We don't normally replace the (since ...) when we deprecate.

>  #
> -# An event is per-vCPU if it has the "vcpu" property in the "trace-events"
> +# There are no longer any events with the "vcpu" property in the "trace-events"

Why would a user still need to know what @vcpu used to mean?  Also, long
line.  See below for a possible alternative.

>  # files.
>  #
>  # Since: 2.2

You need to make it official, like so:

   { 'struct': 'TraceEventInfo',
  -  'data': {'name': 'str', 'state': 'TraceEventState', 'vcpu': 'bool'} }
  +  'data': {'name': 'str', 'state': 'TraceEventState',
  +           'vcpu': { 'type': 'bool', 'features': ['deprecated'] } } }

And then the generator will demand you document it formally, so you also
need something like

 # @state: Tracing state.
 # @vcpu: Whether this is a per-vCPU event (since 2.7).
 #
-# An event is per-vCPU if it has the "vcpu" property in the "trace-events"
-# files.
+# Features:
+# @deprecated: Member @vcpu is deprecated, and always false.
 #
 # Since: 2.2
 ##

Additionally, update docs/about/deprecated.rst.

> @@ -49,19 +49,15 @@
>  # Query the state of events.
>  #
>  # @name: Event name pattern (case-sensitive glob).
> -# @vcpu: The vCPU to query (any by default; since 2.7).
> +# @vcpu: The vCPU to query (deprecated since 8.1).

Again, we don't normally replace the (since ...) when we deprecate.

I suggest to just drop the "any by default" part.

>  #
>  # Returns: a list of @TraceEventInfo for the matching events
>  #
>  #          An event is returned if:
>  #
>  #          - its name matches the @name pattern, and
> -#          - if @vcpu is given, the event has the "vcpu" property.
>  #
> -#          Therefore, if @vcpu is given, the operation will only match per-vCPU events,
> -#          returning their state on the specified vCPU. Special case: if @name is an
> -#          exact match, @vcpu is given and the event does not have the "vcpu" property,
> -#          an error is returned.
> +#          There are no longer any per-vCPU events
>  #
>  # Since: 2.2
>  #

Please add 'features': ['deprecated'].

> @@ -84,17 +80,13 @@
>  # @name: Event name pattern (case-sensitive glob).
>  # @enable: Whether to enable tracing.
>  # @ignore-unavailable: Do not match unavailable events with @name.
> -# @vcpu: The vCPU to act upon (all by default; since 2.7).
> +# @vcpu: The vCPU to act upon (deprecated since 8.1).

Suggest to just drop the "all by default" part.

>  #
>  # An event's state is modified if:
>  #
> -# - its name matches the @name pattern, and
> -# - if @vcpu is given, the event has the "vcpu" property.
> +# - its name matches the @name pattern
>  #
> -# Therefore, if @vcpu is given, the operation will only match per-vCPU events,
> -# setting their state on the specified vCPU. Special case: if @name is an exact
> -# match, @vcpu is given and the event does not have the "vcpu" property, an
> -# error is returned.
> +# There are no longer and per-vCPU events so specifying it will never match.
>  #
>  # Since: 2.2
>  #

Please add 'features': ['deprecated'].