Re: [Qemu-devel] Contribution - L2TPv3 transport

[Anton Ivanov (antivano)]

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.
>