Udp parser #3075
Udp parser #3075
Conversation
| int rc = addr.resolve (name_, bound); | ||
|
|
||
| if (dest_addr_ == NULL) { | ||
| TEST_ASSERT_EQUAL (-1, rc); |
There was a problem hiding this comment.
Maybe check for a specific errno here?
unittests/unittest_udp_address.cpp
Outdated
| @@ -111,7 +109,9 @@ static void test_resolve_ipv4_connect_any () | |||
|
|
|||
| static void test_resolve_ipv4_connect_anyport () | |||
| { | |||
| test_resolve ("127.0.0.1:*", NULL); | |||
| // Not sure if that's useful but the TCP code allows it so why | |||
There was a problem hiding this comment.
Hm. Does this have a defined behaviour on any platform? If not, it should be disallowed IMO.
The same applies to TCP.
Feel free to move it. The reason the resolution is async is that first of all it might block, and secondly the remote endpoint might not be reachable or resolvable until much later. This is a feature of libzmq. |
|
@bluca if resolution can potentially happen much later maybe it's not a good idea to remove this code? Otherwise it will change the current behavior where you get an early error in case of an invalid URL. The port test could and probably should be added to the ip_resolver anyway, for good measure. |
|
Right, I thought by moving you mean refactoring into a function that is still called from the same place so that it can be reused. |
|
Currently the UDP code doesn't delay the resolution (should it?) so there wouldn't be anything to reuse really. I'm about to make a PR to add the test to ip_resolver anyway, as it probably makes sense to have it here at any rate. |
I think it should, yes, to get feature parity with TCP - but don't consider that blocking for the stuff you are working on. Maybe we can open an issue. |
Solution: add unit tests
Solution: replace it with the ip_resolver code shared with the TCP address code It simplifies the UDP parsing code and makes it behave more like the TCP counterpart, in particular it's not possible to connect to hosts by name and bind by NIC names. It also adds support for "*" port resolving to 0 (useful to let the OS allocate the port number).
|
(Sorry for the double push) Rebased the code to remove support for wildcard port on connect, added check for errno in unit tests. |
This code contains two independent albeit related commits:
The first adds unit tests for the
udp_addresscodeThe 2nd replaces the current ad hoc parser with the new
ip_resolvercodeLet me know if you prefer to have two different PRs.
The new resolver code should not break any existing URLs, however it will allow things that didn't work previously such as connecting hostnames (
udp://zeromq.org:5555), binding NIC names (udp://eth0:1234) and wildcard ports (udp://127.0.0.1:*).