[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: Ping: Re: RFR: JDK-8224159: JDWP IPv6 scope support
From: Chris Plummer <chris.plummer () oracle ! com>
Date: 2019-10-30 22:42:57
Message-ID: c52808db-f107-8324-eba2-821112f23a19 () oracle ! com
[Download RAW message or body]
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@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@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
> > > > > >
> > > >
> >
> >
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic