RFR: JDK-8224159: JDWP IPv6 scope support

[Alex Menkov]

Hi Serguei,

thanks you for review.
updated webrev:
http://cr.openjdk.java.net/~amenkov/jdk14/jdwp_scopes/webrev.02/

replies are inline.

On 10/11/2019 17:04, serguei.spitsyn at oracle.com wrote:
> 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'.

fixed.

>   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.

most variable/function names in the file are camelCase, so I think it 
makes sense to use it.

> 
> 301 getAddrInfo(const char *hostname, int hostLen,
> 
> It would be nice to keep hostLen as size_t.

done.

> 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().

made them hostnameLen.

> 
> Spaces are needed arount the '-' sign at line:
> 
> 316 if (hostLen > 2 && hostname[0] == '[' && hostname[hostLen-1] == ']') {

fixed.

> 
> 
> 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.

fixed.

> 
> 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.

done.

--alex

> 
> 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
>