From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40331) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WvVLY-0003RR-5Y for qemu-devel@nongnu.org; Fri, 13 Jun 2014 13:33:05 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WvVLO-0004G7-IP for qemu-devel@nongnu.org; Fri, 13 Jun 2014 13:33:00 -0400 Received: from mx1.redhat.com ([209.132.183.28]:11051) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WvVLO-0004Fm-9S for qemu-devel@nongnu.org; Fri, 13 Jun 2014 13:32:50 -0400 Message-ID: <539B35BE.5030603@redhat.com> Date: Fri, 13 Jun 2014 11:32:46 -0600 From: Eric Blake MIME-Version: 1.0 References: <1401970944-18735-1-git-send-email-wenchaoqemu@gmail.com> <1401970944-18735-6-git-send-email-wenchaoqemu@gmail.com> In-Reply-To: <1401970944-18735-6-git-send-email-wenchaoqemu@gmail.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="XWJQtJ1V74Frb7u3SUM0n1pNK7860T6rP" Subject: Re: [Qemu-devel] [PATCH V6 05/29] qapi: adjust existing defines List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Wenchao Xia , qemu-devel@nongnu.org Cc: mdroth@linux.vnet.ibm.com, armbru@redhat.com, lcapitulino@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --XWJQtJ1V74Frb7u3SUM0n1pNK7860T6rP Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 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. >=20 > 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. >=20 > Signed-off-by: Wenchao Xia > --- > 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(-) >=20 > +++ b/qapi-schema.json > @@ -1163,21 +1163,59 @@ > { 'command': 'query-blockstats', 'returns': ['BlockStats'] } > =20 > ## > -# @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 o= n the > -# host system's service database so symbolic names should no= t 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= =2E > +# > +# @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 Disting= uished > # 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. > =20 > ## > # @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': ['VncClientI= nfo']} } [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 --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --XWJQtJ1V74Frb7u3SUM0n1pNK7860T6rP Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJTmzW+AAoJEKeha0olJ0NqGrYIAJyfxgOwUk5cKvYl5KhP20qw kG/z1VG0XfDlCCbznJ+amx8zDSGxbXjPN68E0aIWMB6FFMxX4ni5h/RfanEOE9uj clobDez5zpmH90QT0WQ8ETQzOW4XYH8PJ5WFLKYAC+wvwkeP+qESuTslMyyEyQVe q9wXwn2bejmXqk9HawhPcgcvv3Ogzn4XYC8bn/u3Vfk1WnmH5qBXN4nH72sMVAJh O/Bf/3ucDHWitBD9Mr4HR5me/Tz/TCOQ7nafVCHURQeyKbQ+p48ZM6OORGZKA3TJ vZ1F4xo9Y7J2wTQfcaT+19bq1GqA9UHPNuPynGJKspgEQnrLemqi2fW9/lEq3uQ= =Lvaq -----END PGP SIGNATURE----- --XWJQtJ1V74Frb7u3SUM0n1pNK7860T6rP--