Re: [Qemu-devel] [PATCH for-2.9 00/47] qapi: Put type information back into QMP documentation

[Marc-André Lureau]

Hi

On Mon, Mar 13, 2017 at 4:14 PM Markus Armbruster <address@hidden> wrote:

> Marc-André Lureau <address@hidden> writes:
>
> > Hi
> >
> > On Mon, Mar 13, 2017 at 10:23 AM Markus Armbruster <address@hidden>
> > wrote:
> >
> >> I'm proposing this is 2.9 because it fixes a documentation regression.
> >> It affects only documentation; generated C code is unchanged except
> >> for the removal of trailing space in PATCH 46.
> >>
> >> Based on my qapi-next branch, which contains Marc-André's PATCH 1/2.
> >>
> >> Marc-André's work to merge qmp-commands.txt and qmp-events.txt into
> >> the QAPI schema and generate their replacements from the schema
> >> (commit b6af8ea..56e8bdd) was a big step forward.  As committed, it
> >> also was a step back: the documentation lost information on JSON
> >> types, because I didn't like Marc-André's patch to add it.  He
> >> reposted it for further review afterwards:
> >>
> >>     Subject: [PATCH 0/2] qapi2texi: add type information
> >>     Message-Id: <address@hidden>
> >>     https://lists.gnu.org/archive/html/qemu-devel/2017-01/msg05432.html
> >>
> >> His PATCH 1/2 is a straightforward cleanup.  His PATCH 2/2 adds type
> >> descriptions in a new formal language to the generated documentation.
> >> Quoting the commit message:
> >>
> >>     Array types have the following syntax: type[]. Ex: str[].
> >>
> >>     - Struct, commands and events use the following members syntax:
> >>
> >>       { 'member': type, ('foo': str), ... }
> >>
> >>     Optional members are under parentheses.
> >>
> >>     A structure with a base type will have 'BaseStruct +' prepended.
> >>
> >>     - Alternates use the following syntax:
> >>
> >>       [ 'foo': type, 'bar': type, ... ]
> >>
> >>     - Simple unions use the following syntax:
> >>
> >>       { 'type': str, 'data': 'type' = [ 'foo': type, 'bar': type... ] }
> >>
> >>     - Flat unions use the following syntax:
> >>
> >>       BaseStruct + 'discriminator' = [ 'foo': type, 'bar': type... ]
> >>
> >> End quote.  Looks like this in generated documentation:
> >>
> >>  -- Event: VNC_CONNECTED {'server': VncServerInfo, 'client':
> >>           VncBasicInfo}
> >>
> >>      Emitted when a VNC client establishes a connection
> >>      ''server''
> >>           server information
> >>      ''client''
> >>           client information
> >>
> >>      Note: This event is emitted before any authentication takes place,
> >>      thus the authentication ID is not provided
> >> [...]
> >>
> >>  -- Struct: VncServerInfo VncBasicInfo + {('auth': str)}
> >>
> >>      The network connection information for server
> >>      ''auth'' (optional)
> >>           authentication method used for the plain (non-websocket) VNC
> >>           server
> >>
> >>      Since: 2.1
> >>
> >>  -- Simple Union: SocketAddress { 'type': str, 'data': 'type' = ['inet':
> >>           InetSocketAddress, 'unix': UnixSocketAddress, 'vsock':
> >>           VsockSocketAddress, 'fd': String] }
> >>
> >>      Captures the address of a socket, which could also be a named file
> >>      descriptor
> >>
> >>      Since: 1.3
> >>
> >> Here's my counter-proposal: instead of inventing a formal language,
> >> fix the natural language documentation to actually mention *all*
> >> members, and add type information in a plain, easy-to-understand way.
> >> Looks like this:
> >>
> >>  -- Event: VNC_CONNECTED
> >>
> >>      Emitted when a VNC client establishes a connection
> >>
> >>      Arguments:
> >>      'server: VncServerInfo'
> >>           server information
> >>      'client: VncBasicInfo'
> >>           client information
> >>
> >>      Note: This event is emitted before any authentication takes place,
> >>      thus the authentication ID is not provided
> >> [...]
> >>
> >>  -- Object: VncServerInfo
> >>
> >>      The network connection information for server
> >>
> >>      Members:
> >>      'auth: string' (optional)
> >>           authentication method used for the plain (non-websocket) VNC
> >>           server
> >>      The members of 'VncBasicInfo'
> >>
> >>      Since: 2.1
> >>
> >>  -- Object: SocketAddress
> >>
> >>      Captures the address of a socket, which could also be a named file
> >>      descriptor
> >>
> >>      Members:
> >>      'type'
> >>           One of "inet", "unix", "vsock", "fd"
> >>      'data: InetSocketAddress' when 'type' is "inet"
> >>      'data: UnixSocketAddress' when 'type' is "unix"
> >>      'data: VsockSocketAddress' when 'type' is "vsock"
> >>      'data: String' when 'type' is "fd"
> >>
> >>      Since: 1.3
> >>
> >>
> > I like both, to me they serve different purposes. I like to have a short
> > overview / signature and then a more detailed documentation for each
> field.
>
> I sympathize with the argument.  Unfortunately, the "short" signatures
> are anything but for real-world QAPI:
>

