Re: [PATCH 2/3] ntdll/tests: Add tests for RtlIpv6StringToAddress (try 3)

[Stefan Dösinger]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi,

Thanks again for your work and patience! This goes in the right direction in my eyes. The full set of address tests are run against both versions, and it documents the differences between non-ex and ex better. Can you do the same with the ipv4 tests in patch 1?

Am 2015-02-19 um 23:38 schrieb Mark Jansen:
> +/* ipv6 addresses from https://github.com/beaugunderson/javascript-ipv6/tree/master/test/data */
> +static const struct
> +{
> +    PCSTR address;
> +    NTSTATUS res;
> +    int terminator_offset;
> +    int ip[8];
> +    int broken;
> What is the meaning of broken here? Different behavior among different Windows versions? Or deviation from your source above?
> +    int ex_fails;   /* 1 means ex should fail, 2 means skip for ex */
> Style hint: You can use an enum in this case.

I guess you need the skip value because the last test fails on non-ex and succeeds on ex and your expected address does not match. Similarly, the behavior that the function writes to the output address even in case of a failure isn't exactly helpful.

- From patch 3:
> +        if (ipv6_tests[i].ex_fails == 2)    /* testing vs ex doesnt >make sense. */
> +            continue;
> Please document why, it took me a while to come up with the guess above. (And I'm not even sure it's the real reason). You're also not checking the output IP for the IP tests from the non-ex tests here. Is there a reason for that? Are there any cases where -Ex returns a different IP than non-ex?
> +        if (ipv6_tests[i].ip[0] == -1)
> +        {
> +            for (j = 0; j < 8; ++j)
> +                expected_ip.s6_words[j] = 0xabab;
> +        }
> I guess you want to avoid writing 0xabab 8 times in the table. I don't have any objection, but I can't predict if Alexandre likes it - technically this is redundant code. I'd say keep it for now, if he doesn't like it he will tell you.
> +            for (j = 0; j < 8; ++j)
> +                expected_ip.s6_words[j] = ipv6_tests[i].ip[j];
> Would this do the right thing on big endian machines?

About some individual tests:
> +    { "1111:2222:3333:4444:5555:6666:7777::",           STATUS_SUCCESS,             34, { 0x1111, 0x2222, 0x3333, 0x4444, 0x5555, 0x6666, 0x7777, 0 }, 1 },
> Now that's an interesting corner case. Out of curiosity, what does "1111:2222:3333:4444:5555:6666:7777:" do in non-ex and ex (one : removed)?
> +    { "::0:0:0:0:0:0:0",                                STATUS_SUCCESS,             13, { 0, 0, 0, 0, 0, 0, 0, 0 }, 0, 1 },
> +    { "::0:a:b:c:d:e:f",                                STATUS_SUCCESS,             13, { 0, 0, 0, 0xa00, 0xb00, 0xc00, 0xd00, 0xe00 }, 0, 1 },
> Here it would be interesting to know what IP -Ex writes before it fails. I don't see why -Ex would fail if -Ex calls non-ex, given that the terminator points way beyond the problematic "::" at the start.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBAgAGBQJU5xDbAAoJEN0/YqbEcdMwmEIP/0ti3aPWMeI8yetig2eTIuAG
2QhNad7bgFgioJBremjKPDuDJViYhkv4XoIwie6bRFSF9VnEECmiv2wObhRz2e0E
MgK1bGkZc0BXs4TiVoa/+JVxWgo3ddWn1Wpj+hyVRAvyEfj4NNfRWvcGaQ+cZmwu
Sd0tx7BqCQLSPnNwyrl53nqVWjkkmARdSF2pLDMgXkwtGXlR2b2irYslYg+Sqgvb
+ORAryKZvVLt8vL6wBclb8ruFWpro7/Rio6eZb8EfoRNH+8YdYWUrdfcoCE4Ui0i
W2xsOnahZuPvuQR9id8zp4etIa+YrlGlVsIfHZrzDzCvbyuBlq+niSeWZxgNwMs+
IUpHoHj0TYNUZ9A7PB16UPHowsPVE5GYvsdjFr4Rj8rHxQqvftpTwFxRqWuV4qkQ
Qx+achRcfA/qXWWYHksENuQJxF29vIbgiLbpvW256A6zmsAvB/+LlqndeF+BuW4I
3rQ2H7ADBAMx8IscePSDNLAKd2vsYWj8gFlw7y10HnIE7/zedqtWPIaGocFnS0u5
QR/ETfUW5uvOr4rqMcyMCxg3yhc/QvYV67LFMrLusJ1IlCBhcfAC4r2B91sECHXr
fJvdfGwL6HBt3tAJ5zxXZjTEwiFc77TGxswbt4lQplhFqTHfi/ZovGUiS/W3nQl6
vX0zFtoYLTRAK0KtCNiP
=JRkZ
-----END PGP SIGNATURE-----

[PATCH 2/3] ntdll/tests: Add tests for RtlIpv6StringToAddress (try 3)

[Stefan Dösinger]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Am 2015-02-20 um 11:47 schrieb Stefan Dösinger:
> Hi,
>
> Thanks again for your work and patience! This goes in the right
> direction in my eyes. The full set of address tests are run against
> both versions, and it documents the differences between non-ex and
> ex better. Can you do the same with the ipv4 tests in patch 1?
> If you want you can move the ipv4 test to the end of the patch series
> and focus on the ipv6 tests first - that way we can see if we find any
> more corner cases before you do all the refactoring work on the ipv4
> test data.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBAgAGBQJU5xPjAAoJEN0/YqbEcdMwFL4P/0881Lo0n/fCEFV7oZm1oG78
wAiQPuHxVKQT6jyTifqfIt14PNUoBc5Y9GpRwIUjkhZ+8DJgyTg9ceCa3Wzxqs+n
gy4Nac59LmDKALM7CHnYixF4rNc509ompZSMBH9WVWvlTtyYW6m+qXDbs+uSKYpz
fsbZRI5vSP9/8fgyuVZOx7Ngt91C6S7mKn6Ub0dzfjwwDnV3Tv6h1lqLaJ3N4cuQ
ogMbAuWSHzikU3LXe+3vIlqNPgfcwe8Eo9jM72ad/35ElO7y9rQZr6k+V2DPl4RN
yk55zvXn2nzlJwQHZyIdhRIp/odeDFQXEyj1u5lJHEclDuqZYhtBppbTdBtFiVbd
Kfu9To+d9feTzOMdKB58p2VYkjE8Mz86np35OMkp2+r6ytQK8EcCkkYZ51KhW0EQ
kVVkmxLcmG/5YtuxQ4fnRMayc1m4cVoLy8FNRYjpvHffm/wtEuQtHDfF9jDTVwOz
O8DRHX/96DJbE5Pbp3OXWPk6/Iq5l7wYIBXpDn1ENwwYG0vCb2/kj8JGoQ4OfLjf
jUJ/B80W6NCjoHkaWJcNHnw/oPAPHc0H/UU/sucTCfGJw9gmx9s9wSaCLx9LFse3
vmTM7oUFgVUr6fLHxY/Uhs9VCDu94Vumbx6N7CU3VbZSmeBs5RYLie4gi/ZywkYS
M2QugqvFbxmEWY8bNc2q
=/GAT
-----END PGP SIGNATURE-----

[PATCH 2/3] ntdll/tests: Add tests for RtlIpv6StringToAddress (try 3)

[Mark Jansen]

Hey,

I do not have the intention to rewrite the ipv4 tests now, i do have
another few tests lined up for different ipv6 functions (addr2str),
i'll look into rewriting the ipv4 then.
As for now, few tests are better than no tests, right?

On Fri, Feb 20, 2015 at 11:47 AM, Stefan Dösinger
<stefandoesinger(a)gmail.com> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Hi,
>
> Thanks again for your work and patience! This goes in the right direction in my eyes. The full set of address tests are run against both versions, and it documents the differences between non-ex and ex better. Can you do the same with the ipv4 tests in patch 1?
>
> Am 2015-02-19 um 23:38 schrieb Mark Jansen:
> > +/* ipv6 addresses from https://github.com/beaugunderson/javascript-ipv6/tree/master/test/data */
> > +static const struct
> > +{
> > +    PCSTR address;
> > +    NTSTATUS res;
> > +    int terminator_offset;
> > +    int ip[8];
> > +    int broken;
> > What is the meaning of broken here? Different behavior among different Windows versions? Or deviation from your source above?
> > Broken means xp pro, 2003, vista and 2008 did not behave correct.
> > +    int ex_fails;   /* 1 means ex should fail, 2 means skip for ex */
> > Style hint: You can use an enum in this case.

Yeah, was a last minute change to add the 2.
> I guess you need the skip value because the last test fails on non-ex and succeeds on ex and your expected address does not match. Similarly, the behavior that the function writes to the output address even in case of a failure isn't exactly helpful.
>
> - From patch 3:
> > +        if (ipv6_tests[i].ex_fails == 2)    /* testing vs ex doesnt >make sense. */
> > +            continue;
> > Please document why, it took me a while to come up with the guess above. (And I'm not even sure it's the real reason). You're also not checking the output IP for the IP tests from the non-ex tests here. Is there a reason for that? Are there any cases where -Ex returns a different IP than non-ex?

Because the function will succeed on ex, it is even in the ex-test table.
Cases like this are exactly why i didnt want it in one table in the first place.
> > +        if (ipv6_tests[i].ip[0] == -1)
> > +        {
> > +            for (j = 0; j < 8; ++j)
> > +                expected_ip.s6_words[j] = 0xabab;
> > +        }
> > I guess you want to avoid writing 0xabab 8 times in the table. I don't have any objection, but I can't predict if Alexandre likes it - technically this is redundant code. I'd say keep it for now, if he doesn't like it he will tell you.

Indeed.
> > +            for (j = 0; j < 8; ++j)
> > +                expected_ip.s6_words[j] = ipv6_tests[i].ip[j];
> > Would this do the right thing on big endian machines?

Probably not, but I was under the impression that windows doesnt run
on big endian machines.
If wine does run on big endian this might need an additional check,
however i do not have a b.e. machine to verify it on.
> About some individual tests:
> > +    { "1111:2222:3333:4444:5555:6666:7777::",           STATUS_SUCCESS,             34, { 0x1111, 0x2222, 0x3333, 0x4444, 0x5555, 0x6666, 0x7777, 0 }, 1 },
> > Now that's an interesting corner case. Out of curiosity, what does "1111:2222:3333:4444:5555:6666:7777:" do in non-ex and ex (one : removed)?
> > +    { "::0:0:0:0:0:0:0",                                STATUS_SUCCESS,             13, { 0, 0, 0, 0, 0, 0, 0, 0 }, 0, 1 },
> > +    { "::0:a:b:c:d:e:f",                                STATUS_SUCCESS,             13, { 0, 0, 0, 0xa00, 0xb00, 0xc00, 0xd00, 0xe00 }, 0, 1 },
> > Here it would be interesting to know what IP -Ex writes before it fails. I don't see why -Ex would fail if -Ex calls non-ex, given that the terminator points way beyond the problematic "::" at the start.

Ex doesnt write anything if it fails.
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v2
>
> iQIcBAEBAgAGBQJU5xDbAAoJEN0/YqbEcdMwmEIP/0ti3aPWMeI8yetig2eTIuAG
> 2QhNad7bgFgioJBremjKPDuDJViYhkv4XoIwie6bRFSF9VnEECmiv2wObhRz2e0E
> MgK1bGkZc0BXs4TiVoa/+JVxWgo3ddWn1Wpj+hyVRAvyEfj4NNfRWvcGaQ+cZmwu
> Sd0tx7BqCQLSPnNwyrl53nqVWjkkmARdSF2pLDMgXkwtGXlR2b2irYslYg+Sqgvb
> +ORAryKZvVLt8vL6wBclb8ruFWpro7/Rio6eZb8EfoRNH+8YdYWUrdfcoCE4Ui0i
> W2xsOnahZuPvuQR9id8zp4etIa+YrlGlVsIfHZrzDzCvbyuBlq+niSeWZxgNwMs+
> IUpHoHj0TYNUZ9A7PB16UPHowsPVE5GYvsdjFr4Rj8rHxQqvftpTwFxRqWuV4qkQ
> Qx+achRcfA/qXWWYHksENuQJxF29vIbgiLbpvW256A6zmsAvB/+LlqndeF+BuW4I
> 3rQ2H7ADBAMx8IscePSDNLAKd2vsYWj8gFlw7y10HnIE7/zedqtWPIaGocFnS0u5
> QR/ETfUW5uvOr4rqMcyMCxg3yhc/QvYV67LFMrLusJ1IlCBhcfAC4r2B91sECHXr
> fJvdfGwL6HBt3tAJ5zxXZjTEwiFc77TGxswbt4lQplhFqTHfi/ZovGUiS/W3nQl6
> vX0zFtoYLTRAK0KtCNiP
> =JRkZ
> -----END PGP SIGNATURE-----

[PATCH 2/3] ntdll/tests: Add tests for RtlIpv6StringToAddress (try 3)

[Stefan Dösinger]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Am 2015-02-20 um 16:07 schrieb Mark Jansen:
> Hey,
>
> I do not have the intention to rewrite the ipv4 tests now, i do have
> another few tests lined up for different ipv6 functions (addr2str),
> i'll look into rewriting the ipv4 then.
> As for now, few tests are better than no tests, right?
> That's OK with me. I recommend to move the ipv4 test to the end of the patch series in this case. That way Alexandre can apply the ipv6 patch if he disagrees with the partial ipv4 test.
> Broken means xp pro, 2003, vista and 2008 did not behave correct.
> What behaves correctly? Windows 7?
> > > +        if (ipv6_tests[i].ex_fails == 2)    /* testing vs ex doesnt >make sense. */
> > > +            continue;
> > > Please document why, it took me a while to come up with the guess above. (And I'm not even sure it's the real reason). You're also not checking the output IP for the IP tests from the non-ex tests here. Is there a reason for that? Are there any cases where -Ex returns a different IP than non-ex?
> > > Because the function will succeed on ex, it is even in the ex-test table.
> > > Cases like this are exactly why i didnt want it in one table in the first place.
> > > There's no problem with running a test or two twice in a situation like this. The reason why you need special handling for this test is that it is the only test that fails on base and succeeds on ex. Your way of handling this works reasonably well, it's just that the comment could be better. E.g. something like "Fails on base, succeeds on -Ex. Success case tested by -Ex test table." would be a better explanation why the test is skipped.

Alternatively you can remove the test from the base table and test it with a separate function call in the base tests.
> Probably not, but I was under the impression that windows doesnt run
> on big endian machines.
> If wine does run on big endian this might need an additional check,
> however i do not have a b.e. machine to verify it on.
> Yeah, it is a fairly academic question. Even Windows NT on Power was little endian. I vaguely remembered that there were helper functions for manipulating IP addresses that take care of endianess. I didn't find any though, just the hostname resolving string-to-IP ones.
> Ex doesnt write anything if it fails.
> This line from the -ex table suggests something else:
> +        { "[3ffe:2a00:100:7031::1].8080",                   STATUS_INVALID_PARAMETER,   0xbadf00d,  0xbeef, { 0xfe3f, 0x2a, 1, 0x3170, 0, 0, 0, 0x100 } },

This has a working address and broken port though. Things may be different if the address parsing fails. Please add an ip == 0xabab comparison in the part of the -ex test that runs the non-ex / IP address checks, and check the returned address in case the function succeeds.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBAgAGBQJU6vHGAAoJEN0/YqbEcdMwKnYP/A7urRWmo4/qu8xtXtXLYCoU
QThhth9J4gMRFQUazH1JaocyeXcLeZ8fXSKSgV+2TM3b7qZIjJVljlNR6ySPJheS
SMZ9dAmrngWZj418SMwqAYjyMeqNWDcn+/zRqr39AIgSh5J6K0YyV1XwzsJRUuV+
C7sEGzlCupiS1N5V0xB5OggE7E2cIFItbHjNhTPmO2YVht8fvf/8Z0CnlVtDPswp
H6o7TgNqhCuZGD5K0sU+XHwMxN/bn8wJkG85OKA20CgS0nJPTPb8BkHaIGClx3dF
zXNxS32fEEa3S1JlsRSb66c/0Whi3GyyrrKhN2aNKZI8t5tS5GzWPG4vwtVyDZnh
7BSzAl5TQDiLWLjFc5skPpNU3rvm8Vg13WYa1ahnw33hL23U52MJZZYQS84CoiVp
rbeUqLWbu4tiTYsUgmfcvnKNfFOXLF1Pzd4fGEW5kI54t5NRvSWmWKqnsvglLYjJ
Tp8juavhqMjv1WjZdz6N2VURKhR5mg44pAL9V0cbFcPsKikSSKXBAEwSAmwu1WN7
XYPPrQhlhmxGk9DUBOzM3V/gwNqm1z9K6ou7MJQrpudTFleWXn6gUOIZkX+lHaPl
eHjMV5wXueIIrVFuGaEKcF+bVBdaziOEzu0XSUHCV2eD1vg5c0bN09YxkahxNm8w
mfzirl4vY77ymgYPrWqb
=wzpF
-----END PGP SIGNATURE-----