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

[Stefan Dösinger]

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

Hi,

I've already talked to Mark about this on #winehackers, just repeating
it for the list: My remaining request with these tests is that I
believe we should test the full set of inputs against the Ex and
non-Ex versions and ideally reuse the test table (for base and ex, not
for ipv4 and ipv6). That will tell us if there's any difference in the
IP parsing part of the base and Ex functions. It'll also be
interesting how the base function handles ports in the string, and, in
case of ipv6, brackets around the IP.

Other than that two minor style issues: Change the subject of patch
one (it only handles RtlIpv4StringToAddressEx), and you still have
different levels of indentation in the line continuations of some ok()
statements.

Cheers,
Stefan

Am 2015-02-15 um 18:02 schrieb Mark Jansen:
> Add test for RtlIpv4StringToAddressEx
>
> --- dlls/ntdll/tests/rtl.c | 101
> +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed,
> 101 insertions(+)

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

iQIcBAEBAgAGBQJU4c5HAAoJEN0/YqbEcdMwnPIP/RT2i0fG4cPqKikwa26VbRyn
EG2yeG78Wjx/vysLuQfGe8alhUycGrgMzW4N4m8QiAx/xNFv4eX0OdI+5gYMbSHK
dYZSzTSKbqoJbnBlZwt1MoaaNceX/zIEy2WoaRCYpsUJfpscT/sOdJ+/gW55gDkH
605vvruhgJFSFVeZDVzfR9gQcY51iHB0KDemX2p1mwHElr6+HwYSHOQt+8KCL8YV
wfC87NSMEAfEjIudYPL7SFHvUuLr3GoHBBt57R9p/2gNT7eJ7ll9ANvShFkRENjL
jmKJQj7exehqrAGNWBqbeIPSjHBQoGl/UZPwIE39UioNhRo+sWi5A8tBbUMyL3o5
VENuxL987kU1L9Xpa6/jEcrQksgafCa742/wWSAjPGE0D2iTUr0hQN0zQoioTPA/
zuaDyDO3CBCyFaF0A9h9FzxOkuRAHo6TSKw0IYOrI2XENAoqQ+B+fX0ll9t/5Tu0
iUIea06yp2/fRButo58sJNAd3dUZpRKf1qT+Eu7Cn+AJfgi3TsKY/Lw0gvyb4XJh
qOdp7EPzqY1PURn6+Z8aDwNZRA2du3NM91SSMfwz+qwZtakf8K/XtkpC1CpNLONJ
kfltxHVALBKT0NTDzvprrSzvTVH90hJLE+nX/E5Qf6SzeRVqxsPomBtOj2VLuapZ
4ehIhXsaEJJz9jc31tr9
=E0dA
-----END PGP SIGNATURE-----

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

[Mark Jansen]

Hello,

I do not agree with this (combining the test into one table):
Any sensible implementation will have the Ex function call the non-Ex
function to handle the ip parsing part,
in which case it does not serve any purpose to run all tests again for
the Ex function (essentially just re-testing the non-Ex function).
Besides that, sharing the table between the functions means having
separate results for the Ex and non-Ex in the same table,
and what is the point of one shared table then?

regards,

On Mon, Feb 16, 2015 at 12:02 PM, Stefan Dösinger
<stefandoesinger(a)gmail.com> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Hi,
>
> I've already talked to Mark about this on #winehackers, just repeating
> it for the list: My remaining request with these tests is that I
> believe we should test the full set of inputs against the Ex and
> non-Ex versions and ideally reuse the test table (for base and ex, not
> for ipv4 and ipv6). That will tell us if there's any difference in the
> IP parsing part of the base and Ex functions. It'll also be
> interesting how the base function handles ports in the string, and, in
> case of ipv6, brackets around the IP.
>
> Other than that two minor style issues: Change the subject of patch
> one (it only handles RtlIpv4StringToAddressEx), and you still have
> different levels of indentation in the line continuations of some ok()
> statements.
>
> Cheers,
> Stefan
>
> Am 2015-02-15 um 18:02 schrieb Mark Jansen:
> > Add test for RtlIpv4StringToAddressEx
> >
> > --- dlls/ntdll/tests/rtl.c | 101
> > +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed,
> > 101 insertions(+)
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v2
>
> iQIcBAEBAgAGBQJU4c5HAAoJEN0/YqbEcdMwnPIP/RT2i0fG4cPqKikwa26VbRyn
> EG2yeG78Wjx/vysLuQfGe8alhUycGrgMzW4N4m8QiAx/xNFv4eX0OdI+5gYMbSHK
> dYZSzTSKbqoJbnBlZwt1MoaaNceX/zIEy2WoaRCYpsUJfpscT/sOdJ+/gW55gDkH
> 605vvruhgJFSFVeZDVzfR9gQcY51iHB0KDemX2p1mwHElr6+HwYSHOQt+8KCL8YV
> wfC87NSMEAfEjIudYPL7SFHvUuLr3GoHBBt57R9p/2gNT7eJ7ll9ANvShFkRENjL
> jmKJQj7exehqrAGNWBqbeIPSjHBQoGl/UZPwIE39UioNhRo+sWi5A8tBbUMyL3o5
> VENuxL987kU1L9Xpa6/jEcrQksgafCa742/wWSAjPGE0D2iTUr0hQN0zQoioTPA/
> zuaDyDO3CBCyFaF0A9h9FzxOkuRAHo6TSKw0IYOrI2XENAoqQ+B+fX0ll9t/5Tu0
> iUIea06yp2/fRButo58sJNAd3dUZpRKf1qT+Eu7Cn+AJfgi3TsKY/Lw0gvyb4XJh
> qOdp7EPzqY1PURn6+Z8aDwNZRA2du3NM91SSMfwz+qwZtakf8K/XtkpC1CpNLONJ
> kfltxHVALBKT0NTDzvprrSzvTVH90hJLE+nX/E5Qf6SzeRVqxsPomBtOj2VLuapZ
> 4ehIhXsaEJJz9jc31tr9
> =E0dA
> -----END PGP SIGNATURE-----

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

