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

Chris Plummer chris.plummer at oracle.com
Wed Oct 30 22:42:57 UTC 2019 

Ok. I think everything is fine then once you do the "connectAddresses" 
rename.

thanks,

Chris

On 10/30/19 3:29 PM, Alex Menkov wrote:
> Hi Chris,
>
> thanks you for review.
>
> On 10/30/2019 14:07, 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");
>> }
>
> address  may be omitted so the address contains only port (look at the 
> comment before the line: /* check for host:port or port */)
> And we have some tests which use "address=0" args.
>
>>
>> Why "hostname" instead of "hostAddress":
>>
>>   301 getAddrInfo(const char *hostname, int hostLen,
>
> This is wrapper for getaddrinfo socket function which is historically
> defined as
> int getaddrinfo(const char* hostname,
>                 const char* service,
>                 const struct addrinfo* hints,
>                 struct addrinfo** res);
> ( https://en.wikipedia.org/wiki/Getaddrinfo )
>
> Note that in the latest webrev 2nd arg is size_t hostnameLen (per 
> Serguei request)
> The latest webrev is
> http://cr.openjdk.java.net/~amenkov/jdk14/jdwp_scopes/webrev.02/
>
>>
>> What is the code path that could result in hostlen < 0?
>>
>>   312         if (hostLen < 0) {
>>   313             hostLen = (int)strlen(hostname);
>>   314         }
>
> This code is changed in the latest webrev (now the value is size_t)
> Initially parseAllowedAddr calls the function with hostLen == -1 
> (because hostname is null-terminated in the case).
>
>>
>> Would using ULONG_MAX be better:
>>
>>   289     if (scopeId > 0xFFFFFFFF) {
>
> we need the value to be in uint32 range (see comments before the 
> function: Scope id is ulong on Windows, uint32 on unix, so returns 
> long which can be cast to uint32)
> I'm not sure if there is a standard definition for UINT32_MAX
>
>>
>> Rename "connectAddresses" to "connectAddress"
>>
>>    99     private static void attachTest(String listenAddress, String 
>> connectAddresses, boolean expectedResult)
>
> Will fix before next iteration or before the push.
>
>>
>> For the tests, it would be nice to see some example output with 
>> scopes being used.
>
> I have saved results only for Windows (i.e. with numeric scope id only)
>
> positive testing (listen address equals connect address):
>
> Test: listen at fe80:0:0:0:a0ff:2429:a9ea:f51a%23, attaching from 
> fe80:0:0:0:a0ff:2429:a9ea:f51a%23, expected: SUCCESS
> Starting listening debuggee at fe80:0:0:0:a0ff:2429:a9ea:f51a%23
> [debuggee]:java.exe 
> -agentlib:jdwp=transport=dt_socket,address=fe80:0:0:0:a0ff:2429:a9ea:f51a%23:0,server=y,suspend=y 
> HelloWorld
> [debuggee] Listening for transport dt_socket at address: 52019
> Debuggee is listening on fe80:0:0:0:a0ff:2429:a9ea:f51a%23:52019
> Connecting from fe80:0:0:0:a0ff:2429:a9ea:f51a%23, expected: SUCCESS
>
>
> negative testing (connect address is different from listen address):
>
> Test: listen at fe80:0:0:0:a0ff:2429:a9ea:f51a%23, attaching from 
> 127.0.0.1, expected: FAILURE
> Starting listening debuggee at fe80:0:0:0:a0ff:2429:a9ea:f51a%23
> [debuggee]:java.exe 
> -agentlib:jdwp=transport=dt_socket,address=fe80:0:0:0:a0ff:2429:a9ea:f51a%23:0,server=y,suspend=y 
> HelloWorld
> [debuggee] Listening for transport dt_socket at address: 52001
> Debuggee is listening on fe80:0:0:0:a0ff:2429:a9ea:f51a%23:52001
> Connecting from 127.0.0.1, expected: FAILURE
>
>
> For unix platforms tests also verify that we can connect if listen 
> address and connect addresses are equals, but one of then has numeric 
> scope and other symbolic scope like
>
> Test: listen at fe80:0:0:0:a0ff:2429:a9ea:f51a%23, attaching from 
> fe80:0:0:0:a0ff:2429:a9ea:f51a%eth0, expected: SUCCESS
>
> --alex
>
>>
>> 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
>>>>>>
>>>>
>>
>>

More information about the serviceability-dev mailing list