Problem: address parsing code is tied to the TCP code

[simias]

Solution: Factor the code into a different file with a well defined API and add
unit tests.

My Debian doesn't have a recent-enough clang-format to run make clang-format-check so apologies in advance if I messed up the coding style here and there (which is almost certain given the size of the patch).

I've attempted to match the previous implementation perfectly, including the arguable "quirks" such as the ones I've reported in #3069 (and a few others that I'm not sure about, such as specifying a %scope for an IPv4 address).

This is a pretty huge patch so I tried to be very comprehensive in the unit tests. I think the only path of the code that's not tested is when specifying an interface name as a bind point since the implementations to find the IP address associated with a network interface vary wildly from platform to platform so it's not trivial to come up with generic test code. I've tested manually that it still worked on Linux at least and I haven't made significant modifications to the architecture-specific code so I have good hopes that I haven't irremediably broken it.

[bluca]

On Debian 9 you can install the clang format tool by itself from stretch-backports:

sudo apt install -t stretch-backports clang-format-5.0

And then pass CLANG_FORMAT=clang-format-5.0 to ./configure to use it

[bluca]

@simias this is great work, thank you very much.
You need to add the new source and header in Makefile.am, right at the top, to fix the autotools build.

[bluca]

With some versions of g++ also there's another error:

/home/travis/build/zeromq/libzmq/unittests/unittest_ip_resolver_dns.cpp: In function ‘void freeaddrinfo(addrinfo*)’:
/home/travis/build/zeromq/libzmq/unittests/unittest_ip_resolver_dns.cpp:125:45: error: declaration of ‘void freeaddrinfo(addrinfo*)’ has a different exception specifier
     void freeaddrinfo (struct addrinfo *res_)
                                             ^
In file included from /home/travis/build/zeromq/libzmq/unittests/unittest_ip_resolver_dns.cpp:26:0:
/usr/include/netdb.h:668:13: error: from previous declaration ‘void freeaddrinfo(addrinfo*) throw ()’
 extern void freeaddrinfo (struct addrinfo *__ai) __THROW;

[bluca]

@simias the header too

[simias]

Ah, that's annoying. I "cheated" by putting the mock implementation in the C++ file in a bit extern "C" to avoid having to mess with how the autotools/cmake link the tests but I suppose that won't work if g++ is capricious. I guess I should put it in an external C file and do it right.

[simias]

Do you prefer that I keep squashing my single commit or do you prefer that I add additionals fixup commits for the time being? I'm not too used to code reviews on github so I don't know what's more convenient.

[bluca]

Please squash them, given they are fixing the first commit

Have you tried adding the __THROW to the mock as well?

[simias]

Do you think that'll work with all compilers? is __THROW standard? I assumed it was "dialectal" but I'm not exactly up to date with the latest C++ standards.

[bluca]

@simias for automake you also need an explicit section like these ones, just copy and paste
that's because each test is a program

[bluca]

this will break Windows XP, you'll want a function that does something like:

#if defined(ZMQ_HAVE_WINDOWS) && (_WIN32_WINNT < 0x0600)
    ip4addr.sin_addr.s_addr = inet_addr ("127.0.0.1");
#else
    inet_pton (AF_INET, "127.0.0.1", &ip4addr.sin_addr);
#endif

[bluca]

Windows:

C:\projects\libzmq\unittests\unittest_ip_resolver_dns.cpp(25): fatal error C1083: Cannot open include file: 'sys/socket.h': No such file or directory
C:\projects\libzmq\unittests\unittest_ip_resolver.cpp(33): fatal error C1083: Cannot open include file: 'net/if.h': No such file or director

[bluca]

The system headers are already included by testutil.hpp with the appropriate ifdefs, add them there if there's extra ones needed

[bluca]

The appveyor job fell off the pr, not sure what happened, but it fails with a new error:

C:\projects\libzmq\unittests\unittest_ip_resolver.cpp(36): error C2556: 'unsigned int if_nametoindex(const char *)': overloaded function differs only by return type from 'NET_IFINDEX if_nametoindex(PCSTR)' [C:\projects\build_libzmq\unittests\unittest_ip_resolver.vcxproj]
  C:\Program Files (x86)\Windows Kits\8.1\Include\shared\netioapi.h(2526): note: see declaration of 'if_nametoindex'
C:\projects\libzmq\unittests\unittest_ip_resolver.cpp(35): error C2373: 'if_nametoindex': redefinition; different type modifiers [C:\projects\build_libzmq\unittests\unittest_ip_resolver.vcxproj]
  C:\Program Files (x86)\Windows Kits\8.1\Include\shared\netioapi.h(2526): note: see declaration of 'if_nametoindex'

