Re: ntdll/tests: Add tests for RtlIpv4StringToAddressEx, RtlIpv6StringToAddress (resend)

[Stefan Dösinger]

-----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-----

ntdll/tests: Add tests for RtlIpv4StringToAddressEx, RtlIpv6StringToAddress (resend)

[Mark Jansen]

Hey,

Thanks for the review :)

On Sun, Feb 15, 2015 at 10:43 AM, Stefan Dösinger
 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-----

ntdll/tests: Add tests for RtlIpv4StringToAddressEx, RtlIpv6StringToAddress (resend)

[Stefan Dösinger]

-----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-----

ntdll/tests: Add tests for RtlIpv4StringToAddressEx, RtlIpv6StringToAddress (resend)

[Dmitry Timoshkov]

Stefan Dösinger  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.