Serguei, Thanks. will fix indend before push.
-Dmitry On 2014-05-16 22:24, serguei.spit...@oracle.com wrote: > Dmitry, > > The lines 197-241 still have wrong indent - must be 4. > Otherwise, it is good - reviewed. > > Thanks, > Serguei > > On 5/16/14 8:19 AM, Dmitry Samersoff wrote: >> 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.
