On 04/03/14 16:33, Eric Blake wrote:
>
> +
> +
> +#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.
OK.
>
>> +#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).
OK, will fix.
>
>> +
>> + /*
>> + * 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.
Forgot to clean it :)
>
>> +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) {
OK.
>
>> + 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.
OK
>
>> +{
>> + 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.
Thanks, will look into it.
>
>> + 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 '= -'?
ret - s->offset /* you need to adjust the ret so that it reflects the
data and not data + header */
>
>
>> +
>> +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) {
OK
>
>> + 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?
Carryover from UML - you have different semantics. There you have to
return 0.
>
>> + }
>> + } 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.
OK.
>
>> + 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?
Maybe - I am not on such a platform so it is very difficult for me to
assess the impact of this.
>
>> + 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?
Once upon a time it was on two lines :)
>
> getaddrinfo() does NOT set errno. Rather, it returns a code that you
> decipher with gai_strerror(). Your error reporting here is very suspect.
Thanks, broke that when converting and accounting for your other comments.
>
>> +++ 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.
>
