From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52883) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WKsI0-0004Za-1F for Qemu-devel@nongnu.org; Tue, 04 Mar 2014 11:34:00 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WKsHu-0006Z5-Lz for Qemu-devel@nongnu.org; Tue, 04 Mar 2014 11:33:55 -0500 Received: from mx1.redhat.com ([209.132.183.28]:42129) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WKsHu-0006XO-CL for Qemu-devel@nongnu.org; Tue, 04 Mar 2014 11:33:50 -0500 Message-ID: <53160065.50908@redhat.com> Date: Tue, 04 Mar 2014 09:33:41 -0700 From: Eric Blake MIME-Version: 1.0 References: <5310489A.4060501@cisco.com> <53105EB0.3060702@redhat.com> <5315EEFE.6060605@cisco.com> In-Reply-To: <5315EEFE.6060605@cisco.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="6XWDI3LQqXHMFShMeR66NpmnSlceEDakr" Subject: Re: [Qemu-devel] Contribution - L2TPv3 transport List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Anton Ivanov (antivano)" , Paolo Bonzini Cc: "Qemu-devel@nongnu.org" , Stefan Hajnoczi This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --6XWDI3LQqXHMFShMeR66NpmnSlceEDakr Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 03/04/2014 08:19 AM, Anton Ivanov (antivano) wrote: > Attached is a revised version. >=20 > + > + > +#define PAGE_SIZE 4096 One of your earlier review comments suggested using sysconf or else renaming this, as not all systems have a page size of 4096. > +#define IOVSIZE 2 > +#define MAX_L2TPV3_MSGCNT 32=20 > +#define MAX_L2TPV3_IOVCNT (MAX_L2TPV3_MSGCNT * IOVSIZE) > + > +#ifndef IPPROTO_L2TP > +#define IPPROTO_L2TP 0x73 > +#endif > + > +typedef struct NetL2TPV3State { > + NetClientState nc; > + int fd; > + int state;=20 > + unsigned int index; > + unsigned int packet_len; > + > + /*=20 > + * these are used for xmit - that happens packet a time > + * and for first sign of life packet (easier to parse that once) > + */=20 > + > + uint8_t * header_buf; Most code uses this style: uint8_t *header_buf; with no space between a pointer designation and the variable name it applies to (several instances in your patch). > + > + /* > + * Bitmask mode determining encaps behaviour > + */ > + > + uint32_t offset; =20 > + uint32_t cookie_offset; > + uint32_t counter_offset; > + uint32_t session_offset; Comment is off, as there is no bitmask here. > +static int l2tpv3_form_header(NetL2TPV3State *s) { > + uint32_t *header; > + uint32_t *session; > + uint64_t *cookie64; > + uint32_t *cookie32; > + uint32_t *counter; > + > + if (s->udp =3D=3D TRUE) { We require a C99 compiler; use 'true' not 'TRUE' (glib's TRUE caters mainly to C89 compilers, and isn't necessarily a true boolean). For that matter, comparison against true or false is verbose; it's fine to just use: if (s->udp) { > + header =3D (uint32_t *) s->header_buf; > + stl_be_p(header, 0x30000); > + } > + session =3D (uint32_t *) (s->header_buf + s->session_offset); > + stl_be_p(session, s->tx_session); > + > + if (s->cookie =3D=3D TRUE ) { > + if (s->cookie_is_64 =3D=3D TRUE) { More cases of TRUE that should be fixed. Also, no space before ). > + if (s->nocounter =3D=3D FALSE) { > + counter =3D (uint32_t *)(s->header_buf + s->counter_offset); > + stl_be_p(counter, ++ s->counter); > + } TAB damage - we indent with spaces. No space after preincrement ++. > + return 0; > +} > + > +static ssize_t net_l2tpv3_receive_dgram_iov(NetClientState *nc, const = struct iovec *iov, int iovcnt) Long line; you can split after , to fit within 80 columns. > +{ > + NetL2TPV3State *s =3D DO_UPCAST(NetL2TPV3State, nc, nc); > + > + struct msghdr message; > + int ret; > + > + if (iovcnt > MAX_L2TPV3_IOVCNT - 1) { > + fprintf(stderr, "iovec too long %d > %d, change l2tpv3.h\n", iovcnt, = MAX_L2TPV3_IOVCNT); > + return -1; Is printing to stderr always the right thing to do? It seems to me that you should look into using QError. > + if (ret < 0) { > + ret =3D - errno; > + } else if (ret =3D=3D 0) { > + ret =3D iov_size (iov, iovcnt); No space before ( in function calls. > + vec++; > + vec->iov_base =3D (void *) buf; This is C, not C++ - no need to cast to (void*). > + if (ret < 0) { > + ret =3D - errno; No space after unary '-'. > + } else if (ret =3D=3D 0) { > + ret =3D size; > + } else { > + ret =3D- s->offset;=20 Trailing whitespace. Please run your submission through scripts/checkpatch.pl, and address all warnings. '=3D-' looks odd; did you mean '=3D -'? > + > +static int l2tpv3_verify_header(NetL2TPV3State *s, uint8_t *buf) { Opening { on its own line. > + > + uint64_t *cookie64; > + uint32_t *cookie32; > + uint32_t *session; > + > + if ((s->udp =3D=3D FALSE) && (s->ipv6 =3D=3D FALSE)){ Too many (), missing space before { - this should be: if (!s->udp && !s->ipv6) { > + buf +=3D sizeof(struct iphdr) /* fix for ipv4 raw */; > + }=20 > + if (s->cookie =3D=3D TRUE) { > + if (s->cookie_is_64 =3D=3D TRUE) { > + /* 64 bit cookie */ > + cookie64 =3D (uint64_t *)(buf + s->cookie_offset); > + if ( ldq_be_p(cookie64) !=3D s->rx_cookie) { > + fprintf(stderr, "unknown cookie id\n"); > + return -1; /* we need to return 0, otherwise barfus */ What's up with that comment being different from the code? > + } > + } else { > + cookie32 =3D (uint32_t *)(buf + s->cookie_offset); > + if (ldl_be_p(cookie32) !=3D * (uint32_t *) &s->rx_cookie) { > + fprintf(stderr,"unknown cookie id\n"); Space after ','. Again, I think QError is better than directly printing to stderr. > + return -1 ; /* we need to return 0, otherwise barfus */ > + } > + } > + } > + session =3D (uint32_t *) (buf + s->session_offset); > + if (ldl_be_p(session) !=3D s->rx_session) { Are you risking bus errors on platforms where you cannot dereference a wide int pointer that gets set to a misaligned offset? > + msgvec =3D s->msgvec; > + offset =3D s->offset; > + if ((s->udp =3D=3D FALSE) && (s->ipv6 =3D=3D FALSE)){ > + offset +=3D sizeof(struct iphdr); > + } =20 Whitespace damage. > + count =3D recvmmsg(s->fd, msgvec, MAX_L2TPV3_MSGCNT, MSG_DONTWAIT,= NULL); > + for (i=3D0;i + > + if ((getaddrinfo(l2tpv3->src, srcport, &hints, &result) !=3D0) || = (result =3D=3D NULL)) { > + fd =3D -errno; > + fprintf(stderr, "l2tpv3_open : could not resolve src, " "errno =3D %d= \n", fd); What's up with the string concatenation in the format string? getaddrinfo() does NOT set errno. Rather, it returns a code that you decipher with gai_strerror(). Your error reporting here is very suspect.= > +++ b/net/net.c > @@ -731,6 +731,9 @@ static int (* const net_client_init_fun[NET_CLIENT_= OPTIONS_KIND_MAX])( > [NET_CLIENT_OPTIONS_KIND_BRIDGE] =3D net_init_bridge, > #endif > [NET_CLIENT_OPTIONS_KIND_HUBPORT] =3D net_init_hubport, > +#ifdef CONFIG_LINUX > + [NET_CLIENT_OPTIONS_KIND_L2TPV3] =3D net_init_l2tpv3, > +#endif Alignment looks off. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --6XWDI3LQqXHMFShMeR66NpmnSlceEDakr 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/ iQEcBAEBCAAGBQJTFgBlAAoJEKeha0olJ0NqFAkH/0iiHr/g3e6TYrV+c4jP02Sd KYAi/7iDTzvQp4qKb8uIEEfofgbvJ8nUSqMqNO9NTErV+kiP158GZcOhS4ydxvN8 9D9r41606lW6C2+fSsvnRI+55d4n8Bm8Sg3EwRSn+ZDts//zs+8Um61yudgKZb9m Kb5//pIreYg4dUGCOKXijp1JVTBDl1ZUO8Sm/pvpdeu4nZ7giEYlNm+Twk9vaTOY p8lc3bD8aeP9knmkymqL9TX0oOykGBdPOrpttNV8KOnsJMAjMvvbsvqMvYBYo1ij ml5K+nAWfAKN6i6LxglJfe0iGIQAm9VbQ7Ygt5uG+QgVd+aFyf9EdKwbB3MpdlY= =MmJ2 -----END PGP SIGNATURE----- --6XWDI3LQqXHMFShMeR66NpmnSlceEDakr--