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

[Chris Plummer]

On 10/30/19 2:07 PM, Chris Plummer wrote:
> 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");
> }
>
Actually, I misunderstood the code. Looks like you can just specify a 
port without an address, in which case there is no colon, so the code is 
fine as-is. Sorry.

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