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.
