On 03/04/2014 08:19 AM, Anton Ivanov (antivano) wrote: > Attached is a revised version. >
> + > + > +#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 > +#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; > + unsigned int index; > + unsigned int packet_len; > + > + /* > + * these are used for xmit - that happens packet a time > + * and for first sign of life packet (easier to parse that once) > + */ > + > + 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; > + 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 == 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 = (uint32_t *) s->header_buf; > + stl_be_p(header, 0x30000); > + } > + session = (uint32_t *) (s->header_buf + s->session_offset); > + stl_be_p(session, s->tx_session); > + > + if (s->cookie == TRUE ) { > + if (s->cookie_is_64 == TRUE) { More cases of TRUE that should be fixed. Also, no space before ). > + if (s->nocounter == FALSE) { > + counter = (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 = 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 = - errno; > + } else if (ret == 0) { > + ret = iov_size (iov, iovcnt); No space before ( in function calls. > + vec++; > + vec->iov_base = (void *) buf; This is C, not C++ - no need to cast to (void*). > + if (ret < 0) { > + ret = - errno; No space after unary '-'. > + } else if (ret == 0) { > + ret = size; > + } else { > + ret =- s->offset; Trailing whitespace. Please run your submission through scripts/checkpatch.pl, and address all warnings. '=-' looks odd; did you mean '= -'? > + > +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 == FALSE) && (s->ipv6 == FALSE)){ Too many (), missing space before { - this should be: if (!s->udp && !s->ipv6) { > + buf += sizeof(struct iphdr) /* fix for ipv4 raw */; > + } > + if (s->cookie == TRUE) { > + if (s->cookie_is_64 == TRUE) { > + /* 64 bit cookie */ > + cookie64 = (uint64_t *)(buf + s->cookie_offset); > + if ( ldq_be_p(cookie64) != 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 = (uint32_t *)(buf + s->cookie_offset); > + if (ldl_be_p(cookie32) != * (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 = (uint32_t *) (buf + s->session_offset); > + if (ldl_be_p(session) != 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 = s->msgvec; > + offset = s->offset; > + if ((s->udp == FALSE) && (s->ipv6 == FALSE)){ > + offset += sizeof(struct iphdr); > + } Whitespace damage. > + count = recvmmsg(s->fd, msgvec, MAX_L2TPV3_MSGCNT, MSG_DONTWAIT, NULL); > + for (i=0;i<count;i++) { Should be: for (i = 0; i < count; i++) { Lots of other sites need similar fixes. > + > + if ((getaddrinfo(l2tpv3->src, srcport, &hints, &result) !=0) || (result > == NULL)) { > + fd = -errno; > + fprintf(stderr, "l2tpv3_open : could not resolve src, " "errno = %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] = net_init_bridge, > #endif > [NET_CLIENT_OPTIONS_KIND_HUBPORT] = net_init_hubport, > +#ifdef CONFIG_LINUX > + [NET_CLIENT_OPTIONS_KIND_L2TPV3] = net_init_l2tpv3, > +#endif Alignment looks off. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature