Re: [Qemu-devel] Contribution - L2TPv3 transport

[Eric Blake]

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