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/
src/jdk.jdwp.agent/share/native/include/jdwpTransport.h
Needs copyright year update.
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.
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.
L396: while(1) {
Please add space before '('.
L408: // Input is not consumed, something bad happens
typo: 'happens' -> 'happened'
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?
L421: _peers_cnt += 1;
Why not ++_peers_cnt or _peers_cnt++?
I don't see any checks for overflow of MAX_PEER_ENTRIES in
parseAllowedPeers().
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).
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);
src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c
No comments.
src/jdk.jdwp.agent/share/native/libjdwp/transport.c
Needs copyright year update.
L150: if (name == NULL) {
You should add a check for parameter 'info' after this block.
'info' should not be NULL either.
L203: size_t i;
This decl can be moved to this line:
L209 for (i = 0; i < sizeof(supported_versions); ++i) {
L210-214: four space indents should be used.
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.
L241: * even if info is already dealocated.
Typo: 'dealocated' -> 'deallocated'
L507-511: four space indents should be used.
L513-523: four space indents should be used.
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.
src/jdk.jdwp.agent/share/native/libjdwp/transport.h
Needs copyright year update.
test/com/sun/jdi/BasicJDWPConnectionTest.java
L32-34 - imports should be sorted.
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
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=*";
So you have two bad ones before the good option '*'. Not
sure if that's what you were really looking for though...
I think that's it. I still need to review the CSR...
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
