[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] Contribution - L2TPv3 transport
From: |
Anton Ivanov (antivano) |
Subject: |
Re: [Qemu-devel] Contribution - L2TPv3 transport |
Date: |
Tue, 4 Mar 2014 11:32:26 +0000 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20131103 Icedove/17.0.10 |
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.
Re: [Qemu-devel] Contribution - L2TPv3 transport, Eric Blake, 2014/03/04