Hi Alex,
I have a couple of minor suggestions according to our recent conversation.
http://cr.openjdk.java.net/~amenkov/jdk14/jdwp_scopes/webrev/src/jdk.jdwp.agent/share/native/libdt_socket/socketTransport.c.frames.html
272 * Parses scope id.
273 * Scope id is ulong on Windows, uint32 on unix, so returns long
which can be casted to uint32.
274 * On error sets last error and return -1.
275 */
At line 274 replace: 'casted' => 'cast' and 'return' => 'returns'.
Alternatively, you may want to consider imperative style:
272 * Parse scope id.
273 * Scope id is ulong on Windows, uint32 on unix, so return long which
can be cast to uint32.
274 * On error set last error and return -1.
275 */
278 unsigned long scopeId = if_nametoindex(str);
Better to rename 'scopeId' to 'scope_id' for c-style naming.
301 getAddrInfo(const char *hostname, int hostLen,
It would be nice to keep hostLen as size_t.
Then this condition can be removed:
312 if (hostLen < 0) {
313 hostLen = (int)strlen(hostname);
314 }
and this line:
439 err = getAddrInfo(buffer, -1, NULL, &hints, &addrInfo);
replaced with:
439 err = getAddrInfo(buffer, strlen(buffer), NULL, &hints, &addrInfo);
Also, this line can be kept as it is: 320 buffer =
(*callback->alloc)((int)hostLen + 1); Otherwise, the cast (int)hostLen is not needed as hostLen is already int.
Better to rename 'hostLen' to 'hostlen' to keep the c-style naming
convention.
To make it consistent, the same is needed in the function parseAddress().
Spaces are needed arount the '-' sign at line:
316 if (hostLen > 2 && hostname[0] == '[' && hostname[hostLen-1] == ']') {
http://cr.openjdk.java.net/~amenkov/jdk14/jdwp_scopes/webrev/test/jdk/com/sun/jdi/JdwpAttachTest.java.frames.html
Please, consider making the field 'isWindows' final and renaming to
'IsWindows'.
The same suggestion is for JdwpListenTest.java.
60 // It's off by default as it caused significant test time increase\
61 // (tests <number_of_addresse> * <number_of_addresse> cases, each
case fails by timeout).
Replace: 'number_of_addresse' => 'number_of_addresses'.
The '\' is not needed at the end of line 60.
172 // So on Windows test only addresses with numeric scope,
173 // On other platforms test both symbolic and numeric scopes.
Replace comma with dot at the and of 172.
The same suggestion is for JdwpListenTest.java.
Thanks,
Serguei
On 10/10/19 5:42 PM, Alex Menkov wrote:
Hi all,
Please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8224159
webrev:
http://cr.openjdk.java.net/~amenkov/jdk14/jdwp_scopes/webrev/
The change implements IPv6 symbolic scope support in JDWP agent.
Tested
jdk/com/sun/jdi,open/test/hotspot/jtreg/vmTestbase/nsk/jdi
hotspot/jtreg/vmTestbase/nsk/jdwp
hotspot/jtreg/vmTestbase/nsk/jdb
--alex
