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

[Chris Plummer]

Hi Alex,

I'm not too fond of this bit of code, although it is pre-existing:

  285     port = (colon == NULL ? address : colon + 1);

It's invalid to not specify a port, but the code is lazy in detecting 
this, and instead lets "port" point to something invalid that will later 
fail when getPortNumber() is called. I think it should be made more 
obvious that the port number is bad if there is no colon. Maybe 
something like this:

int portNum = (colon == NULL ? -1 : getPortNumber(colon));
if (portnum < 0) {
   RETURN_ERROR(JDWPTRANSPORT_ERROR_ILLEGAL_ARGUMENT, "invalid port 
number specified");
}

Why "hostname" instead of "hostAddress":

  301 getAddrInfo(const char *hostname, int hostLen,

What is the code path that could result in hostlen < 0?

  312         if (hostLen < 0) {
  313             hostLen = (int)strlen(hostname);
  314         }

Would using ULONG_MAX be better:

  289     if (scopeId > 0xFFFFFFFF) {

Rename "connectAddresses" to "connectAddress"

   99     private static void attachTest(String listenAddress, String 
connectAddresses, boolean expectedResult)

For the tests, it would be nice to see some example output with scopes 
being used.

thanks,

Chris

On 10/23/19 10:51 AM, Alex Menkov wrote:
> 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
>>>>
>>