OpenCPN/OpenCPN — pull request #124 by stripydog (2014-05-23 18:40:38+00:00)
Title: * Make TCP DataStreams bi-directional

* Change send buffer sockopt to Linux minimum
* Fix crash in creating datastreams from badly formatted params
* Fix crash when commiting datastreams with unvalidated options

This patch changes the behaviour of TCP DataStreams in OpenCPN. Serial and UDP DataStreams should be unaffected, with changes to the options tab set/greyed out as appropriate. With this patch "Server" no longer means "Output Only" and "Client" no longer means "Input Only".

Backwards compatibility and not breaking existing configs was a priority. Behaviour of existing configs *should* be unaffected.

There's lots more work to do here but I thought I'd submit this for feedback before proceeding further. This has been tested on Linux (Ubuntu 12.04). Assuming feedback on the patch is OK I will get hold of a mac to test on but there's nothing too platform-specific in here

New TCP Behaviour:
In addition to the "Output on this port" checkbox, there's a checkbox on the connections tab to receive input. For serial and udp connections this is checked and greyed out: Per pre-existing code there is no "output only" option: I will come back to that in a later patch if this one works out ok. To preserve compatibility with existing config files I had to change round the order of DS_TYPE_INPUT_OUTPUT and DS_TYPE_OUTPUT in the dsPortType enum. You might prefer to sacrifice backwards compatibility for a more logical ordering. For TCP connections:
With input box checked and output box unchecked, the connection is DS_TYPE_INPUT
With Input box checked and output box checked, the connection is DS_TYPE_INPUT_OUTPUT
With the Input box unchecked and the output box checked the connection is DS_TYPE_OUTPUT
If neither box is checked the Apply callback will put up an error telling the user to check at least one of the two boxes. This also fixed an existing crash bug where connection data wasn't being validated when apply was hit.

When a user specifies an address of 0.0.0.0, a server datastream is created. This still has some of the same flaws as the current version: It's not a concurrent server (that comes in a subsequent patch) and things are *very* dodgy when a second client attempts to connect. This is unchanged from existing behaviour. This connection may be input, output or bi-directional according to the datastream options. The (to my mind a little aggressive) read timeout applies on these connections for input and bi-directional connections. I would probably drop that if the server was concurrent, but as we're currently only allowing a single client, dropping potentially dead connections makes sense.

When a user specifies a non-zero address a client datastream is created. Again this may be input, output or both and the watchdog timer doesn't apply on output connections. This *should* solve the issue that was being raised last week regarding bi-directional connections to the vesper unit.

The (very sensible) reduction in SO_SNDBUF that's been added recently is a little too aggressive for Linux: The minimum you can set SO_SNDBUF to on linux is 2048 (which equates to a parameter to setsockopt of 1024 which isn't very explicitly documented) so I've changed it to that. Previously the setsockopt was failing but the return code wasn't being checked. The return code *still* isn't being checked. I will confirm that value is ok for MacOS when I get a test rig set up.

If this is accepted, the next plan is to make the server side concurrent, followed by altering UDP sockets to follow the TCP model a bit more.

My C++ is a bit rusty but I've tried to follow your style and resist the temptation to do anything with pointers and bit masks :-). Criticism/rejection happily received