Re: [Qemu-devel] [PATCH v2 12/12] Drop qmp-commands.hx

[Markus Armbruster <armbru@redhat.com>]

marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Only the documentation remains useful, so strip it.

What about:

    The only remaining function of qmp-commands.hx is to let us generate
    qmp-commands.txt from it.  Replace qmp-commands.hx by qmp-commands.txt.

Assuming that's what you do.  Is it?

>                                                     (a later update will
> move the documentation in the respective json files and generate the
> text file)

Suggest "again generate".

> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  .gitignore                          |    1 -
>  MAINTAINERS                         |    2 +-
>  Makefile                            |    3 -
>  docs/qapi-code-gen.txt              |    6 +-
>  docs/writing-qmp-commands.txt       |   38 --
>  qmp-commands.hx => qmp-commands.txt | 1113 +----------------------------------
>  6 files changed, 7 insertions(+), 1156 deletions(-)
>  rename qmp-commands.hx => qmp-commands.txt (87%)
>
> diff --git a/.gitignore b/.gitignore
> index 88ec249..0ab8263 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -53,7 +53,6 @@
>  /qemu-bridge-helper
>  /qemu-monitor.texi
>  /qemu-monitor-info.texi
> -/qmp-commands.txt
>  /vscclient
>  /fsdev/virtfs-proxy-helper
>  *.[1-9]
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d1439a8..ff52548 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1238,7 +1238,7 @@ M: Markus Armbruster <armbru@redhat.com>
>  S: Supported
>  F: qmp.c
>  F: monitor.c
> -F: qmp-commands.hx
> +F: qmp-commands.txt
>  F: docs/*qmp-*
>  F: scripts/qmp/
>  T: git git://repo.or.cz/qemu/armbru.git qapi-next
> diff --git a/Makefile b/Makefile
> index fcdc192..8ec1c3e 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -554,9 +554,6 @@ qemu-monitor.texi: $(SRC_PATH)/hmp-commands.hx $(SRC_PATH)/scripts/hxtool
>  qemu-monitor-info.texi: $(SRC_PATH)/hmp-commands-info.hx $(SRC_PATH)/scripts/hxtool
>  	$(call quiet-command,sh $(SRC_PATH)/scripts/hxtool -t < $< > $@,"  GEN   $@")
>  
> -qmp-commands.txt: $(SRC_PATH)/qmp-commands.hx $(SRC_PATH)/scripts/hxtool
> -	$(call quiet-command,sh $(SRC_PATH)/scripts/hxtool -q < $< > $@,"  GEN   $@")
> -
>  qemu-img-cmds.texi: $(SRC_PATH)/qemu-img-cmds.hx $(SRC_PATH)/scripts/hxtool
>  	$(call quiet-command,sh $(SRC_PATH)/scripts/hxtool -t < $< > $@,"  GEN   $@")
>  
> diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
> index de298dc..5d4c2cd 100644
> --- a/docs/qapi-code-gen.txt
> +++ b/docs/qapi-code-gen.txt
> @@ -964,9 +964,9 @@ Example:
>  
>  Used to generate the marshaling/dispatch functions for the commands
>  defined in the schema. The generated code implements
> -qmp_marshal_COMMAND() (mentioned in qmp-commands.hx, and registered
> -automatically), and declares qmp_COMMAND() that the user must
> -implement.  The following files are generated:
> +qmp_marshal_COMMAND() (registered automatically), and declares
> +qmp_COMMAND() that the user must implement.  The following files are
> +generated:

I see you're thorough :)

>  
>  $(prefix)qmp-marshal.c: command marshal/dispatch functions for each
>                          QMP command defined in the schema. Functions
> diff --git a/docs/writing-qmp-commands.txt b/docs/writing-qmp-commands.txt
> index 6208715..cfa6fe7 100644
> --- a/docs/writing-qmp-commands.txt
> +++ b/docs/writing-qmp-commands.txt
> @@ -119,16 +119,6 @@ There are a few things to be noticed:
>  5. Printing to the terminal is discouraged for QMP commands, we do it here
>     because it's the easiest way to demonstrate a QMP command
>  
> -Now a little hack is needed. As we're still using the old QMP server we need
> -to add the new command to its internal dispatch table. This step won't be
> -required in the near future. Open the qmp-commands.hx file and add the

For a value of "near": almost five years!

> -following at the bottom:
> -
> -    {
> -        .name       = "hello-world",
> -        .args_type  = "",
> -    },
> -
>  You're done. Now build qemu, run it as suggested in the "Testing" section,
>  and then type the following QMP command:
>  
> @@ -173,20 +163,6 @@ There are two important details to be noticed:
>  2. The C implementation signature must follow the schema's argument ordering,
>     which is defined by the "data" member
>  
> -The last step is to update the qmp-commands.hx file:
> -
> -    {
> -        .name       = "hello-world",
> -        .args_type  = "message:s?",
> -    },
> -
> -Notice that the "args_type" member got our "message" argument. The character
> -"s" stands for "string" and "?" means it's optional. This too must be ordered
> -according to the C implementation and schema file. You can look for more
> -examples in the qmp-commands.hx file if you need to define more arguments.
> -
> -Again, this step won't be required in the future.
> -
>  Time to test our new version of the "hello-world" command. Build qemu, run it as
>  described in the "Testing" section and then send two commands:
>  
> @@ -452,13 +428,6 @@ There are a number of things to be noticed:
>  6. You have to include the "qmp-commands.h" header file in qemu-timer.c,
>     otherwise qemu won't build
>  
> -The last step is to add the correspoding entry in the qmp-commands.hx file:
> -
> -    {
> -        .name       = "query-alarm-clock",
> -        .args_type  = "",
> -    },
> -
>  Time to test the new command. Build qemu, run it as described in the "Testing"
>  section and try this:
>  
> @@ -597,13 +566,6 @@ iteration of the loop. That's because the alarm timer method in use is the
>  first element of the alarm_timers array. Also notice that QAPI lists are handled
>  by hand and we return the head of the list.
>  
> -To test this you have to add the corresponding qmp-commands.hx entry:
> -
> -    {
> -        .name       = "query-alarm-methods",
> -        .args_type  = "",
> -    },
> -
>  Now Build qemu, run it as explained in the "Testing" section and try our new
>  command:
>  
> diff --git a/qmp-commands.hx b/qmp-commands.txt
> similarity index 87%
> rename from qmp-commands.hx
> rename to qmp-commands.txt

Shouldn't qmp-commands.txt live in docs/?

> index 1ad8dda..f796342 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.txt
> @@ -1,8 +1,3 @@

Instead of reviewing this diff, let me review how your patch changes
qmp-commands.txt:

   diff -u bld-x86/qmp-commands.txt qmp-commands.txt
  --- bld-x86/qmp-commands.txt	2016-08-05 16:51:04.854000570 +0200
  +++ qmp-commands.txt	2016-08-05 16:54:23.650093974 +0200
  @@ -3381,7 +3381,7 @@
                       "child": "children.1" } }
   <- { "return": {} }

  -@query-named-block-nodes
  +query-named-block-nodes
   ------------------------

   Return a list of BlockDeviceInfo for all the named block driver nodes
  @@ -3507,7 +3507,7 @@
        ]
      }

  -@query-memory-devices
  +query-memory-devices
   --------------------

   Return a list of memory devices.
  @@ -3525,7 +3525,8 @@
                           "slot": 0},
                      "type": "dimm"
                    } ] }
  -@query-acpi-ospm-status
  +
  +query-acpi-ospm-status
   --------------------

   Return list of ACPIOSTInfo for devices that support status reporting
  @@ -3538,6 +3539,7 @@
                    { "slot": "2", "slot-type": "DIMM", "source": 0, "status": 0},
                    { "slot": "3", "slot-type": "DIMM", "source": 0, "status": 0}
      ]}
  +
   rtc-reset-reinjection
   ---------------------

  @@ -3549,6 +3551,7 @@

   -> { "execute": "rtc-reset-reinjection" }
   <- { "return": {} }
  +
   trace-event-get-state
   ---------------------

  @@ -3572,6 +3575,7 @@

   -> { "execute": "trace-event-get-state", "arguments": { "name": "qemu_memalign" } }
   <- { "return": [ { "name": "qemu_memalign", "state": "disabled" } ] }
  +
   trace-event-set-state
   ---------------------

Sensible fixes only.  Separate patch?