|
On 8/31/17 15:08, Daniel D. Daugherty
wrote:
On 8/31/17 4:06 PM, serguei.spit...@oracle.com
wrote:
On 8/31/17 14:51, Daniel D.
Daugherty wrote:
I compared the .2 and .3 patches. Everything
good except for
this whitespace change that didn't show up in the webrev
(IIRC):
- /*
+ /*
* Start the transport loop in a separate thread
*/
Fixed this one and one more comment above:
598 /*
599 * If we're connecting to another process, there
shouldn't be
I'll wait to see how we resolve the new "exit
code path" issue
I removed this line now.
Will try to reproduce the situation I saw before as a follow up.
Thank you for catching this problem!
I know this code much better now. :)
before giving the official thumbs up!
Thanks, I'm waiting for it! :)
You have an official thumbs up. :-)
Thanks a lot, Dan!
I'll run the JDI tests before pushing to be sure nothing is broken
with the latest updates.
Thanks,
Serguei
Dan
Thanks,
Serguei
Dan
On 8/31/17 11:54, Daniel D.
Daugherty wrote:
On 8/29/17 2:44 AM, serguei.spit...@oracle.com
wrote:
src/jdk.jdwp.agent/share/native/include/jdwpTransport.h
L202: /* 12: SetTransportConfiguration */
I missed updating this comment also. Perhaps:
/* 12: SetTransportConfiguration added
in JDWPTRANSPORT_VERSION_1_1 */
and add this one before L262:
/* SetTransportConfiguration added in
JDWPTRANSPORT_VERSION_1_1 */
Fixed.
src/jdk.jdwp.agent/share/native/libdt_socket/socketTransport.c
L414: "invalid ip address
in allow option");
Should this "ip" be "IP"?
Fixed.
L435: if (++_peers_cnt
>= MAX_PEER_ENTRIES) {
L436: fprintf(stderr, "Error in
allow option: '%s'\n", allowed_peers);
L437:
RETURN_ERROR(JDWPTRANSPORT_ERROR_ILLEGAL_ARGUMENT,
L438: "exceeded max
number of allowed peers (32)");
L439: }
I think this error block will execute if you
happen to
have exactly 32 allowed peers, i.e., "*s == 0"
and I
don't think that's what you want.
I think you want the check to be:
if (_peers_cnt >= MAX_PEER_ENTRIES) {
and you want that check to be before L433.
Basically, if the
current count has overflowed, error out. Of
course, you'll
want the "++peer_cnt" increment just before "if
(*s == 0)".
Nice catch.
Fixed as you suggested.
L438:
"exceeded max number of allowed peers (32)");
That literal '32' is a maintenance problem when
you have
MAX_PEER_ENTRIES available.
Fixed.
Now, it is: "exceeded max number of allowed peers: "
MAX_PEER_ENTRIES);
L444: // advance to next ip block
Should this "ip" be "IP"?
Fixed.
L623: static int
option_was_printed = 0;
Variable is not used.
Removed.
L645: if (err) {
Not your bug, but this if-statement should be:
if (err != JDWPTRANSPORT_ERROR_NONE) {
Fixed.
I saw it too but was trying minimize the volume of review.
src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c
L574: EXIT_ERROR(map2jvmtiError(serror),
"JDWP Transport failed to initialize");
This is a new exit code path. Previously the
process
did not exit here. Why the change in behavior?
This improves the diagnosability.
I investigated a situation with this error in transport
initialization and was puzzled why the test was passed.
This line fixed the issue.
But I see another message on this topic from you.
Will continue this discussion in reply on it.
src/jdk.jdwp.agent/share/native/libjdwp/transport.c
L208: jint supported_versions[2] =
{JDWPTRANSPORT_VERSION_1_1, JDWPTRANSPORT_VERSION_1_0};
Please consider adding a comment above this
line:
/* If a new version is added here, update 'case
JNI_EVERSION' below. */
Done.
L463: info->transportVersion =
transportVersion;
Perhaps init name, address and allowed_peers
fields to NULL
here. I don't think jvmtiAllocate() guarantees
NULL init.
Added the initialization lines and removed a couple of
lines
for isServer case as they became dups.
L529: cfg.allowed_peers =
info->allowed_peers;
In the 'goto handlerError' case on L534, you are
publishing the
info->allowed_peers in cfg.allowed_peers, but
you're going to
free it. Do you want to NULL out
cfg.allowed_peers in the
'goto handlerError' case?
No need to do it as the cfg is a local.
L602: if (err !=
JDWPTRANSPORT_ERROR_NONE) {
In this error block, you added the free of
'info'. Nice catch!
Perhaps add a comment that the name, address and
allowed_peers
fields in 'info' are not allocated in the
non-server case so
they do not need to be freed.
Added a comment.
src/jdk.jdwp.agent/share/native/libjdwp/transport.h
No comments.
test/com/sun/jdi/BasicJDWPConnectionTest.java
L173: // Bad mix of allow address value with
allow option '*'
L174: String allowOpt =
",allow=allow=127.0.0.1+*";
Sorry, I'm still puzzled by this test case. With
the
description on L173, I would expect L174 to be:
String allowOpt =
",allow=127.0.0.1+allow=*";
Ok, I fixed the comments like this:
167 // Bad mix of allow option '*' with address
value
168 String allowOpt = ",allow=*+allow=127.0.0.1";
. . .
173 // Bad mix of allow address value with '*'
174 String allowOpt = ",allow=allow=127.0.0.1+*";
Please, updated webrevs:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2017/hotspot/8061228-jdi-transport.3/
http://cr.openjdk.java.net/~sspitsyn/webrevs/2017/hotspot/8061228-jdi-transport.3.inc/
The last one is relative to the webrev.2, not the Dmitry's
webrev.18.
Thanks a lot, Dan!
Serguei
Dan
http://cr.openjdk.java.net/~sspitsyn/webrevs/2017/hotspot/8061228-jdi-transport.2.inc/
I think, I've resolved all you comments/suggestions.
The list of allowed peers is still not printed in
socketTransport_accept()
in case of a rejected peer (not sure, if it is very
necessary at this point).
The issue is that the allow option is not available at
this point.
Regenerating it from the _peers array is non-trivial
and error-prone.
I'll try to implement it, if you think it is
important.
The nsk.jdi and JTreg jdk_jdi test runs are in
progress.
Thanks,
Serguei
On 8/28/17 15:12, serguei.spit...@oracle.com
wrote:
Hi Dan,
Thank you a lot for review!
On 8/28/17 11:00, Daniel D. Daugherty wrote:
Resending with Dmitry's e-mail
address included.
Please delete the previous version.
On 8/22/17 5:22 PM, serguei.spit...@oracle.com
wrote:
Please, review another revision of the fix for the
enhancement:
https://bugs.openjdk.java.net/browse/JDK-8061228
CSR:
https://bugs.openjdk.java.net/browse/CCC-8061228
The SCR is in the DRAFT state.
Joe suggested to consider this CSR approved and
gave a GO for integration.
It will be moved to the right state later when
the CSR tools are ready.
I'm still asking at least one reviewer to look
at this CSR and give a thumbs up.
It is to ensure everything is going in a right
direction.
I'll finalize the CSR after that.
Webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2017/hotspot/8061228-jdi-transport.1/
> http://cr.openjdk.java.net/~sspitsyn/webrevs/2017/hotspot/8061228-jdi-transport.2/
You seems to looked at 8061228-jdi.transport.2
that I generated
temporarily for myself and which is obsolete now.
The 8061228-jdi.transport.1 was sent
for review and needs to be used.
I will consider and fix all the comments that are
still relevant for v1.
src/jdk.jdwp.agent/share/native/include/jdwpTransport.h
Needs copyright year update.
It was fixed in the The 8061228-jdi.transport.1.
L150: const char* allowed_peers; /*
Peers allowed for connection */
Please consider adding the following
comment above this line:
/* Field added in
JDWPTRANSPORT_VERSION_1_1: */
That should provide a hint to future
maintainers about
how to add fields to
jdwpTransportConfiguration.
src/jdk.jdwp.agent/share/native/libdt_socket/socketTransport.c
Needs copyright year update.
It was fixed in the The 8061228-jdi.transport.1.
L31: #include <netinet/in.h>
L34: #include <netinet/in.h>
Duplicated includes. Would be easier to
spot if the includes
were sorted, but that doesn't seem to be
the style in the file.
For the includes that you add, can you
sort those? I don't
recommend sorting the existing ones since
that would make this
patch messier.
Nice catch.
Fixed.
L396: while(1) {
Please add space before '('.
Fixed.
L408: // Input is not
consumed, something bad happens
typo: 'happens' -> 'happened'
Fixed.
L410:
RETURN_ERROR(JDWPTRANSPORT_ERROR_ILLEGAL_ARGUMENT,
You don't print the current value of 's'
before this error
return like you did in the previous error
return. Why?
This is printed/fixed in v1.
L421: _peers_cnt += 1;
Why not ++_peers_cnt or _peers_cnt++?
As it is minor, I did not want to fix it to minimize
my incremental webrev.
Fixed now.
I don't see any checks for overflow of
MAX_PEER_ENTRIES in
parseAllowedPeers().
Nice catch.
Fixed.
L590: fprintf(stderr, "ERROR: Peer
not allowed to connect, peers_cnt: %d\n",
_peers_cnt);
_peers_cnt is not particular interesting.
It might be
more interesting to print info about the
peer that's
trying to connect and maybe the list of
allowed peers
(one time).
The _peers_cnt value is not printed in the webrev.1.
I agree, it is better to print
I'm not sure in what form to print the details about
the peer that's trying to connect
Should I use something like this:
char buffer[20] = { 0 };
inet_ntop(AF_INET, &(sa.sin_addr),
buffer, len);
socketTransport_accept() is executing a "do
{...} while (socketFD < 0);"
loop with various return points due to errors.
Your new
"if (_peers_cnt > 0)" block short circuits
the logic in the
"if (err) {" block that manages the
acceptTimeout variable
so the time we spent waiting for the
connection won't be
counted against the overall timeout specified
by the
caller.
Example:
- Say the caller asks for a 30 second timeout.
- After 25 seconds we get a connection from an
unapproved peer.
- We won't update acceptTimeout (decrement by
25
seconds) so we won't return from the
socketTransport_accept() call for 55
seconds.
I think acceptTimeout management has to be
refactored
to be common to both the not-allowed-peer path
and
the error path.
L935: int err;
Can move this variable decl to this line:
L955: err =
parseAllowedPeers(allowed_peers);
Good suggestion - fixed.
src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c
No comments.
src/jdk.jdwp.agent/share/native/libjdwp/transport.c
Needs copyright year update.
It was fixed in the The 8061228-jdi.transport.1.
L150: if (name == NULL) {
You should add a check for parameter
'info' after this block.
'info' should not be NULL either.
Nice catch - fixed.
L203: size_t i;
This decl can be moved to this line:
L209 for (i = 0; i <
sizeof(supported_versions); ++i) {
It is not a C++ code, so the declaration can not be
moved to the line 203.
At least, some C compilers would not accept it.
L210-214: four space indents should be used.
Fixed.
L224:
ERROR_MESSAGE(("transport doesn't recognize
supported versions"));
Perhaps you should also list the supported
versions that were
tried so there's more failure info.
Nice suggestion.
Fixed.
L241: * even if info is already
dealocated.
Typo: 'dealocated' -> 'deallocated'
Fixed.
L507-511: four space indents should be used.
L513-523: four space indents should be used.
Fixed.
L527-541: four space indents should be used,
but I don't think
the switch statement is a good idea. That
logic block should
be something like:
err = (*trans)->StartListening(trans,
address, &retAddress);
if (err != JDWPTRANSPORT_ERROR_NONE) {
printLastError(trans, err);
serror = JDWP_ERROR(TRANSPORT_INIT);
goto handleError;
}
if (info->transportVersion >=
JDWPTRANSPORT_VERSION_1_1) {
config.allowed_peers =
info->allowed_peers;
err =
(*trans)->SetTransportConfiguration(trans,
&config);
if (err != JDWPTRANSPORT_ERROR_NONE) {
printLastError(trans, err);
serror =
JDWP_ERROR(TRANSPORT_INIT);
goto handleError;
}
}
The error checking block at L544-548 is
now above. Note
that I don't see a reason to error here if
the version
is newer than JDWPTRANSPORT_VERSION_1_1.
Agreed - fixed.
I was thinking about the same refactoring but decided
to keep the original minimize my update.
Also, please, note that the order of calls to SetTransportConfiguration
and StartListening is different in the
webrev.1.
src/jdk.jdwp.agent/share/native/libjdwp/transport.h
Needs copyright year update.
It was fixed in the The 8061228-jdi.transport.1.
test/com/sun/jdi/BasicJDWPConnectionTest.java
L32-34 - imports should be sorted.
Fixed.
L165: // Bad mix of option '*' with
other adress values
Typo: 'adress' -> 'address'
Not sure I like that description though.
Perhaps:
// Bad mix of option '*' with bad allow
address value
Fixed.
The address should not be bad, so I've put the
127.0.0.1 there.
It looks like this now:
167 // Bad mix of allow option '*' with allow
address value
168 String allowOpt =
",allow=*+allow=127.0.0.1";
L171: // Bad mix of option '*' with
other adress values
Typo: 'adress' -> 'address'
Not sure I like that description though
because you
don't have a correctly formed '*' option
there. Perhaps:
// Bad mix of bad allow address values
with option '*':
String allowOpt =
",allow=allow=0.0.0.0+allow=*";
Fixed.
The address should not be bad, so I've put the
127.0.0.1 there.
It looks like this now:
173 // Bad mix of allow address value with
allow option '*'
174 String allowOpt =
",allow=allow=127.0.0.1+*";
So you have two bad ones
before the good option '*'. Not
sure if that's what you were really
looking for though...
Right.
A good address must be there, a bad address was used
by mistake.
I think that's it. I still need to review the
CSR...
Wow!
Good catches and nice suggestions.
The updated webrev is (one comment has not been
resolved yet):
http://cr.openjdk.java.net/~sspitsyn/webrevs/2017/hotspot/8061228-jdi-transport.2/
The original 8061228-jdi-transport.2 was moved to
8061228-jdi-transport.2.old .
Thanks a lot, Dan!
Serguei
Dan
The lastest webrev from Dmitry:
http://cr.openjdk.java.net/~dsamersoff/JDK-8061228/webrev.18/
Incremental webrev vs the latest webrev from
Dmitry:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2017/hotspot/8061228-jdi-transport.1.inc/
Summary:
This enhancement was developed by Dmitry who
left the team.
I don't know what email address to use to CC him
at this point.
I hope, Dmitry will find this discussion and
reply accordingly.
The latest webrev revision from Dmitry was v18
(please, see above).
This revision covers the following:
- Cleanup for versioning negotiation protocol
(back up to the original).
Now the transport library supports both
versions 1_0 and 1_1 (newly introduced).
- The transport native interface was changed.
The function SetTransportConfiguration() is
introduced instead of the
StartListeningWithAllow(). It allows to the
same transport library to support
both old and new version of the transport
interface. At this point, the
new structure jdwpTransportConfiguration
includes only one field:
const char*
allowed_peers;
But it can be extended in the future
if any other update in configuration
will be required.
- The unit test was updated to provide better
coverage of the corner cases
for 'allow' option introduced by this
enhancement.
- Fixes to improve diagnosability.
- A couple of bugs/regressions were fixed so
that all the JDI tests are passed now.
- A cleanup that includes some renaming and
reformatting.
Testing:
Tested new agent flag (allow), with new test:
jdk/test/com/sun/jdi/BasicJDWPConnectionTest.java
Ran the nsk.jdi, nsk.jdwp and jtreg jdk_jdi for
both release and fastdebug builds.
All tests are passed.
Thanks,
Serguei
|
