Re: [Qemu-devel] [PATCH v2 2/4] qapi: add SysEmuTarget to "common.json"

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2414 bytes --]

On 04/26/2018 01:34 PM, Laszlo Ersek wrote:
> We'll soon need an enumeration type that lists all the softmmu targets
> that QEMU (the project) supports. Introduce @SysEmuTarget to
> "common.json".
> 
> The enum constant @x86_64 is not spelled @x86-64 because the corresponding
> emulator is called qemu-system-x86_64.
> 
> Like other enum types, @SysEmuTarget too can be used for discriminator
> fields in unions. For the @i386 constant, a C-language union member called
> "i386" would be generated. On mingw build hosts, "i386" is a macro
> however. Add "i386" to "polluted_words" at once.
> 
> Cc: "Daniel P. Berrange" <berrange@redhat.com>
> Cc: Eric Blake <eblake@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---

> +++ b/qapi/common.json

> +
> +##
> +# @SysEmuTarget:
> +#
> +# The comprehensive enumeration of QEMU system emulation ("softmmu")
> +# targets. Run "./configure --help" in the project root directory, and
> +# look for the *-softmmu targets near the "--target-list" option. The
> +# individual target constants are not documented here, for the time
> +# being.
> +#
> +# Notes: @x86_64 doesn't match the QAPI convention of preferring hyphen
> +#        ("-") over underscore ("_"). This is intentional; the
> +#        @SysEmuTarget constants are supposed to produce QEMU executable
> +#        names when stringified and appended to the "qemu-system-"
> +#        prefix. Put differently, the replacement text of the
> +#        TARGET_NAME preprocessor macro must be possible to look up in
> +#        the list of (stringified) enum constants.

I think the Notes: goes a little bit deep into implementation details,
considering that it is part of the user-facing documentation.  Maybe it
is sufficient to shorten to just:

Notes: The resulting QMP strings can be appended to the "qemu-system-"
prefix to produce the corresponding QEMU executable name.

and maybe add:

This is true even for "qemu-system-x86_64".

and leave the rest for the commit message.

Everything else looks good, so depending on what happens to the Notes
section after other reviewers have chimed in, you can probably add:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]