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