[Stefan Dösinger]

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

Hi,

Am 2015-02-19 um 11:00 schrieb Mark Jansen:
> I do not agree with this (combining the test into one table): Any
> sensible implementation will have the Ex function call the non-Ex 
> function to handle the ip parsing part,
> The way the terminator parameter works is a strong indicator of that,
> yeah. But the usual way is for the non-Ex function to call the Ex one.
> Microsoft also loves to duplicate code and functionality (see e.g. gdi
> vs gdiplus vs dwrite). I guess our implementation can be built that
> way, but I don't think the tests should be written based on that
> assumption. Instead, the tests should try their best to show that this
> is how the relation between base and ex works.

I guess the "terminator" output helps the caller (in your case the -Ex
function) find out how many characters the base function read and
where it has to look for the port. Though there are still some
potential trouble cases like "192.168.0.1    :80"
> in which case it does not serve any purpose to run all tests again
> for the Ex function (essentially just re-testing the non-Ex
> function).
> Running the extra tests is not much of a problem. And the full set of
> tests needs to be written anyway.
> Besides that, sharing the table between the functions means having 
> separate results for the Ex and non-Ex in the same table, and what
> is the point of one shared table then?
> If there is no reusable overlap then it obviously makes no sense. But
> my understanding is that for any input that does not contain a port,
> non-ex and -ex will behave in the same way. So I expect that you can
> share IP address based tests like 256.0.0.1 between both. You have to
> test how the non-ex version reacts to port information - these sets of
> input data are needed for both versions too. That will mostly be the
> success cases of the -Ex tests.

Of course there will be some tests (e.g. port corner cases that fail
in -Ex) that only make sense for the Ex version. I don't see a problem
with running them against the base version and checking that you
indeed get STATUS_INVALID_PARAMETER back.

Fwiw, if you still think that merging the tables doesn't make sense
I'll trust you on that - after all, you are writing the code. But
please give it some serious thought.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBAgAGBQJU5bzKAAoJEN0/YqbEcdMwmoYP/jygOokw3Sb1PdVbXBbMwbnb
tPBPY2hxiyLu6dZd1t3GcjhDkxwfoHb3ohC2VuZq6Vl14JMSs2cXJlb8ALuf+tER
06z5gcEtrrj9ggjOgaRrTqxeCKuDXgRWuT/gZ+7yB+0U8hxcDfKRNXEpESnNgqIZ
QzLKLHoFxm0wKakTYyP9ivjQFjD7ge7wypmC5jS0QMVMo7ycHqQ4yowDnPY+6iag
NBDqzxhSt0tomj2Y/lBczuhkeunRN0DA1DU+Nl2jGcnsR7ZZGSA0S+hmz351Y/oz
ysyc6fhESNb1oB0Of3VjOjF3JNX28H7m8JvnyHxTw+7wPVIP854W5PcA7IL4s3Ey
I7y9JgwccMeOek6CN7qo8ei4OwtEsnp82uWMXg1B1tVyNXf8hbGulgzyRIulhxU6
BWcOXaIfRDFzajIVi9JzdXVG28FKbkJvpfhtTiXZEAuQO8ZR2n4gHP+7BejVaIXg
mAxzYJVgCDh6TNTaHMfN0Z0etQF5YxNrnV7LgERZmZsfaxljVaitPch+bPoTdKG3
TsOAUQKFvnW8WKauv4oxHAZ6vYms71A3klISQ9ZyehlpVYS9FUXPfRFR38Sszbm8
J6x2txu3cIO6zHzvyOxOYmwBuA6ewLJiV4uYbutQvsKwCcyR1fDtlFO5NZvtR0qd
NxZe1eNuGKJJhDbC2Aux
=0MMJ
-----END PGP SIGNATURE-----