Hi Alex,
It looks okay to me.
Minor:
+ return bytes[0] == 0x20 && bytes[1] == 0x01 && bytes[2] == 00 && bytes[3] == 0;
'00' looks strange, maybe you want something like this:
+ return bytes[0] == 0x20 && bytes[1] == 0x01 && bytes[2] == 0x0 && bytes[3] == 0x0;
Thanks,
Serguei
On 2/7/20 14:06, Alex Menkov wrote:
Updated
webrev:
http://cr.openjdk.java.net/~amenkov/jdk15/JdwpTestsTeredo/webrev.02/
I decided to go 2nd way.
--alex
On 02/06/2020 17:31, Chris Plummer wrote:
Either is fine by me.
Chris
On 2/6/20 3:40 PM, Alex Menkov wrote:
Hi Chris,
Thank you for the review.
So we have 2 ways - create new RFE for refactoring and then
fix this bug in updated code.
or just fix this 2 tests without refactoring (the changes in
the tests will be identical).
Do you think it makes sense to go #1 or just do #2?
Regarding using Platform.isWindows - it's good for the case,
I'll fix it in the next iteration.
--alex
On 02/06/2020 15:01, Chris Plummer wrote:
Hi Alex,
When refactoring is big and the bug fix is small, I prefer
to see the refactoring done first. It just keeps things
cleaner and makes it easier for the reviewer to see the
important changes. It also helps anyone looking at this bug
or these tests in the future to better recognize what the
actual bug fix was, and what was just refactoring. Think if
there was another test with this issue, and someone was
looking at the diff of this fix to see how to apply it to
the other test.
BTW, there is already a Platform.isWindows() API. It should
probably be used rather than the check the test is using. It
is a slightly different test however, testing for a prefix
of "win" rather than "windows" anywhere in the string.
thanks,
Chris
On 2/6/20 1:14 PM, Alex Menkov wrote:
Hi all,
Please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8234935
webrev:
http://cr.openjdk.java.net/~amenkov/jdk15/JdwpTestsTeredo/webrev/
The failures are caused by Teredo clients
(https://en.wikipedia.org/wiki/Teredo_tunneling).
The fix filters out corresponding addresses.
JdwpListenTest and JdwpAttachTest use the same way to get
addresses for testing. As this is not the 1st time the
algorithm is updated I decided to deduplicate the code and
move shared code to new base class.
So actual change is the addition of
71 // Teredo clients cause intermittent errors on listen
("bind failed")
72 // and attach ("no route to host").
73 // Teredo is supposed to be a temporary measure, but
some test machines have it.
74 if (isTeredo(addr6)) {
75 continue;
76 }
and isTeredo method implementation.
--alex
|