https://ci.appveyor.com/project/zeromq/libzmq/build/job/tw9mnlgjut7a07bk

[bluca]

Various Linux builds from very old to very new and various architectures all look good:

https://build.opensuse.org/package/show/home:bluca:branches:network:messaging:zeromq:git-stable/libzmq

[simias]

I'm not quite sure I understand the Windows build error there, is it a prototype mismatch or something else? I guess I should try to setup a Windows build environment and see for myself.

[bluca]

From the looks of it, it would seem there is a prototype mismatch - I have no idea how this translates to, maybe @sigiesec can help:

NET_IFINDEX WINAPI if_nametoindex(
  _In_ PCSTR InterfaceName
);

https://msdn.microsoft.com/en-us/library/windows/desktop/bb408409(v=vs.85).aspx

[bluca]

There's a few more:

C:\projects\libzmq\unittests\unittest_ip_resolver_dns.cpp(43): error C2373: 'getaddrinfo' : redefinition; different type modifiers [C:\projects\build_libzmq\unittests\unittest_ip_resolver_dns.vcxproj]
          C:\Program Files (x86)\Windows Kits\8.1\Include\um\ws2tcpip.h(108) : see declaration of 'getaddrinfo'
C:\projects\libzmq\unittests\unittest_ip_resolver_dns.cpp(43): error C2491: 'getaddrinfo' : definition of dllimport function not allowed [C:\projects\build_libzmq\unittests\unittest_ip_resolver_dns.vcxproj]
C:\projects\libzmq\unittests\unittest_ip_resolver_dns.cpp(49): error C2065: 'EAI_ADDRFAMILY' : undeclared identifier [C:\projects\build_libzmq\unittests\unittest_ip_resolver_dns.vcxproj]
C:\projects\libzmq\unittests\unittest_ip_resolver_dns.cpp(56): warning C4800: 'int' : forcing value to bool 'true' or 'false' (performance warning) [C:\projects\build_libzmq\unittests\unittest_ip_resolver_dns.vcxproj]
C:\projects\libzmq\unittests\unittest_ip_resolver_dns.cpp(127): error C2373: 'freeaddrinfo' : redefinition; different type modifiers [C:\projects\build_libzmq\unittests\unittest_ip_resolver_dns.vcxproj]
          C:\Program Files (x86)\Windows Kits\8.1\Include\um\ws2tcpip.h(386) : see declaration of 'freeaddrinfo'
C:\projects\libzmq\unittests\unittest_ip_resolver_dns.cpp(127): error C2491: 'freeaddrinfo' : definition of dllimport function not allowed [C:\projects\build_libzmq\unittests\unittest_ip_resolver_dns.vcxproj]

[sigiesec]

I can give the Windows build a look tomorrow morning.

[simias]

I've refactored the tests to have only one case per test and I've rewritten if_nametoindex to use the virtual interface trick.

[bluca]

Great, looks good - except now windows has something else to complain about:

[C:\projects\build_libzmq\unittests\unittest_ip_resolver.vcxproj]
unity.lib(unity.obj) : error LNK2019: unresolved external symbol _setUp referenced in function _UnityDefaultTestRun [C:\projects\build_libzmq\unittests\unittest_ip_resolver.vcxproj]
unity.lib(unity.obj) : error LNK2019: unresolved external symbol _tearDown referenced in function _UnityDefaultTestRun [C:\projects\build_libzmq\unittests\unittest_ip_resolver.vcxproj]
C:\projects\build_libzmq\bin\Release\unittest_ip_resolver.exe : fatal error LNK1120: 2 unresolved externals [C:\projects\build_libzmq\unittests\unittest_ip_resolver.vcxproj]

[sigiesec]

Just add

void setUp ()
{
}

void tearDown ()
{
}

to unittest_ip_resolver.cpp

This is also some issue with missing weak linking support. Probably this could be overcome more elegantly, but this is the solution that is currently used in some other tests.

[simias]

Sorry for the multiple pushes, I messed up.

[sigiesec]

Hopefully this is only a minor remaining issue, and the tests not only build but also run successfully under Windows :) If not, I can give it a closer look again.

Apart from that: The PR looks really good to me now :) Great job, it's an important step in increasing unit test coverage in libzmq!

[bluca]

Hopefully this is only a minor remaining issue, and the tests not only build but also run successfully under Windows :) If not, I can give it a closer look again.

They build now which is great, unfortunately there are a few test failures now :-/

Apart from that: The PR looks really good to me now :) Great job, it's an important step in increasing unit test coverage in libzmq!

Ditto - thanks for this great work

