Re: [Qemu-devel] [PATCH V6 05/29] qapi: adjust existing defines

[Eric Blake <eblake@redhat.com>]

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

On 06/05/2014 06:22 AM, Wenchao Xia wrote:
> In order to let event defines use existing types later, instead of
> redefine new ones, some old type defines for spice and vnc are changed,
> and BlockErrorAction is moved from block.h to qapi schema. Note that
> BlockErrorAction is not merged with BlockdevOnError.

If you were to respin this, I'd split it into two - one part moving
BlockErrorAction from block.h into the schema, and the other tweaking
spice and vnc schema members.  But at this point, I'm more interested in
getting it into the tree than worrying about another round of delays for
a respin.

> 
> One thing not perfect is that, VncInfo should be foldered but may break

s/that,/that/

"foldered" is not a word, but off-hand I have no idea what you meant. [1]

> API stability.
> 
> Signed-off-by: Wenchao Xia <wenchaoqemu@gmail.com>
> ---
>  block.c                |   17 ++++---
>  block/backup.c         |    2 +-
>  block/mirror.c         |    7 ++-
>  block/stream.c         |    4 +-
>  blockjob.c             |   11 ++--
>  hmp.c                  |    5 +-
>  hw/block/virtio-blk.c  |    6 +-
>  hw/ide/core.c          |    6 +-
>  hw/scsi/scsi-disk.c    |    6 +-
>  include/block/block.h  |    4 --
>  include/qemu/sockets.h |    2 +
>  qapi-schema.json       |  126 ++++++++++++++++++++++++++++++++++++++----------

Of course, some of this is in qapi/block-core.json now; but Paolo's
rebased series picked up on that.

>  ui/spice-core.c        |    7 ++-
>  ui/vnc.c               |    9 ++--
>  util/qemu-sockets.c    |   10 ++++
>  15 files changed, 156 insertions(+), 66 deletions(-)
> 

> +++ b/qapi-schema.json
> @@ -1163,21 +1163,59 @@
>  { 'command': 'query-blockstats', 'returns': ['BlockStats'] }
>  
>  ##
> -# @VncClientInfo:
> +# @NetworkAddressFamily
>  #
> -# Information about a connected VNC client.
> +# The network address family
> +#
> +# @ipv4: IPV4 family
> +#
> +# @ipv6: IPV6 family
> +#
> +# @unix: unix socket
> +#
> +# @unknown: otherwise
> +#
> +# Since: 2.1
> +##
> +{ 'enum': 'NetworkAddressFamily',
> +  'data': [ 'ipv4', 'ipv6', 'unix', 'unknown' ] }

Nice. Conversion of 'str' to 'NetworkAddressFamily' has no impact to the
on-the-wire format, but makes us have better type safety.

> +
> +##
> +# @VncBasicInfo
>  #
> -# @host: The host name of the client.  QEMU tries to resolve this to a DNS name
> -#        when possible.
> +# The basic information for vnc network connection
>  #
> -# @family: 'ipv6' if the client is connected via IPv6 and TCP
> -#          'ipv4' if the client is connected via IPv4 and TCP
> -#          'unix' if the client is connected via a unix domain socket
> -#          'unknown' otherwise
> +# @host: IP address
>  #
> -# @service: The service name of the client's port.  This may depends on the
> -#           host system's service database so symbolic names should not be
> -#           relied on.
> +# @service: The service name of vnc port. This may depend on the host system's

s/of/of the/

> +#           service database so symbolic names should not be relied on.
> +#
> +# @family: address family
> +#
> +# Since: 2.1
> +##
> +{ 'type': 'VncBasicInfo',
> +  'data': { 'host': 'str',
> +            'service': 'str',
> +            'family': 'NetworkAddressFamily' } }
> +
> +##
> +# @VncServerInfo
> +#
> +# The network connection information for server
> +#
> +# @auth: #optional, authentication method

I wonder if this parameter should eventually be converted to an enum
rather than open-coded str, but that doesn't need to happen right away.

> +#
> +# Since: 2.1
> +##
> +{ 'type': 'VncServerInfo',
> +  'base': 'VncBasicInfo',
> +  'data': { '*auth': 'str' } }
> +
> +##
> +# @VncClientInfo:
> +#
> +# Information about a connected VNC client.
>  #
>  # @x509_dname: #optional If x509 authentication is in use, the Distinguished
>  #              Name of the client.
> @@ -1188,8 +1226,8 @@
>  # Since: 0.14.0
>  ##
>  { 'type': 'VncClientInfo',
> -  'data': {'host': 'str', 'family': 'str', 'service': 'str',
> -           '*x509_dname': 'str', '*sasl_username': 'str'} }
> +  'base': 'VncBasicInfo',
> +  'data': { '*x509_dname'   : 'str', '*sasl_username': 'str' } }

Spurious addition of spaces; s/   :/:/ in your cleanup patch.

>  
>  ##
>  # @VncInfo:
> @@ -1228,7 +1266,8 @@
>  # Since: 0.14.0
>  ##
>  { 'type': 'VncInfo',
> -  'data': {'enabled': 'bool', '*host': 'str', '*family': 'str',
> +  'data': {'enabled': 'bool', '*host': 'str',
> +           '*family': 'NetworkAddressFamily',
>             '*service': 'str', '*auth': 'str', '*clients': ['VncClientInfo']} }

[1] Okay, I think I see what you meant in your commit message.  You did
NOT convert 'VncInfo' to use 'VncBasicInfo' as a base class, because you
were worried about whether it would break things.  And your worry is
justified - VncBasicInfo marks 'host', 'family', and 'service' as
manadatory, while 'VncInfo' marks them as optional.

If VncInfo is only used on the output side of commands (which indeed
appears to be the case - it is only used as the return of 'query-vnc'),
then converting from optional to mandatory is fine from the
backwards-compatibility aspect; the only question is whether the
existing code for query-vnc has sane defaults for those three fields in
the case where it was previously omitting them.  If you choose to make
that change, do it as a followup patch.  This patch is fine.  And now
that I understand your concern, I'd rewrite that paragraph in the commit
message to this (and either Paolo can rebase it back into his github
tree, or whoever applies the patches can make the tweak when adding my R-b):

At this point, VncInfo is not made a child of VncBasicInfo, due to the
difference in mandatory vs. optional parameters.

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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