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