From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37935) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WKnaR-0000Xi-BN for Qemu-devel@nongnu.org; Tue, 04 Mar 2014 06:32:46 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WKnaH-0002nx-A7 for Qemu-devel@nongnu.org; Tue, 04 Mar 2014 06:32:39 -0500 Received: from rcdn-iport-8.cisco.com ([173.37.86.79]:46040) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WKnaH-0002mi-5E for Qemu-devel@nongnu.org; Tue, 04 Mar 2014 06:32:29 -0500 From: "Anton Ivanov (antivano)" Date: Tue, 4 Mar 2014 11:32:26 +0000 Message-ID: <5315B9C8.1000300@cisco.com> References: <5310489A.4060501@cisco.com> <20140303145303.GF21055@stefanha-thinkpad.redhat.com> In-Reply-To: <20140303145303.GF21055@stefanha-thinkpad.redhat.com> Content-Language: en-US Content-Type: text/plain; charset="iso-8859-1" Content-ID: Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [Qemu-devel] Contribution - L2TPv3 transport List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: "Qemu-devel@nongnu.org" 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=20 extend to do jumbo frame support via a vector. If this is the wrong way=20 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=20 this so it works with jumbo frames so you can pass 8K packets if one=20 wants to. My current ideas are that in order for that to happen=20 efficiently the vectors should have 2-3 buffers page sized each=20 (allocated only if the user request jumbo frames). [snip] > + count =3D recvmmsg(s->fd, msgvec, MAX_L2TPV3_IOVCNT/2, MSG_DONTWAIT,= NULL); > + for (i=3D0;i + if (msgvec->msg_len > 0) { > + vec =3D msgvec->msg_hdr.msg_iov; > + if (l2tpv3_verify_header(s, vec->iov_base) =3D=3D 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 =3D 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=20 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+=20 different structs (cookies can vary in size so you cannot union-ize=20 them, compiler will allocate the size for the "biggest option"). In=20 addition to that the standard has slightly different headers on raw and=20 udp. The linux kernel people have done the same - header offsets. It is=20 an unfortunate necessity for code like this. Also, there is one nearly universal non-standard feature which I would=20 like to put back. It is present in the linux kernel implementation and=20 it is the "arbitrary offset after header" so you can stick extra=20 metadata between header and packet. That will necessitate offset=20 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?=20 > I haven't looked into how IP_HDRINCL works...=20 It works the other way - you can get headers on v6 using that option,=20 but v6 does not give you headers by default. AFAIK v4 raw always gives=20 you the header do you like it or not. Makes the code very ugly=20 unfortunately. [snip] A.=