That's a worse case, a regular case is more readable. And it is still
useful anyway since the common members would be listed first.


>  -- Flat Union: BlockdevOptions {'driver': BlockdevDriver, ('node-name':
>           str), ('discard': BlockdevDiscardOptions), ('cache':
>           BlockdevCacheOptions), ('read-only': bool), ('detect-zeroes':
>           BlockdevDetectZeroesOptions)} + 'driver' = ['archipelago':
>           BlockdevOptionsArchipelago, 'blkdebug':
>           BlockdevOptionsBlkdebug, 'blkverify':
>           BlockdevOptionsBlkverify, 'bochs':
>           BlockdevOptionsGenericFormat, 'cloop':
>           BlockdevOptionsGenericFormat, 'dmg':
>           BlockdevOptionsGenericFormat, 'file': BlockdevOptionsFile,
>           'ftp': BlockdevOptionsCurl, 'ftps': BlockdevOptionsCurl,
>           'gluster': BlockdevOptionsGluster, 'host_cdrom':
>           BlockdevOptionsFile, 'host_device': BlockdevOptionsFile,
>           'http': BlockdevOptionsCurl, 'https': BlockdevOptionsCurl,
>           'iscsi': BlockdevOptionsIscsi, 'luks': BlockdevOptionsLUKS,
>           'nbd': BlockdevOptionsNbd, 'nfs': BlockdevOptionsNfs,
>           'null-aio': BlockdevOptionsNull, 'null-co':
>           BlockdevOptionsNull, 'parallels':
>           BlockdevOptionsGenericFormat, 'qcow2': BlockdevOptionsQcow2,
>           'qcow': BlockdevOptionsGenericCOWFormat, 'qed':
>           BlockdevOptionsGenericCOWFormat, 'quorum':
>           BlockdevOptionsQuorum, 'raw': BlockdevOptionsRaw, 'rbd':
>           BlockdevOptionsRbd, 'replication': BlockdevOptionsReplication,
>           'sheepdog': BlockdevOptionsSheepdog, 'ssh':
>           BlockdevOptionsSsh, 'vdi': BlockdevOptionsGenericFormat,
>           'vhdx': BlockdevOptionsGenericFormat, 'vmdk':
>           BlockdevOptionsGenericCOWFormat, 'vpc':
>           BlockdevOptionsGenericFormat, 'vvfat': BlockdevOptionsVVFAT]
>
>      Options for creating a block device.  Many options are available
>      for all block devices, independent of the block driver:
>      ''driver''
>           block driver name
>      ''node-name'' (optional)
>           the node name of the new node (Since 2.0).  This option is
>           required on the top level of blockdev-add.
>      ''discard'' (optional)
>           discard-related options (default: ignore)
>      ''cache'' (optional)
>           cache-related options
>      ''read-only'' (optional)
>           whether the block device should be read-only (default: false)
>      ''detect-zeroes'' (optional)
>           detect and optimize zero writes (Since 2.1) (default: off)
>      Remaining options are determined by the block driver.
>
>      Since: 1.7
>
> >> Additionally, my series fixes a number of bugs and cleans up along the
> >> way.  In particular, it converts qapi2texi.py from parse trees to the
> >> visitor interface the other generators use.
> >>
> >>
> > Your series failed to apply in patchew, and I can't find the branch in
> your
> > repo. Could you publish it?
>
> Done: branch qapi-doc at http://repo.or.cz/w/qemu/armbru.git
>
>
thanks


> >> Future generated documentation work includes eliding types that aren't
> >> visible in QMP (like introspection does), and making uses of type
> >> names links in HTML.
> >>
> >>
> > Yes, links would be really nice.
>
> Your work brought them into reach, let's grab them :)
>
-- 
Marc-André Lureau