RFR: JDK-8224159: JDWP IPv6 scope support

[serguei.spitsyn at oracle.com]

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20191011/ab62c54f/attachment-0001.html>