qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Marc-André Lureau
Subject: Re: [Qemu-devel] [PATCH for-2.9 00/47] qapi: Put type information back into QMP documentation
Date: Wed, 15 Mar 2017 14:00:52 +0000

Hi

On Tue, Mar 14, 2017 at 8:14 PM Markus Armbruster <address@hidden> wrote:

> Marc-André Lureau <address@hidden> writes:
>
> > Hi
> >
> > On Mon, Mar 13, 2017 at 5:12 PM Markus Armbruster <address@hidden>
> wrote:
> >
> >> Marc-André Lureau <address@hidden> writes:
> >>
> >> > 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.
> >>
> >> There are readable cases, but there are plenty of cases that plainly
> >> aren't.
> >>
> >> 102 out of 472 signatures don't count because they're empty.
> >>
> >> Roughly half the non-empty signatures fit on a single line.  That's
> short.
> >>
> >> A bit under a third take two lines.  I guess that's still short enough.
> >>
> >> More than one in six signatures is three lines or more.
> >>
> >> >                                                       And it is still
> >> > useful anyway since the common members would be listed first.
> >>
> >> Whatever comes first in signatures comes first in the table of members,
> >> too.  The names are easier to spot there, because they're all on the
> >> left.
> >>
> >> Compare
> >>
> >>  -- 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
> >>
> >> to
> >>
> >>  -- 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
> >>
> >> In my opinion, the three lines of signature add nothing but noise to the
> >> six lines of member table.
> >>
> >
> > It is more natural and faster to read to me for commands and events for
> > example.  The verbose description is mixing description and sometime even
> > providing redundant information (ex: keys: array of KeyValue,  An array
> of
> > 'KeyValue' elements...), slowing reading even more.
>
> Doc comments that merely restate the type should be cleaned up.
>
>
It's not just that, it's also the verbosity of the description that
clutters the information you need.

>                                                     Often you don't need
> to
> > read the documentation / description, you want to quickly check the
> return
> > type, and remind you the arguments.
>
> Point taken.
>
> A formal description of unbounded (and often excessive) length can't
> serve that purpose, though.
>

In which case we are screwed anyway in any form


>
> A sufficiently condensed summaries just might.  Perhaps names only, no
> types.  Certainly no more than a few.
>
> For instance, having
>
>  -- Command: block-job-set-speed device speed
>
> instead of just
>
>  -- Command: block-job-set-speed
>
> feels okay; the additional two words are technically redundant, but they
> might occasionally serve someone as a reminder, and they're not
> distracting.
>

In my generated pdf, using my type proposal (I use it daily), I have:

block-job-set-speed (’device’: str , ’speed’: int )

Which I find useful. I am going to lack it, and in fact I'll probably
maintain my own version of the document if we don't have that declaration
form. At least for a while, until I get used to your type version
eventually).

It feels like bikeshedding at this point, and you are the maintainer, so
I'll probably stop arguing, but it doesn't mean I am satisfied with your
proposal.


> But I feel
>
>  -- Command: blockdev-mirror [job-id] device target [replaces] sync
>           [speed] [granularity] [buf-size] [on-source-error]
>           [on-target-error] [filter-node-name]
>
> is pushing it.
>
> So this begs the question which ones to omit when there are more than a
> few.  I'm afraid asking a stupid computer program to pick out
> "important" arguments is asking for too much.  For high-quality
> summaries, we'd have to pick ourselves.
>
> Moreover, what to do for truly complex commands like blockdev-add?
> Simply omitting all variant members is one option:
>
>  -- Command: blockdev-add driver [node-name] [discard] [cache]
>           [read-only] [detect-zeroes] ...
>
> But what may work for blockdev-add need not work for other complex
> commands.
>
> > struct/objects are more commonly declared with a line per member, so it
> > doesn't bother me as much.
> >
> > I would appreciate if can have the declarative form for commands and
> events
> > at least. Other types are usually more complex or long, so that may clear
> > your concerns for the long declarations.
>
> The worst offenders are actually commands such as blockdev-add and
> block_set_io_throttle, unless we give up on the "reminder" mission for
> them and merely add a reference to their (named) argument type.
>
-- 
Marc-André Lureau


reply via email to

[Prev in Thread] Current Thread [Next in Thread]