[simias]

Looks like the calls to getaddrinfo all fail for some reason. That's very odd.

[simias]

And it seems to work on Travis. Very strange indeed. Even test_parse_ipv4_simple fails on Windows and that's just:

test_resolve (resolver_opts, "1.2.128.129", "1.2.128.129");

My guess is either that I have some subtle memory corruption issue in the code that breaks the code or there's something different on Windows with regards to this API. At any rate I can't seem to reproduce it on Linux, even by tweaking the compilation flags.

[sigiesec]

@simias I try running it on Windows on my local machine.

[sigiesec]

@simias zmq::initialize_network must be called on Windows, see sigiesec@1595345 (this has quite a lot of changes since your file was not formatted with clang-format, but essentially only the calls to initialize_network and shutdown_network were added).

Now, only 4 test cases fail, which seem to be related to ipv6 handling, first failure is https://ci.appveyor.com/project/sigiesec/libzmq/build/build-488/job/abgf6r84i26q2t3h#L1458

[sigiesec]

Ah, this is due to this piece of code:
https://github.com/sigiesec/libzmq/blob/159534544b838989ba066633fed85398fd65e60c/src/ip_resolver.cpp#L279

Maybe to get uniform behaviour, within the if block, the resulting address could be manually converted into a mapped IPV6 address?

[simias]

this has quite a lot of changes since your file was not formatted with clang-format,

About that: "make clang-format-diff" does nothing on my machine for a reason I don't quite understand, it seemed suspicious given the amount of changes I've made but I thought I was just very lucky. I should've known better:

Formatting with clang-format (using clang-format) and showing differences with latest commit
Built target clang-format-diff

Regarding the new test error it seems to be that windows returns an AF_INET instead of an embedded IPV6 when asked to parse a numeric IPv4 IP:

test_parse_ipv4_in_ipv6:FAIL: Expected 23 Was 2

[simias]

Ah, good catch. I was trying to figure out what was wrong with address mapping on Windows, I should've read the code. I guess I will tweak the testing code to work in this scenario.

[bluca]

About clang, the CI doesn't show anything either, so not sure what's going on?

[simias]

Looks like window's getaddrinfo accepts brackets around IPv6 when Linux doesn't. Oh well. I'll just remove that test then, it's useless.

[simias]

Since clang-format still refuses to do its job on my end I fear you'll have to do it yourself (or let me know how to fix it).

[simias]

Actually I could tweak the parsing code so that brackets are accepted on Linux as well. I just need to add a copy of the bracket stripping code after the scope parsing code. This way you won't have the issue of some URLs being accepted on windows but not on linux. I can make a PR for that later if you want.

[sigiesec]

About that: "make clang-format-diff" does nothing on my machine for a reason I don't quite understand, it seemed suspicious given the amount of changes I've made but I thought I was just very lucky. I should've known better:

Hm, this should probably produce an error instead.

Regarding the new test error it seems to be that windows returns an AF_INET instead of an embedded IPV6 when asked to parse a numeric IPv4 IP:

Yes, as I wrote in my comment before this is explicitly done on Windows at this point:
https://github.com/sigiesec/libzmq/blob/159534544b838989ba066633fed85398fd65e60c/src/ip_resolver.cpp#L279

[simias]

Yeah I read the comment when I was porting the code but I didn't realize that meant that the code would behave differently.

Anyway, only one build fails on appveyor now and it seems unrelated to my patch. Is this a known problem or did I break something?

https://ci.appveyor.com/project/zeromq/libzmq/build/build-2249/job/uaei3wy47y8p7ai7

68: test_curve_security_with_valid_credentials (large routing id)
70/72 Test #68: test_security_curve ..............   Passed    5.70 sec
71/72 Test #15: test_immediate ...................***Timeout  30.01 sec
72/72 Test #44: test_many_sockets ................***Timeout  30.52 sec
97% tests passed, 2 tests failed out of 72
Total Test time (real) =  39.12 sec
The following tests FAILED:
	 15 - test_immediate (Timeout)
	 44 - test_many_sockets (Timeout)
Errors while running CTest

[sigiesec]

@simias test_immediate and test_many_sockets are unfortunately failing for quite some time, this is not related to your changes!

[sigiesec]

From my point of view, this is now ready to merge.
@bluca Do you want to have a final look?

[bluca]

LGTM!

@simias thanks again. Given the amount of changes, please send another PR with 2 commits: one to add yourself to AUTHORS, and another one with a relicensing grant, see https://github.com/zeromq/libzmq/tree/master/RELICENSE for details

[simias]

Will do, thank you for your help @bluca and @sigiesec !