Ping: Re: RFR: JDK-8224159: JDWP IPv6 scope support
Alex Menkov
alexey.menkov at oracle.com
Wed Oct 30 22:29:08 UTC 2019
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