Re: [PATCH v5 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.
>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Message-Id: <20230523125000.3674739-6-alex.bennee@linaro.org>
>
> ---
> v5
>   - reword match description
>   - fix reference to return for set operation
> ---
>  docs/about/deprecated.rst |  9 +++++++++
>  qapi/trace.json           | 40 +++++++++++++++++----------------------
>  2 files changed, 26 insertions(+), 23 deletions(-)
>
> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> index e934e0a13a..e44cde057f 100644
> --- a/docs/about/deprecated.rst
> +++ b/docs/about/deprecated.rst
> @@ -254,6 +254,15 @@ it. Since all recent x86 hardware from the past >10 years is capable of the
>  QEMU API (QAPI) events
>  ----------------------

Not this patch's fault: the headline should be "QEMU Machine Protocol
(QMP) events".  The section should directly follow section "QEMU Machine
Protocol (QMP) commands".

I'd go one step farther, and fuse the two sections under the heading
"QEMU Machine Protocol (QMP)".

>  
> +``vcpu`` trace events (since 8.1)
> +'''''''''''''''''''''''''''''''''
> +
> +The ability to instrument QEMU helper functions with vcpu aware trace

Should this be "vCPU-aware"?

> +points was removed in 7.0. However the QAPI still exposed the vcpu

s/the QAPI/QMP/

> +parameter. This argument has now been deprecated and the remaining
> +used trace points converted to plain trace points selected just by

"remaining trace points that used it"?

> +name.
> +
>  ``MEM_UNPLUG_ERROR`` (since 6.2)
>  ''''''''''''''''''''''''''''''''''''''''''''''''''''''''
>  
> diff --git a/qapi/trace.json b/qapi/trace.json
> index 6bf0af0946..e561f3d3da 100644
> --- a/qapi/trace.json
> +++ b/qapi/trace.json
> @@ -37,13 +37,14 @@
>  #
>  # @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
>  ##
>  { 'struct': 'TraceEventInfo',
> -  'data': {'name': 'str', 'state': 'TraceEventState', 'vcpu': 'bool'} }
> +  'data': {'name': 'str', 'state': 'TraceEventState',
> +           'vcpu': { 'type': 'bool', 'features': ['deprecated'] } } }
>  
>  ##
>  # @trace-event-get-state:
> @@ -52,19 +53,15 @@
>  #
>  # @name: Event name pattern (case-sensitive glob).
>  #
> -# @vcpu: The vCPU to query (any by default; since 2.7).
> +# @vcpu: The vCPU to query (since 2.7).
>  #
> -# Returns: a list of @TraceEventInfo for the matching events
> -#
> -# An event is returned if:
> +# Features:
> +# @deprecated: Member @vcpu is deprecated, and always false.

This isn't quite right: parameter @vcpu cannot be false, it's int.

I figure specifying the parameter makes no sense anymore, because if you
do, the command will return an empty list.  Correct?

>  #
> -# - its name matches the @name pattern, and
> -# - if @vcpu is given, the event has the "vcpu" property.
> +# Returns: a list of @TraceEventInfo for the matching events
>  #
> -# 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.
> +# An event is returned if its name matches the @name pattern
> +# (There are no longer any per-vCPU events).
>  #
>  # Since: 2.2
>  #
> @@ -75,7 +72,8 @@
>  # <- { "return": [ { "name": "qemu_memalign", "state": "disabled", "vcpu": false } ] }
>  ##
>  { 'command': 'trace-event-get-state',
> -  'data': {'name': 'str', '*vcpu': 'int'},
> +  'data': {'name': 'str',
> +           '*vcpu': {'type': 'int', 'features': ['deprecated'] } },
>    'returns': ['TraceEventInfo'] }
>  
>  ##
> @@ -91,15 +89,11 @@
>  #
>  # @vcpu: The vCPU to act upon (all by default; since 2.7).
>  #
> -# An event's state is modified if:
> -#
> -# - its name matches the @name pattern, and
> -# - if @vcpu is given, the event has the "vcpu" property.
> +# Features:
> +# @deprecated: Member @vcpu is deprecated, and always false.

Again, parameter @vcpu cannot be false.

What happens when you use it?

>  #
> -# 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.
> +# An event is enabled if its name matches the @name pattern
> +# (There are no longer any per-vCPU events).
>  #
>  # Since: 2.2
>  #
> @@ -111,4 +105,4 @@
>  ##
>  { 'command': 'trace-event-set-state',
>    'data': {'name': 'str', 'enable': 'bool', '*ignore-unavailable': 'bool',
> -           '*vcpu': 'int'} }
> +           '*vcpu': {'type': 'int', 'features': ['deprecated'] } } }