Re: [PATCH v2 1/2] docs/interop: Convert qmp-spec.txt to rST

[Peter Maydell]

On Mon, 15 May 2023 at 13:36, Markus Armbruster <armbru@redhat.com> wrote:
>
> Peter Maydell <peter.maydell@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 <eblake@redhat.com>
> > Signed-off-by: Peter Maydell <peter.maydell@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