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

[Marc-André Lureau]

Hi

On Fri, Aug 5, 2016 at 6:58 PM Markus Armbruster <address@hidden> wrote:

> address@hidden writes:
>
> > From: Marc-André Lureau <address@hidden>
> >
> > 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 <address@hidden>
> > ---
> >  .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 <address@hidden>
> >  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/?
>
>
I don't mind. The following patches will generate the file. It could also
generate it under docs/. But then other documentation should also be
generated there. Let's leave itfor a different time.

> 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": {} }
>
>   address@hidden
>   +query-named-block-nodes
>    ------------------------
>
>    Return a list of BlockDeviceInfo for all the named block driver nodes
>   @@ -3507,7 +3507,7 @@
>         ]
>       }
>
>   address@hidden
>   +query-memory-devices
>    --------------------
>
>    Return a list of memory devices.
>   @@ -3525,7 +3525,8 @@
>                            "slot": 0},
>                       "type": "dimm"
>                     } ] }
>   address@hidden
>   +
>   +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?
>

ok
-- 
Marc-André Lureau