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.
