Hi David. New patch attached, some comments inline. Thanks, -ralph Signed-off-by: Ralph Schmieder <ralph.schmieder at gmail.com> -------------- next part -------------- A non-text attachment was scrubbed... Name: oc-qos-v4.patch Type: application/octet-stream Size: 7004 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/openconnect-devel/attachments/20151017/4051af1b/attachment.obj> -------------- next part -------------- On Oct 07, 2015, at 11:07 AM GMT+2, David Woodhouse <dwmw2 at infradead.org> wrote: > On Fri, 2015-08-14 at 18:59 +0200, Ralph Schmieder wrote: >> Here we go again. Thanks for the comments, hope that I got everything >> right. For getting the TCLASS I could have used the word instead of >> the longword, too. But I guess there's no penalty for doing it this >> way, or is there? And it could use some testing beyond the simple >> IPv4 in IPv4 use case of mine :) > > Thanks again for working on this, and apologies again for the delay. > > I'm still slightly nervous about the whole concept ? we are > deliberately leaking information from the inner packet into the outer > packet. So people will be able to *see* that we're doing VoIP > traffic.... which in practice they could have inferred quite trivially > from the packet size and regularity anyway. > > But now I look harder, I see that OpenVPN does already have this > facility, at least for Legacy IP, with the --passtos option. It's > disabled by default though, and I wonder if we should do the same. And > make the option have the same name too? changed the option to --passtos and given the name it's therefore also disabled by default > > I might ask on the OpenVPN list about passing the values through > between Legacy IP and IPv6, and propose a patch. > > As for the code... it looks good, in general, with a few minor problems > remaining: > > The IPV6_TCLASS sockopt requires an 'int', not a 'uint8_t'. I think > that IP_TOS can also take an 'int' on all platforms (that's what > OpenVPN uses), so let's just change that in dtls_mainloop(). done, made it an int. > (Actually, I also wonder if we should just be setting it per-packet by > using sendmsg(), rather than separately calling setsockopt() each time > it changes?) I'm not 100% sure where this change would be applied, probably further down in SSL_write() or gnutls_record_send(). For me, this is good enough from a performance point of view, I'm not sure if this warrants any detailed profiling. I'm running this with multi-megabit video calls and it behaves fine. But that's only my opinion. And since you now have to deliberately enable it I don't see that much of an issue. Is performance your main concern? > > I think your initial value of vpninfo->dtls_tos_current wants to be > something that's *completely* outside the range of normal values, to > ensure that it does correctly get set the first time. When opening a socket, the default is set to 0 anyway. Anyway, I set it to 255 which is way outside the possible values so it should be set for the first packet sent. > > I'm also not sure you're extracting the tclass from the IPv6 header > correctly: > tos = (ntohl(0x0FF00000) & load_be32(this->data)) >> 20; > I don't > think the 0x0FF00000 needs to be byte-swapped, does it? You're *always* > going to get zero with the above version on a little-endian machine? > > The bits you're after are the low 4 bits of the first byte (as the high nybble of tos), and the high 4 bits of the second byte (as low nybble). So I think this would give you the correct result: > tos = (load_be16(this->data) >> 4) & 0xff; > > thanks... adjusted. > > Finally, I think we need to expose this to the library API with an > openconnect_set_pass_tos() function. I added it to openconnect.h and library.c. I also added a few lines to libopenconnect.map.in assuming that this needs a new API version. But totally guessing here. You'll know what to do :) > > > -- > David Woodhouse Open Source Technology Centre > David.Woodhouse at intel.com Intel Corporation >
