Serguei, Fixed in place, please press shift-reload.
http://cr.openjdk.java.net/~dsamersoff/JDK-8041435/webrev.02/ -Dmitry On 2014-05-16 03:19, serguei.spit...@oracle.com wrote: > On 5/15/14 3:20 PM, serguei.spit...@oracle.com wrote: >> Dmitry, >> >> I've reviewed the .01 webrev. > > Sorry, wanted to say .02 webrev. > > > Thanks, > Serguei > >> >> src/share/transport/socket/socketTransport.c >> >> Lines 197-240: to be consistent with the rest of the file the indent must be >> 4, not 2. >> The comment at the line 200 is better to start from capital letter: "It >> looks up ..." >> >> 202 struct addrinfo hints, *res; >> ... >> 205 memset(&hints, 0, sizeof(hints)); >> Would it more simple to use initialization instead of memset: >> >> 202 struct addrinfo hints = { 0 }; >> 203 struct addrinfo *res = NULL; >> >> >> The space is missed after the commas and around '+': >> >> 228 n = strtoul(s_port,&eptr,10); >> 229 if (eptr != s_port+strlen(s_port)) { >> 247 memset((void *)sa,0,sizeof(struct sockaddr_in)); >> 261 } else if (strncmp(address,"localhost:",10) == 0) { >> >> Unnecessary dot: >> 235 // invalid port number. >> >> >> The n is declared as u_long but a cast to u_short is used in comparison: >> 220 u_long n; >> ... >> 234 if (n > (u_short) -1) { >> Not sure I understand the condition at line 234. >> The unsigned long n is always bigger than -1, right? >> Or the type u_long is not unsigned long? >> >> >> Thanks, >> Serguei >> >> On 5/15/14 12:31 PM, Dmitry Samersoff wrote: >>> Dan, >>> >>> Thank you for the review. Next version is: >>> >>> http://cr.openjdk.java.net/~dsamersoff/JDK-8041435/webrev.02/ >>> >>> please, see below. >>> >>> On 2014-05-15 19:58, Daniel D. Daugherty wrote: >>>> On 5/15/14 5:50 AM, Dmitry Samersoff wrote: >>>>> Hi Everyone, >>>>> >>>>> Please review the fix: >>>>> >>>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8041435/webrev.01/ >>>> src/share/transport/socket/socketTransport.c >>>> line 200: // it lookups "localhost" and return 127.0.0.1 if lookup >>>> Typo: "it lookups" -> "it looks up" >>>> Typo: "and return 127.0.0.1" -> "and returns 127.0.0.1" >>> fixed. . >>> >>>> line 210: return dbgsysHostToNetworkLong(INADDR_LOOPBACK); >>>> nit: indent should be 4-spaces (for this file) >>> fixed. >>> >>>> line 235: u_short port = (u_short)atoi(address+10); >>>> Will this crash on "localhost:"? (colon, but no port number) >>>> >>>> line 236: sa->sin_port = dbgsysHostToNetworkShort(port); >>>> If atoi() on line 235 is passed a non-decimal, what will the >>>> value of 'port' be and what will happen here? >>>> >>>> line 241: u_short port = (u_short)atoi(address+2); >>>> line 242: sa->sin_port = dbgsysHostToNetworkShort(port); >>>> Same questions about these two lines... >>> Original code doesn't have any guards for invalid user input and I plan >>> to add it later as a part of upcoming cleanup. >>> >>> But it's worth to do it now. I also add a test for it. >>> >>> >>>> old line 440: err = parseAddress(addressString, &sa, 0x7f000001); >>>> Is it bad that my brain automatically translated that HEX >>>> string into 127.0.0.1? I guess I still remember networking >>>> code... >>> Yes, you are correct. It's a bad way to specify loopback address. I >>> replaced it with INADDR_LOOPBACK. >>> >>> >>>> I checked out this doc: >>>> >>>> http://docs.oracle.com/javase/7/docs/technotes/guides/jpda/conninv.html >>> Thanks! >>> >>> -Dmitry >>> >>>> and the help message: >>>> >>>> $ java -agentlib:jdwp=help >>>> Java Debugger JDWP Agent Library >>>> -------------------------------- >>>> >>>> (see http://java.sun.com/products/jpda for more information) >>>> >>>> jdwp usage: java -agentlib:jdwp=[help]|[<option>=<value>, ...] >>>> >>>> Option Name and Value Description Default >>>> --------------------- ----------- ------- >>>> suspend=y|n wait on startup? y >>>> transport=<name> transport spec none >>>> address=<listen/attach address> transport spec "" >>>> server=y|n listen for debugger? n >>>> launch=<command line> run debugger on event none >>>> onthrow=<exception name> debug on throw none >>>> onuncaught=y|n debug on any uncaught? n >>>> timeout=<timeout value> for listen/attach in milliseconds n >>>> mutf8=y|n output modified utf-8 n >>>> quiet=y|n control over terminal messages n >>>> >>>> Obsolete Options >>>> ---------------- >>>> strict=y|n >>>> stdalloc=y|n >>>> >>>> Examples >>>> -------- >>>> - Using sockets connect to a debugger at a specific address: >>>> java -agentlib:jdwp=transport=dt_socket,address=localhost:8000 ... >>>> - Using sockets listen for a debugger to attach: >>>> java -agentlib:jdwp=transport=dt_socket,server=y,suspend=y ... >>>> >>>> Notes >>>> ----- >>>> - A timeout value of 0 (the default) is no timeout. >>>> >>>> Warnings >>>> -------- >>>> - The older -Xrunjdwp interface can still be used, but will be removed in >>>> a future release, for example: >>>> java -Xdebug -Xrunjdwp:[help]|[<option>=<value>, ...] >>>> >>>> >>>> I don't think your change requires any doc changes... >>>> >>>> Dan >>>> >>>> >>>>> After the fix, JDWP server attempts to guess localhost address and bind >>>>> to it only if no address is specified in command line but user can >>>>> explicitly bind server to all of available addresses by using * as an >>>>> address. >>>>> >> > -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
