On Mon, 15 May 2023 at 13:36, Markus Armbruster <arm...@redhat.com> wrote:
>
> Peter Maydell <peter.mayd...@linaro.org> writes:
>
> > Convert the qmp-spec.txt document to restructuredText.
> > Notable points about the conversion:
> > * numbers at the start of section headings are removed, to match
> > the style of the rest of the manual
> > * cross-references to other sections or documents are hyperlinked
>
> You also add new references to QMP and QGA reference manuals.
> Thoughtful!
>
> > * various formatting tweaks (notably the examples, which need the
> > -> and <- prefixed so the QMP code-block lexer will accept them)
>
> You could add
>
> * English prose fixed in a few places.
>
> Appreciated!
>
> > Reviewed-by: Eric Blake <ebl...@redhat.com>
> > Signed-off-by: Peter Maydell <peter.mayd...@linaro.org>
> > ---
> > v1->v2: minor tweaks per Eric's review
> > * consistently use '.' at end of sentences in Where: lists
> > * s/the same of the/the same as for the/
> > ---
> > docs/interop/index.rst | 1 +
> > docs/interop/{qmp-spec.txt => qmp-spec.rst} | 337 +++++++++++---------
> > 2 files changed, 186 insertions(+), 152 deletions(-)
> > rename docs/interop/{qmp-spec.txt => qmp-spec.rst} (55%)
>
> Leaves a few dangling references:
>
> $ git-grep -F qmp-spec.txt
>
> docs/devel/qapi-code-gen.rst:See qmp-spec.txt for out-of-band execution
> syntax and semantics.
> python/qemu/qmp/models.py: Defined in qmp-spec.txt, section 2.2,
> "Server Greeting".
> python/qemu/qmp/models.py: Defined in qmp-spec.txt, section 2.2,
> "Server Greeting".
> python/qemu/qmp/models.py: Defined in qmp-spec.txt, section 2.4.2,
> "error".
> python/qemu/qmp/models.py: Defined in qmp-spec.txt, section 2.4.2,
> "error".
> python/qemu/qmp/qmp_client.py: # See "NOTE" in qmp-spec.txt,
> section 2.4.2
> python/qemu/qmp/qmp_client.py: # qmp-spec.txt, section 2.4:
> qapi/control.json:# docs/interop/qmp-spec.txt)
> qapi/control.json:# qmp-spec.txt for more information on OOB)
> qapi/qapi-schema.json:# Please, refer to the QMP specification
> (docs/interop/qmp-spec.txt)
> qobject/json-lexer.c: * state; see docs/interop/qmp-spec.txt.
>
> Easy enough to fix up.
Yep, I'll correct these in v3. (The section numbers all have to go,
as the sections aren't numbered any more. The refs in qapi-code-gen.rst
and qapi-schema.json can turn into hyperlinks now.)
> > @@ -45,83 +45,89 @@ important unless specifically documented otherwise.
> > Repeating a key
> > within a json-object gives unpredictable results.
> >
> > Also for convenience, the server will accept an extension of
> > -'single-quoted' strings in place of the usual "double-quoted"
> > +``'single-quoted'`` strings in place of the usual ``"double-quoted"``
> > json-string, and both input forms of strings understand an additional
> > -escape sequence of "\'" for a single quote. The server will only use
> > +escape sequence of ``\'`` for a single quote. The server will only use
>
> Pre-patch plain text "\'" becomes just \' in HTML output, but rendered
> as code, i.e. in a fixed-width font. I'd prefer to retain the quotation
> marks, like ``"\'"``, just like in JSON RFC 8259, but then plain text
> output becomes ""\'"".
>
> Likewise, ``'single-quoted'`` and ``"double-quoted"`` produce
> "'single-quoted'" and ""double-quoted"" in plain text output.
>
> Can't win.
>
> git-grep '``"' docs/ shows preexisting instances.
>
> More of the same below, not flagging again. No use fighting the tool.
I'm not sure if this is requesting a change, so I'm going to
assume it is not :-)
> > -3.2 Capabilities negotiation
> > -----------------------------
> > + .. code-block:: QMP
> >
> > -C: { "execute": "qmp_capabilities", "arguments": { "enable": ["oob"] } }
> > -S: { "return": {}}
> > + <- { "QMP": {"version": {"qemu": {"micro": 0, "minor": 0, "major": 3},
> > + "package": "v3.0.0"}, "capabilities": ["oob"] } }
>
> Opportunity to adjust the spacing to match what the server actually
> sends:
>
> <- {"QMP": {"version": {"qemu": {"micro": 0, "minor": 0, "major": 3},
> "package": "v3.0.0"}, "capabilities": ["oob"]}}
>
> May want to adjust spacing in input as well for a more consistent look.
>
> Suggestion, not demand.
I think this is more change than I want to do at this point,
since it would require testing all the JSON examples in the
doc and fixing all the other bits of JSON in it.
> > +
> > +.. admonition:: Example
> > +
> > + Parsing error
> > +
> > + .. code-block:: QMP
> > +
> > + -> { "execute": }
> > + <- { "error": { "class": "GenericError", "desc": "Invalid JSON syntax"
> > } }
>
> This error changed long ago (I believe). Actual response is
>
> {"error": {"class": "GenericError", "desc": "JSON parse error,
> expecting value"}}
>
> Please update this one even if you decide to leave the spacing as is.
OK, but that should definitely be a separate patch.
thanks
-- PMM
