Re: ntdll/tests: Add tests for RtlIpv4StringToAddressEx, RtlIpv6StringToAddress (resend)
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Hi, Please split the patch into one patch per tested function (ipv4ex, ipv6, ipv6ex). Am 2015-02-13 um 22:35 schrieb Mark Jansen:
+ struct + { + PCSTR address; + NTSTATUS res; + int ip[4]; + USHORT port; + } tests[] = You can make this const (or even static const, not much difference though).
+ { "1.1.1.1:", STATUS_INVALID_PARAMETER, { 1, 1, 1, 1 }, 0xdead }, + { "1.1.1.1+", STATUS_INVALID_PARAMETER, { 1, 1, 1, 1 }, 0xdead }, + { "1.1.1.1:1", STATUS_SUCCESS, { 1, 1, 1, 1 }, 0x100 }, I recommend to add a few more tests: "1.1.1.1" - without a port "256.1.1.1:1" - out of range ip "-1.1.1.1:1" - negative ip "1.1.0.1" - There's a zero in it, should still be valid though. Hex and octal IPs
+ const int testcount = sizeof(tests) / sizeof(tests[0]); + int i, Strict; Technically i and testcount can be made unsigned. According to your function definition earlier in the file Strict should be a BOOLEAN. For consistency "Strict" should be renamed to lower case "strict".
+ ok(res == STATUS_INVALID_PARAMETER, + "[null address] res = 0x%08x, expected 0x%08x\n", + res, STATUS_INVALID_PARAMETER); ... + ok(res == tests[i].res, + "[%s] res = 0x%08x, expected 0x%08x\n", + tests[i].address, res, tests[i].res); Indentation inconsistency in the line continuation.
+ expected_ip.S_un.S_un_b.s_b1 = tests[i].ip[0]; + expected_ip.S_un.S_un_b.s_b2 = tests[i].ip[1]; + expected_ip.S_un.S_un_b.s_b3 = tests[i].ip[2]; + expected_ip.S_un.S_un_b.s_b4 = tests[i].ip[3]; Is there a reason why you didn't just put a IN_ADDR in the test structure? You'll probably need an extra curly brackets around each member of struct S_un_b and one for the union: {{1}, {1}, {1}, {1}}
Most of this applies in the same fashion to the ipv6 tests.
+/* ipv6 addresses from https://github.com/beaugunderson/javascript-ipv6/tree/master/test/data */ This is licensed under MIT. IANAL, but I think it is ok to transfer to an LGPLv2 licensed project.
+static void test_RtlIpv6StringToAddress(void) ... + struct + { + PCSTR address; + NTSTATUS res; + int terminator_offset; + int ip[8]; + } tests[] = ... + struct + { + PCSTR address; + NTSTATUS res; + ULONG scope; + USHORT port; + int ip[8]; + } tests[] = Did you try to share the data for the ipv6 and ipv6ex test?
Unfortunately my ipv6-foo is too limited to say much about the tests in the table :-( .
+ /* sanity check */ + ok(sizeof(ip._S6_un) == (sizeof(USHORT)* 8), "sizeof(ip._S6_un)\n"); I think we have compile-time asserts for things like this, but I don't know how they work.
+ res = pRtlIpv6StringToAddressExA("::", &ip, &scope, &port); + ok(res == STATUS_SUCCESS, "[validate] res = 0x%08x, expected STATUS_SUCCESS\n", res); + res = pRtlIpv6StringToAddressExA(NULL, &ip, &scope, &port); + ok(res == STATUS_INVALID_PARAMETER, "[null string] res = 0x%08x, expected STATUS_INVALID_PARAMETER\n", res); + res = pRtlIpv6StringToAddressExA("::", NULL, &scope, &port); + ok(res == STATUS_INVALID_PARAMETER, "[null result] res = 0x%08x, expected STATUS_INVALID_PARAMETER\n", res); + res = pRtlIpv6StringToAddressExA("::", &ip, NULL, &port); + ok(res == STATUS_INVALID_PARAMETER, "[null scope] res = 0x%08x, expected STATUS_INVALID_PARAMETER\n", res); + res = pRtlIpv6StringToAddressExA("::", &ip, &scope, NULL); + ok(res == STATUS_INVALID_PARAMETER, "[null port] res = 0x%08x, expected STATUS_INVALID_PARAMETER\n", res); Checking that the output variables remain unmodified in the ipv4 test was a good idea IMO. Please add it here as well. (It is somewhat redundant because the test table has invalid tests and checks if the output is untouched, but this wouldn't be the first place where we find strange error handling behavior...
-----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBAgAGBQJU4Go3AAoJEN0/YqbEcdMwP/wP/j5pbxPuTt4dBpr/fxA/E/rv H6+hoXqq3irBQWgIuU4sprfvR/VEcKYR7HnMvL8SX/aa7xvuFPSdZerjHGgpoz1R SKlqg46X8HxIe6BvODUSpi90OrJ4UNmAutL2vhpFQVzSsblZ1xPkk3GKHnIubslC fmUi9I91Vyk6Upvk4xI9FNyf7OddUcHkb+Xkw5qFCip/R4Mcad/lZ5lKq6pJWNhE 4VRIA19IlCshi0Z3CY1NM8ZdRCCg8Wi8MqP8jgBaHsR3yPfEoBa3qUjsB2ryhrlP 2ityxJSFgAl38OQ89kwwkL7KIxxrP7t8oDrjGpb76mPdnpWxRBNarmSSu+JAmDDg jUlzsjftCifjTs2vZloDWbqqhEBY3a/SQ2tUHeIxuYplKpR/AN75WH7WxFOKpR1h KgafrCMeCGi0s2dVymkqXNa8zluQ3nq/pC16nddelgXD4D3RMPwWBnEDY8bpspR7 n0SR2mMTDAt2mndeI8DXvSyyVE4umsl8sUCjWVpKE/lj2xKZjKyPWFVx8yv1omx+ EqF+RUA2Qld+HOCy7d2c00mPy0bNmthaj7T5tmQSjEVStmGgwSH0RytkrI1yqwU8 MyDotAIxZ3OAiHvMl6RHvq5pquWDM4FTJ+yMpdxiEKPIZ4dwMa5mkZvaCYWBB2Qv NnxWITxzj7GvV1YH7U/5 =uuX8 -----END PGP SIGNATURE-----
Hey, Thanks for the review :) On Sun, Feb 15, 2015 at 10:43 AM, Stefan Dösinger <stefandoesinger(a)gmail.com> wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Hi,
Please split the patch into one patch per tested function (ipv4ex, ipv6, ipv6ex).
Allright.
Am 2015-02-13 um 22:35 schrieb Mark Jansen:
+ struct + { + PCSTR address; + NTSTATUS res; + int ip[4]; + USHORT port; + } tests[] = You can make this const (or even static const, not much difference though).
Will do, I was just following the style of the RtlIpv4StringToAddressA test function.
+ { "1.1.1.1:", STATUS_INVALID_PARAMETER, { 1, 1, 1, 1 }, 0xdead }, + { "1.1.1.1+", STATUS_INVALID_PARAMETER, { 1, 1, 1, 1 }, 0xdead }, + { "1.1.1.1:1", STATUS_SUCCESS, { 1, 1, 1, 1 }, 0x100 }, I recommend to add a few more tests: "1.1.1.1" - without a port "256.1.1.1:1" - out of range ip "-1.1.1.1:1" - negative ip "1.1.0.1" - There's a zero in it, should still be valid though. Hex and octal IPs
There is already tests for that in the RtlIpv4StringToAddressA test, this testcase is for the port handling.
+ const int testcount = sizeof(tests) / sizeof(tests[0]); + int i, Strict; Technically i and testcount can be made unsigned. According to your function definition earlier in the file Strict should be a BOOLEAN. For consistency "Strict" should be renamed to lower case "strict".
Will do.
+ ok(res == STATUS_INVALID_PARAMETER, + "[null address] res = 0x%08x, expected 0x%08x\n", + res, STATUS_INVALID_PARAMETER); ... + ok(res == tests[i].res, + "[%s] res = 0x%08x, expected 0x%08x\n", + tests[i].address, res, tests[i].res); Indentation inconsistency in the line continuation.
+ expected_ip.S_un.S_un_b.s_b1 = tests[i].ip[0]; + expected_ip.S_un.S_un_b.s_b2 = tests[i].ip[1]; + expected_ip.S_un.S_un_b.s_b3 = tests[i].ip[2]; + expected_ip.S_un.S_un_b.s_b4 = tests[i].ip[3]; Is there a reason why you didn't just put a IN_ADDR in the test structure? You'll probably need an extra curly brackets around each member of struct S_un_b and one for the union: {{1}, {1}, {1}, {1}}
I did not do this because i followed the style of the RtlIpv4StringToAddressA function, and if i would have to guess the extra brackets are precisely the reason.
Most of this applies in the same fashion to the ipv6 tests.
+/* ipv6 addresses from https://github.com/beaugunderson/javascript-ipv6/tree/master/test/data */ This is licensed under MIT. IANAL, but I think it is ok to transfer to an LGPLv2 licensed project.
+static void test_RtlIpv6StringToAddress(void) ... + struct + { + PCSTR address; + NTSTATUS res; + int terminator_offset; + int ip[8]; + } tests[] = ... + struct + { + PCSTR address; + NTSTATUS res; + ULONG scope; + USHORT port; + int ip[8]; + } tests[] = Did you try to share the data for the ipv6 and ipv6ex test?
No i did not, mainly because the RtlIpv6AddressToString test function focuses on the Ip address, and the Ex test focuses on port / scopeid.
Unfortunately my ipv6-foo is too limited to say much about the tests in the table :-( .
+ /* sanity check */ + ok(sizeof(ip._S6_un) == (sizeof(USHORT)* 8), "sizeof(ip._S6_un)\n"); I think we have compile-time asserts for things like this, but I don't know how they work.
+ res = pRtlIpv6StringToAddressExA("::", &ip, &scope, &port); + ok(res == STATUS_SUCCESS, "[validate] res = 0x%08x, expected STATUS_SUCCESS\n", res); + res = pRtlIpv6StringToAddressExA(NULL, &ip, &scope, &port); + ok(res == STATUS_INVALID_PARAMETER, "[null string] res = 0x%08x, expected STATUS_INVALID_PARAMETER\n", res); + res = pRtlIpv6StringToAddressExA("::", NULL, &scope, &port); + ok(res == STATUS_INVALID_PARAMETER, "[null result] res = 0x%08x, expected STATUS_INVALID_PARAMETER\n", res); + res = pRtlIpv6StringToAddressExA("::", &ip, NULL, &port); + ok(res == STATUS_INVALID_PARAMETER, "[null scope] res = 0x%08x, expected STATUS_INVALID_PARAMETER\n", res); + res = pRtlIpv6StringToAddressExA("::", &ip, &scope, NULL); + ok(res == STATUS_INVALID_PARAMETER, "[null port] res = 0x%08x, expected STATUS_INVALID_PARAMETER\n", res); Checking that the output variables remain unmodified in the ipv4 test was a good idea IMO. Please add it here as well. (It is somewhat redundant because the test table has invalid tests and checks if the output is untouched, but this wouldn't be the first place where we find strange error handling behavior...
Will do.
-----BEGIN PGP SIGNATURE----- Version: GnuPG v2
iQIcBAEBAgAGBQJU4Go3AAoJEN0/YqbEcdMwP/wP/j5pbxPuTt4dBpr/fxA/E/rv H6+hoXqq3irBQWgIuU4sprfvR/VEcKYR7HnMvL8SX/aa7xvuFPSdZerjHGgpoz1R SKlqg46X8HxIe6BvODUSpi90OrJ4UNmAutL2vhpFQVzSsblZ1xPkk3GKHnIubslC fmUi9I91Vyk6Upvk4xI9FNyf7OddUcHkb+Xkw5qFCip/R4Mcad/lZ5lKq6pJWNhE 4VRIA19IlCshi0Z3CY1NM8ZdRCCg8Wi8MqP8jgBaHsR3yPfEoBa3qUjsB2ryhrlP 2ityxJSFgAl38OQ89kwwkL7KIxxrP7t8oDrjGpb76mPdnpWxRBNarmSSu+JAmDDg jUlzsjftCifjTs2vZloDWbqqhEBY3a/SQ2tUHeIxuYplKpR/AN75WH7WxFOKpR1h KgafrCMeCGi0s2dVymkqXNa8zluQ3nq/pC16nddelgXD4D3RMPwWBnEDY8bpspR7 n0SR2mMTDAt2mndeI8DXvSyyVE4umsl8sUCjWVpKE/lj2xKZjKyPWFVx8yv1omx+ EqF+RUA2Qld+HOCy7d2c00mPy0bNmthaj7T5tmQSjEVStmGgwSH0RytkrI1yqwU8 MyDotAIxZ3OAiHvMl6RHvq5pquWDM4FTJ+yMpdxiEKPIZ4dwMa5mkZvaCYWBB2Qv NnxWITxzj7GvV1YH7U/5 =uuX8 -----END PGP SIGNATURE-----
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Am 2015-02-15 um 15:50 schrieb Mark Jansen:
You can make this const (or even static const, not much difference though). Will do, I was just following the style of the RtlIpv4StringToAddressA test function. I recommend to change it to const there as well in a separate patch.
I recommend to add a few more tests: "1.1.1.1" - without a port "256.1.1.1:1" - out of range ip "-1.1.1.1:1" - negative ip "1.1.0.1" - There's a zero in it, should still be valid though. Hex and octal IPs There is already tests for that in the RtlIpv4StringToAddressA test, this testcase is for the port handling. My preference would be to run the full set of tests against both functions (as far as the functionality is there of course), since it's always possible that there is subtly different behavior.
Is there a reason why you didn't just put a IN_ADDR in the test structure? You'll probably need an extra curly brackets around each member of struct S_un_b and one for the union: {{1}, {1}, {1}, {1}} I did not do this because i followed the style of the RtlIpv4StringToAddressA function, and if i would have to guess the extra brackets are precisely the reason. The curly brackets are not really an issue. I suspect that the author of the previous test saw the compiler warning and did not know how to fix it, so he wrote a workaround.
(That said, I have not tried to change this myself. There may be unforseen problems.) If a direct initialization works the way I think it works sending a patch that changes the existing code would be appreciated. -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBAgAGBQJU4OpYAAoJEN0/YqbEcdMw0uoP/19T794mbuc4/xEKpLHQph97 qqz522GdH1ZqhKeo3krqO0bU3FB7Jy2XIHXcEv/lcj0ibuFADuRx+krhLmPj4Fq3 +6wQr0g6xyRL5BNMe4lwRwmZXs/tT7qiQR96L+fK5Rq5QClnjBUap9LoRgslTlA7 g8dd2bEum8lnyt6yEXkgb0pHKlEd5mQh+5tu42uB0iLSa7vyDFFK/XVxmu/Un8Jx fv8PUbDRoYeOp2Ukxozyrb1qs0Am+biD6zqY4Lji+zzlIXi9s9X/yDusxaGbz/LR JP0eWQYOj41jsFyRPrZRhq7UDJkkKuloaDrwFmallT6RlWQ+CnTkfhpeuR3p2ZUj An4vDqStaVJkGHGF8nMa223eLgb9/PMHPK9ohHETkFH2EzA75ohBIDAuyv2oq3tl 7TDjPLZZ/nRlROkygjRYXM8lsNOEVL3jkvmJnu1WdN0pTItclx8eMCk5TOiTe/ra 1n0dkhGnG9hAPSWSKuPzS8oGw09qk8Qqa9HNrfgR93G2UuIXWkzJJRhAkZK0bRXJ sCXux4p8QNC4QrzeyXza/ZEWfw6jFAVa5E1lCz372NEOMPhgTPeNWVns7xjitnuH rUTRf8WubUPiv/Z7lDo08/n5GW9MYrC4VFmJja/M3Np9dmJyltXu78SPJrE8DsDr FPXv42VRuCx0/VTCQVCk =ebeD -----END PGP SIGNATURE-----
Stefan Dösinger <stefandoesinger(a)gmail.com> wrote:
+ struct + { + PCSTR address; + NTSTATUS res; + int ip[4]; + USHORT port; + } tests[] = You can make this const (or even static const, not much difference though).
There is a difference, with 'static const' the compiler will place the data into a read-only section, simple 'const' still goes into a read-write one. -- Dmitry.
participants (3)
-
Dmitry Timoshkov -
Mark Jansen -
Stefan Dösinger