[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR: JDK-8224159: JDWP IPv6 scope support
From:       serguei.spitsyn () oracle ! com
Date:       2019-10-12 1:55:44
Message-ID: 0a4b1703-77f6-b503-2d9f-0e8272418849 () oracle ! com
[Download RAW message or body]

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]