From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44089) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WJNgA-0000yu-JO for Qemu-devel@nongnu.org; Fri, 28 Feb 2014 08:40:47 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WJNg2-0003jz-WC for Qemu-devel@nongnu.org; Fri, 28 Feb 2014 08:40:42 -0500 Received: from mx1.redhat.com ([209.132.183.28]:58611) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WJNg2-0003je-Hs for Qemu-devel@nongnu.org; Fri, 28 Feb 2014 08:40:34 -0500 Message-ID: <531091CB.9050908@redhat.com> Date: Fri, 28 Feb 2014 06:40:27 -0700 From: Eric Blake MIME-Version: 1.0 References: <5310489A.4060501@cisco.com> In-Reply-To: <5310489A.4060501@cisco.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="Bc7JliJlNQRfKBwAv1GDLBTwo7gw0M0gc" Subject: Re: [Qemu-devel] Contribution - L2TPv3 transport List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Anton Ivanov (antivano)" , "Qemu-devel@nongnu.org" This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --Bc7JliJlNQRfKBwAv1GDLBTwo7gw0M0gc Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 02/28/2014 01:28 AM, Anton Ivanov (antivano) wrote: > Hi all, >=20 > On behalf of Cisco Systems I am authorized to contribute a new transpor= t > to the network subsystem in qemu. >=20 >=20 > Patch attached. Your patch is huge - it might be better to break it into smaller logical chunks to make it easier to review. See this page for more hints: http://wiki.qemu.org/Contribute/SubmitAPatch Your patch is lacking a diffstat, and was sent as an attachment rather than inline. This makes it harder to review. I'm going to focus my review on just the qapi interface for now. > +++ b/net/l2tpv3.c > @@ -0,0 +1,630 @@ > +/* > + * QEMU System Emulator > + * > + * Copyright (c) 2003-2008 Fabrice Bellard > + * Copyright (c) 2012 Cisco Systems It's now 2014. > +++ b/qapi-schema.json > @@ -2941,6 +2941,45 @@ > '*udp': 'str' } } > =20 > ## > +# @NetdevL2TPv3Options > +# > +# Connect the VLAN to Ethernet over L2TPv3 Static tunnel > +# > +# @fd: #optional file descriptor of an already opened socket Other commands name this parameter 'fdname' (see 'add_client'); it takes an fd named via the 'getfd' command. However, this style is old, and new code should instead prefer file names rather than fd names. That is, we want the newer style of using 'add-fd', then referring to the file by name (/dev/fdset/nnn), and not the older style of 'getfd'; using a file name also gives you the flexibility of using actual Unix socket files stored in the file system. > +# > +# @src: source address > +# > +# @dst: #optional destination address > +# > +# @mode: bitmask mode for the tunnel (see l2tpv3.h for actual definiti= on) Ewww. This is is gross. This API should be self-documented here, without making the end-reader chase down a source file. And passing raw numbers for bit ids is not user-friendly. Better would be making this parameter be an enum (if you can describe all modes via a single name, as in 'mode':'cookie') or a series of bool arguments (perhaps optional with sane defaults, as in 'udp':true,'cookie':false). > +# > +# @txcookie:#optional 32 or 64 bit tx cookie for the tunnel (set mode = for actual type selection) > +# > +# @rxcookie:#optional 32 or 64 bit rx cookie for the tunnel (set mode = for actual type selection) > +# > +# @txsession: tx 32 bit session > +# > +# @rxsession: rx 32 bit session > +# > +# > +# Since 1.0 s/1.0/2.0/ > +## > +## > +{ 'type': 'NetdevL2TPv3Options', > + 'data': { > + '*fd': 'str', > + '*src': 'str',=20 You didn't list 'src' as optional above. Trailing whitespace - run your submission through checkpatch.pl. > + '*dst': 'str',=20 > + '*mode': 'str',=20 As mentioned above, 'mode' should not be a string. > + '*txcookie': 'str', > + '*rxcookie': 'str', > + '*txsession': 'str', > + '*rxsession': 'str' These should probably be 'int', not 'str'. If you have to hand-parse a string into a numeric value, you encoded the QMP wrong. > + > +} } > + > +## > +## > # @NetdevVdeOptions > # > # Connect the VLAN to a vde switch running on the host. > @@ -3021,6 +3060,7 @@ > 'nic': 'NetLegacyNicOptions', > 'user': 'NetdevUserOptions', > 'tap': 'NetdevTapOptions', > + 'l2tpv3': 'NetdevL2TPv3Options', Please add documentation that the l2tpv3 arm of this union was added in 2.0 (yes, I know we haven't been very good at documenting when unions were extended, but it can't hurt to do a better job going forward). > 'socket': 'NetdevSocketOptions', > 'vde': 'NetdevVdeOptions', > 'dump': 'NetdevDumpOptions', >=20 --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --Bc7JliJlNQRfKBwAv1GDLBTwo7gw0M0gc 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/ iQEcBAEBCAAGBQJTEJHLAAoJEKeha0olJ0NqVi4H/2U/zVnQLn2Rw5QDu0XFdBUT SpucOXCPattU2yq31EuxuDbaKN0WN4xyFJWR6zNrWLy18PK+X9z3AoVflP1gYQYJ Ut5/S3SK3VAyUxYdR26hRgLwlWQRwTtcaTkX/07cjfltnfNH11RZI2a0Jg8rxM/e Ttj2++RLS78C2IosoU6xsndoTpBtK38HemHZn2U9ibr33i4zRio3HlJ1GAVQ8xAb 9EPRQtDS32tQvdnuCu+wi6QL3oCntZKLFGWxg+JdlAZEKT0wzpQmeUNL2/oIzREO /5UY8a71gwQ6mp27Sco9fT9FujqrvQrun3DHjNZpyC/3LJLUls22m16LwjsWTRs= =A5jQ -----END PGP SIGNATURE----- --Bc7JliJlNQRfKBwAv1GDLBTwo7gw0M0gc--