Ping: Re: RFR: JDK-8224159: JDWP IPv6 scope support

[Alex Menkov]

Need 2nd reviewer.
The latest webrev:
http://cr.openjdk.java.net/~amenkov/jdk14/jdwp_scopes/webrev.02/

(it includes fix for "final" modifiers in the tests)

--alex

On 10/11/2019 18:55, serguei.spitsyn at oracle.com wrote:
> Hi Alex,
> 
> Thank you for the update!
> It looks good.
> 
> The final modifier is still missed for the IsWindows field.
> No need for new webrev if you fix it.
> 
> You are right, camel case naming style is used all over the file.
> So, I'm Okay with keeping it.
> Replacement 'hostLen' => 'hostnameLen' is a good one.
> 
> Thanks,
> Serguei
> 
> On 10/11/19 6:30 PM, Alex Menkov wrote:
>> 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
>>>
>