Re: [Qemu-devel] Contribution - L2TPv3 transport

[Anton Ivanov (antivano)]

Hi Stefan,

I am addressing a few more comments which I missed on first pass.

> If you really *need* the page size, please use sysconf(_SC_PAGESIZE).

I like to have it page aligned and if possible page sized so I can later 
extend to do jumbo frame support via a vector. If this is the wrong way 
of doing it, I am happy to fix.

>
> If you just need a big buffer size, please rename this to L2TPV3_BUFSIZE
> or similar to avoid linking the name to page size.

At this stage ~ 1580bytes will do. However, later I would like to extend 
this so it works with jumbo frames so you can pass 8K packets if one 
wants to. My current ideas are that in order for that to happen 
efficiently the vectors should have 2-3 buffers page sized each 
(allocated only if the user request jumbo frames).

[snip]


> +    count = recvmmsg(s->fd, msgvec, MAX_L2TPV3_IOVCNT/2, MSG_DONTWAIT, NULL);
> +    for (i=0;i<count;i++) {
> +     if (msgvec->msg_len > 0) {
> +         vec = msgvec->msg_hdr.msg_iov;
> +         if (l2tpv3_verify_header(s, vec->iov_base) == 0) {
> Header verification should check the size of the packet too.  It must be
> large enough for the L2TPv3 header, otherwise we'd access stale data
> that happened to be in vec->iov_base...

That should be

msgvec->msg_len > offset


Thanks for noting it.


>
> +         //vec->iov_len = PAGE_SIZE; /* reset for next read */
> I think it *is* necessary to reset ->iov_len for both msgvec iovecs.

mmsgsend does not return these modified. However better be safe than 
sorry - I am uncommenting these in the next revision.


> Can you use C structs and unions instead of choosing an arbitrary
> 256-byte size and calculating offsets at runtime?

It is has now updated to be the correct size for the actual config.

As far as structs - not really.

I tried that once upon a time in an early version, I ended up with 8+ 
different structs (cookies can vary in size so you cannot union-ize 
them, compiler will allocate the size for the "biggest option"). In 
addition to that the standard has slightly different headers on raw and 
udp. The linux kernel people have done the same - header offsets. It is 
an unfortunate necessity for code like this.

Also, there is one nearly universal non-standard feature which I would 
like to put back. It is present in the linux kernel implementation and 
it is the "arbitrary offset after header" so you can stick extra 
metadata between header and packet. That will necessitate offset 
calculations anyway.

>
> typedef struct {
>      uint16_t flags;
>      uint16_t reserved;
>      uint32_t session_id;
>      union {
>       uint32_t cookie32;

[snip]


> This buffer isn't used?

Not any more, removing.


> Is there a way to disable the IP header included in received packets? 
> I haven't looked into how IP_HDRINCL works... 

It works the other way - you can get headers on v6 using that option, 
but v6 does not give you headers by default. AFAIK v4 raw always gives 
you the header do you like it or not. Makes the code very ugly 
unfortunately.

[snip]

A.