Bidirectional udp #12
Conversation
| @@ -253,6 +254,8 @@ class Driver | |||
| * The read_port port can be 0 if the local port does not need to be fixed. | |||
| */ | |||
| void openUDP(std::string const& hostname, int remote_port); | |||
| void openUDPBidirectional(std::string const& hostname, int out_port, int in_port); | |||
There was a problem hiding this comment.
Documentation is missing
|
@jmachowinski Forgot to push that, sorry. |
| @@ -194,6 +194,18 @@ void Driver::openURI(std::string const& uri) | |||
| { // UDP udp://hostname:remoteport | |||
| if (marker == string::npos) | |||
| throw std::runtime_error("missing port specification in udp:// URI"); | |||
|
|
|||
| string::size_type second_marker = device.find_last_of(":"); | |||
| int extended_info = 0; | |||
There was a problem hiding this comment.
Declare local variables in the deepest scope possible, and where they are used first.
|
I remember having dropped packet issues in the past if you get packets in a POSIX socket before you call read() and couldn't find any reference to what the expected behavior is so I just think it is safer to read() and then wirte() rather than the other way around. And readPacket() is sync, hence the threads. Do you think it is better to remove the threads? |
I very highly doubt that. Very. Think about it: there is always a time where the receiver is not reading: between the two calls to read. What if a packet arrives there ?!? You can have buffer overruns, especially in UDP, but other than that ... So, yes, I would very much prefer removing the threads. I like to have non-suspicious tests ... |
| string::size_type second_marker = device.find_last_of(":"); | ||
| if (second_marker != string::npos) | ||
| { | ||
| int remote_port = 0; |
There was a problem hiding this comment.
int remote_port = <actual value> there initializing it to assign it one line later is just noise.
|
The buffer overrun happens quite fast. |
While we should definitely control it, and thank you for pointing it out, "quite fast" is probably a very subjective measurement. The default receive buffer on my machine is 213kiB, which given that Linux limits UDP datagrams to the PMTU by default is 70 datagrams or something. I don't see that as horribly fast unless the pulling process (in most cases, the driver oroGen component) would really be sleepy. In any case, we should probably have a way to control these. Proposal |
|
In our use case we were streaming multiple ports from one system to one UDP port (including images) so it overrun quite fast.... |
| @@ -247,6 +257,73 @@ void Driver::openIPServer(int port, addrinfo const& hints) | |||
| setMainStream(new UDPServerStream(sfd,true)); | |||
| } | |||
|
|
|||
| void find_addr(const char *hostname, const char *port, addrinfo const& hints, struct sockaddr *addr, size_t *addr_len) | |||
|
Isin't that a bit counterintuitive? The streams/fds are created and set in the openIP* methods (openIPServer, openIPClient...). To make openIPBidirectional a static helper function I would have to pass a reference to the socket descriptor between the helper itself and the method that will create the stream. Is it what you prefer? |
|
The only one that makes sense is Unless I am mistaken, the creation and binding of the server-side socket is copy/pasted between openIPServer and openIPBidirectional. I think it would make a lot of sense that the creation of the IP server socket be refactored as a helper methods. E.g.
Get rid of the |
|
Makes sense. I was avoiding touching existing methods because there are no test cases for those but there is indeed a lot of repeated code and some refactoring would be good... 👍 |
|
I think it looks much better now! |
This is important to prevent leaking server socket descriptor in case createIPClientSocket throws
|
Ping? |
|
Looks good. Thanks ! |
This will add support for a new URI format udp://ip_address:remote_port[:local_port] that will allow udp communication with a single peer.