[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR 8163083: SocketListeningConnector does not allow invocations with port 0
From: serguei.spitsyn () oracle ! com
Date: 2018-09-27 18:58:07
Message-ID: cd8645f0-0afb-29c1-9c6a-7b117a8db52a () oracle ! com
[Download RAW message or body]
It looks good to me.
Thanks!
Serguei
On 9/27/18 11:50 AM, Daniil Titov wrote:
>
> Hi Serguei,
>
> The webrev was updated to address all these comments.
>
> Webrev: http://cr.openjdk.java.net/~dtitov/8163083/webrev.08
> <http://cr.openjdk.java.net/%7Edtitov/8163083/webrev.08>
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8163083
>
> Thanks!
>
> --Daniil
>
> *From: *<serguei.spitsyn@oracle.com>
> *Organization: *Oracle Corporation
> *Date: *Thursday, September 27, 2018 at 11:33 AM
> *To: *Daniil Titov <daniil.x.titov@oracle.com>, JC Beyler
> <jcbeyler@google.com>
> *Cc: *<alexey.menkov@oracle.com>, <gary.adams@oracle.com>,
> <serviceability-dev@openjdk.java.net>
> *Subject: *Re: RFR 8163083: SocketListeningConnector does not allow
> invocations with port 0
>
> Just noticed one more minor issue:
>
> +import java.util.Collection;
> +import java.util.Iterator;
> +import java.util.LinkedHashMap;
>
>
> It seems the above imports are not really used and can be removed.
>
> Thanks,
> Serguei
>
> On 9/27/18 11:22 AM, serguei.spitsyn@oracle.com
> <mailto:serguei.spitsyn@oracle.com> wrote:
>
> Hi Daniil,
>
> It looks great, thank you for the update.
> I have a couple of more minor comments on the test.
>
> 56 testWithDefaultArgs(connector);
>
> 57 testWithDefaultArgs2(connector);
>
> 58 testWithWildcardPort1(connector);
>
> 59 testWithWildcardPort2(connector);
>
>
> Please, rename testWithDefaultArgs to testWithDefaultArgs1
> to make naming consistent.
>
> 111 throw new RuntimeException("Test \
> testWithDefaultArgsNegative failed. The argument map was not updated with" +
> 112 " the bound port number.");
>
> 115 // This call should fail since the previous the \
> argument map is already updated with the port
> 116 // number of the started listener
>
>
> Could you, please, re-balance the above line pairs to make the
> first line shorter?
>
> No need in new webrev if you fix the above.
>
> Thanks,
> Serguei
>
> On 9/27/18 11:00 AM, Daniil Titov wrote:
>
> Hi Serguei,
>
> Thank you for reviewing this change. Initially I understood
> the whole fragment below (from the Javadoc for
> com.sun.jdi.connect.ListeningConnector.startListening()
> method) as a direction for the user how to obtain and prepare
> the argument map before passing it in startListening() method.
>
> 61 * The argument map associates argument name strings to instances
>
> 62 * of {@link Connector.Argument}. The default argument map for a
>
> 63 * connector can be obtained through {@link \
> Connector#defaultArguments}.
> 64 * Argument map values can be changed, but map entries should not be
>
> 65 * added or deleted.
>
> But I agree that the last sentence could also mean that the
> argument map values could be changes as a result of the method
> invocation and in this case the new fragment in the Javadoc is
> not needed.
>
> Please review the updated webrev that does not add new Javadoc
> fragment and includes other changes you suggested.
>
>
> Webrev: http://cr.openjdk.java.net/~dtitov/8163083/webrev.07/
> <http://cr.openjdk.java.net/%7Edtitov/8163083/webrev.07/>
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8163083
>
> Thanks!
>
> --Daniil
>
> *From: *<serguei.spitsyn@oracle.com>
> <mailto:serguei.spitsyn@oracle.com>
> *Organization: *Oracle Corporation
> *Date: *Wednesday, September 26, 2018 at 8:12 PM
> *To: *Daniil Titov <daniil.x.titov@oracle.com>
> <mailto:daniil.x.titov@oracle.com>, JC Beyler
> <jcbeyler@google.com> <mailto:jcbeyler@google.com>
> *Cc: *<alexey.menkov@oracle.com>
> <mailto:alexey.menkov@oracle.com>, <gary.adams@oracle.com>
> <mailto:gary.adams@oracle.com>,
> <serviceability-dev@openjdk.java.net>
> <mailto:serviceability-dev@openjdk.java.net>
> *Subject: *Re: RFR 8163083: SocketListeningConnector does not
> allow invocations with port 0
>
> Hi Daniil,
>
> It is great that you came up the fix for this issue.
> Thanks to Gary for the help too.
>
> I wonder if we could get away without updating the javadoc
> of com/sun/jdi/connect/ListeningConnector.java.
> Filing a CSR would not be needed then (simple javadoc
> reformatting should not require a CSR).
>
> So, my question is if this new fragment is really needed:
>
> 76 * <p>
>
> 77 * If the addressing information provided in {@code
> arguments} implies
>
> 78 * the auto detection this information might be
> updated with the address
>
> 79 * of the started listener.
>
> The javadoc for startListening already has this fragment:
>
> 61 * The argument map associates argument name strings to instances
>
> 62 * of {@link Connector.Argument}. The default argument map for a
>
> 63 * connector can be obtained through {@link \
> Connector#defaultArguments}.
> 64 * Argument map values can be changed, but map entries should not be
>
> 65 * added or deleted.
>
>
>
> that allows to change the argument map values.
>
> It looks like, it has to be Okay to add the bound port number there.
>
>
>
> Please, let me know what you think.
>
> Some formatting comments to addition to Jc's comments:
>
> 77 * If the addressing information provided in {@code
> arguments} implies
>
> 78 * the auto detection this information might be
> updated with the address
>
> 79 * of the started listener.
>
>
>
> This sentence needs to be split in two:
>
>
>
> 77 * If the addressing information provided in {@code
> arguments} implies
>
> 78 * the auto detection. This information might be
> updated with the address
>
> 79 * of the started listener.
>
> +
>
> + protected void updateArgumentMapIfRequired(
>
> + Map<String, ? extends Connector.Argument>
> args,TransportService.ListenKey listener) {
>
> + }
>
> +
>
> The indent has to be 4, not 8.
>
> + if(isWildcardPort(args)) {
>
> + String[] address = listener.address().split(":");
>
> + if (address.length > 1) {
>
> + args.get(ARG_PORT).setValue(address[1]);
>
> + }
>
> + }
>
> A space is missed after the first 'if'.
>
> 50 filter(c ->
>
> 51 \
> c.name().equals("com.sun.jdi.SocketListen")).findFirst().get();
> This is better to be one liner.
>
> 57 testWithDefaultArgs(connector);
>
> 58 testWithDefaultArgs2(connector);
>
> 59 testWithWildcardPort(connector);
>
> 60 testWithWildcardPort2(connector);
>
>
>
> A suggestion is to rename above as below to have the names more unified:
>
>
>
> 57 testWithDefaultArgs1(connector);
>
> 58 testWithDefaultArgs2(connector);
>
> 59 testWithWildcardPort1(connector);
>
> 60 testWithWildcardPort2(connector);
>
>
>
> Thanks,
> Serguei
>
>
> On 9/25/18 10:32 AM, Daniil Titov wrote:
>
> HI JC,
>
> The webrev is updated to address this.
>
> Webrev:
> http://cr.openjdk.java.net/~dtitov/8163083/webrev.06
> <http://cr.openjdk.java.net/%7Edtitov/8163083/webrev.06>
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8163083
>
> Thanks!
> --Daniil
>
> *From: *JC Beyler <jcbeyler@google.com>
> <mailto:jcbeyler@google.com>
> *Date: *Monday, September 24, 2018 at 8:47 PM
> *To: *<daniil.x.titov@oracle.com>
> <mailto:daniil.x.titov@oracle.com>
> *Cc: *<alexey.menkov@oracle.com>
> <mailto:alexey.menkov@oracle.com>, <gary.adams@oracle.com>
> <mailto:gary.adams@oracle.com>,
> <serviceability-dev@openjdk.java.net>
> <mailto:serviceability-dev@openjdk.java.net>
> *Subject: *Re: RFR 8163083: SocketListeningConnector does
> not allow invocations with port 0
>
> Hi Daniil,
>
> Still looks good to me :)
>
> Nit: empty line 83 of the new test is not required!
>
> Jc
>
> On Mon, Sep 24, 2018 at 5:54 PM Daniil Titov
> <daniil.x.titov@oracle.com
> <mailto:daniil.x.titov@oracle.com>> wrote:
>
> Hi Alex,
>
> Please review the updated webrev that has new test
> moved to test/jdk/comsun/jdi/connect folder.
>
> Webrev:
> http://cr.openjdk.java.net/~dtitov/8163083/webrev.05/
> <http://cr.openjdk.java.net/%7Edtitov/8163083/webrev.05/>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8163083
>
> Thanks!
> --Daniil
>
>
> On 9/24/18, 2:56 PM, "Alex Menkov"
> <alexey.menkov@oracle.com
> <mailto:alexey.menkov@oracle.com>> wrote:
>
> Daniil,
>
> Just remembered that SQE requested to not add new
> tests to vmTestbase
> (see test/hotspot/jtreg/vmTestbase/README.md)
> Could you please move the test to correct location
> (I suppose it's
> test/jdk/com/sun/jdi)
>
> --alex
>
>
> On 09/24/2018 14:30, Alex Menkov wrote:
> > +1
> >
> > --alex
> >
> > On 09/24/2018 10:39, Gary Adams wrote:
> > > Looks good to me.
> > >
> > > On 9/24/18, 1:25 PM, Daniil Titov wrote:
> > > > Hi Alex and Gary,
> > > >
> > > > Please review the updated patch that includes
> suggested changes.
> > > >
> > > >
> http://cr.openjdk.java.net/~dtitov/8163083/webrev.04/
> <http://cr.openjdk.java.net/%7Edtitov/8163083/webrev.04/>
> > > > Bug:
> https://bugs.openjdk.java.net/browse/JDK-8163083
> > > >
> > > > Thanks!
> > > > --Daniil
> > > >
> > > >
> > > >
> > > >
> > > > On 9/21/18, 3:59 PM, "Alex
> Menkov"<alexey.menkov@oracle.com
> <mailto:alexey.menkov@oracle.com>> wrote:
> > > >
> > > > One more note:
> > > > please add "@Override" annotation to the
> > > >
> SocketListeningConnector.updateArgumentMapIfRequired
> > > >
> > > > --alex
> > > >
> > > > On 09/21/2018 02:26,
> gary.adams@oracle.com <mailto:gary.adams@oracle.com>
> wrote:
> > > > > Looks good to me.
> > > > >
> > > > > For the javadoc
> > > > >
> > > > > 72 *<p>
> > > > > 73 * If<code>arguments</code>
> contains addressing
> > > > information. and
> > > > > 74 * only one connection will
> be accepted, the {@link
> > > > #accept accept} method
> > > > > 75 * can be called immediately
> without calling this
> > > > method.
> > > > > 76 *
> > > > > 77 * If the addressing information
> provided
> > > > in<code>arguments</code>
> > > > > implies
> > > > > 78 * the auto detection this
> information might be updated
> > > > with the address
> > > > > 79 * of the started listener.
> > > > >
> > > > > - you could add a<p> tag if you
> want to maintain the
> > > > spacing in the
> > > > > generated javadoc.
> > > > > - I've seen other changesets
> migrating to {@code ..} from
> > > > the older
> > > > > <code>...</code>
> > > > >
> > > > > It would be good to include some
> negative testing.
> > > > > Not sure if it is already covered in
> other tests, e.g.
> > > > >
> > > > > args1 = defaultArguments()
> > > > > startListening(args1) // bound port
> updated
> > > > > startListening(args1) // already
> listening
> > > > >
> > > > >
> > > > > On 9/20/18 10:59 PM, Daniil Titov wrote:
> > > > > > Please review the change that fixes
> the issue in
> > > >
> com.sun.tools.jdi.SocketListenerConnector.startListening()
> method.
> > > > > >
> > > > > > When the argument map passed to
> startListening() methods has
> > > > the port number unspecified or set to zero the
> port is auto detected.
> > > > However, the consequent call of
> startListening() methods with
> > > > unspecified port number fails rather than
> starts a new listener on
> > > > auto detected port. This happens since the
> original argument map
> > > > passed to the startListening() methods is used
> as a key to store the
> > > > mapping to the started listeners.
> > > > > >
> > > > > > The fix ensures that in cases when
> the port is auto detected
> > > > the argument map is updated with the bound
> port number.
> > > > > >
> > > > > > Mach5 vmTestbase_nsk_jdi tests
> successfully passed.
> > > > > >
> > > > > >
> Bug:https://bugs.openjdk.java.net/browse/JDK-8163083
> > > > > >
> Webrev:http://cr.openjdk.java.net/~dtitov/8163083/webrev.02/
> <http://cr.openjdk.java.net/%7Edtitov/8163083/webrev.02/>
> > > > > >
> > > > > > Thanks!
> > > > > > --Daniil
> > > > > >
> > > > > >
> > > > > >
> > > > >
> > > >
> > > >
> > > >
> > >
>
>
>
>
>
> --
>
> Thanks,
>
> Jc
>
>
>
>
>
>
[Attachment #3 (text/html)]
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
It looks good to me.<br>
<br>
Thanks!<br>
Serguei<br>
<br>
<div class="moz-cite-prefix">On 9/27/18 11:50 AM, Daniil Titov
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:350E9BBD-1F19-46F6-A91E-AEAA1D0466D7@oracle.com">
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<meta name="Generator" content="Microsoft Word 15 (filtered
medium)">
<style><!--
/* Font Definitions */
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
{font-family:Consolas;
panose-1:2 11 6 9 2 2 4 3 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0in;
margin-bottom:.0001pt;
font-size:11.0pt;
font-family:"Calibri",sans-serif;}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:purple;
text-decoration:underline;}
pre
{mso-style-priority:99;
mso-style-link:"HTML Preformatted Char";
margin:0in;
margin-bottom:.0001pt;
font-size:10.0pt;
font-family:"Courier New";}
p.msonormal0, li.msonormal0, div.msonormal0
{mso-style-name:msonormal;
mso-margin-top-alt:auto;
margin-right:0in;
mso-margin-bottom-alt:auto;
margin-left:0in;
font-size:11.0pt;
font-family:"Calibri",sans-serif;}
span.HTMLPreformattedChar
{mso-style-name:"HTML Preformatted Char";
mso-style-priority:99;
mso-style-link:"HTML Preformatted";
font-family:Consolas;}
span.new
{mso-style-name:new;}
span.EmailStyle21
{mso-style-type:personal;
font-family:"Calibri",sans-serif;}
span.EmailStyle22
{mso-style-type:personal;
font-family:"Calibri",sans-serif;}
span.EmailStyle23
{mso-style-type:personal-reply;
font-family:"Calibri",sans-serif;}
.MsoChpDefault
{mso-style-type:export-only;
font-size:10.0pt;}
@page WordSection1
{size:8.5in 11.0in;
margin:1.0in 1.0in 1.0in 1.0in;}
div.WordSection1
{page:WordSection1;}
--></style>
<div class="WordSection1">
<p class="MsoNormal">Hi Serguei,<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">The webrev was updated to address all these
comments.<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal"><span lang="ES">Webrev: <a
href="http://cr.openjdk.java.net/%7Edtitov/8163083/webrev.08"
moz-do-not-send="true">http://cr.openjdk.java.net/~dtitov/8163083/webrev.08</a><o:p></o:p></span></p>
<p class="MsoNormal">Bug: <a
href="https://bugs.openjdk.java.net/browse/JDK-8163083"
target="_blank" \
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8163083</a> \
<o:p></o:p></p> <p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">Thanks!<o:p></o:p></p>
<p class="MsoNormal">--Daniil<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<div style="border:none;border-top:solid #B5C4DF
1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b><span
style="font-size:12.0pt;color:black">From: </span></b><span
style="font-size:12.0pt;color:black"><a class="moz-txt-link-rfc2396E" \
href="mailto:serguei.spitsyn@oracle.com"><serguei.spitsyn@oracle.com></a><br> \
<b>Organization: </b>Oracle Corporation<br> <b>Date: </b>Thursday, September 27, \
2018 at 11:33 AM<br> <b>To: </b>Daniil Titov
<a class="moz-txt-link-rfc2396E" \
href="mailto:daniil.x.titov@oracle.com"><daniil.x.titov@oracle.com></a>, JC \
Beyler
<a class="moz-txt-link-rfc2396E" \
href="mailto:jcbeyler@google.com"><jcbeyler@google.com></a><br>
<b>Cc: </b><a class="moz-txt-link-rfc2396E" \
href="mailto:alexey.menkov@oracle.com"><alexey.menkov@oracle.com></a>,
<a class="moz-txt-link-rfc2396E" \
href="mailto:gary.adams@oracle.com"><gary.adams@oracle.com></a>,
<a class="moz-txt-link-rfc2396E" \
href="mailto:serviceability-dev@openjdk.java.net"><serviceability-dev@openjdk.java.net></a><br>
<b>Subject: </b>Re: RFR 8163083: SocketListeningConnector
does not allow invocations with port 0<o:p></o:p></span></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<p class="MsoNormal" style="margin-bottom:12.0pt">Just noticed
one more minor issue:<o:p></o:p></p>
<pre><span class="new">+import java.util.Collection;</span><o:p></o:p></pre>
<pre><span class="new">+import java.util.Iterator;</span><o:p></o:p></pre>
<pre><span class="new">+import \
java.util.LinkedHashMap;</span><o:p></o:p></pre> <p class="MsoNormal" \
style="margin-bottom:12.0pt"><br> It seems the above imports are not really used and \
can be removed.<br>
<br>
Thanks,<br>
Serguei<o:p></o:p></p>
<div>
<p class="MsoNormal">On 9/27/18 11:22 AM, <a
href="mailto:serguei.spitsyn@oracle.com"
moz-do-not-send="true">serguei.spitsyn@oracle.com</a>
wrote:<o:p></o:p></p>
</div>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<p class="MsoNormal" style="margin-bottom:12.0pt">Hi Daniil,<br>
<br>
It looks great, thank you for the update.<br>
I have a couple of more minor comments on the test.<o:p></o:p></p>
<pre> 56 testWithDefaultArgs(connector);<o:p></o:p></pre>
<pre> 57 \
testWithDefaultArgs2(connector);<o:p></o:p></pre>
<pre> 58 \
testWithWildcardPort1(connector);<o:p></o:p></pre>
<pre> 59 \
testWithWildcardPort2(connector);<o:p></o:p></pre> <p class="MsoNormal" \
style="margin-bottom:12.0pt"><br>
Please, rename testWithDefaultArgs to testWithDefaultArgs1<br>
to make naming consistent.<o:p></o:p></p>
<pre> 111 throw new RuntimeException("Test \
testWithDefaultArgsNegative failed. The argument map was not updated with" \
+<o:p></o:p></pre>
<pre> 112 " the bound port \
number.");<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre> 115 // This call should fail since the \
previous the argument map is already updated with the \
port<o:p></o:p></pre>
<pre> 116 // number of the started \
listener<o:p></o:p></pre> <p class="MsoNormal" style="margin-bottom:12.0pt"><br>
Could you, please, re-balance the above line pairs to make
the first line shorter?<br>
<br>
No need in new webrev if you fix the above.<br>
<br>
Thanks,<br>
Serguei<o:p></o:p></p>
<div>
<p class="MsoNormal">On 9/27/18 11:00 AM, Daniil Titov
wrote:<o:p></o:p></p>
</div>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<p class="MsoNormal">Hi Serguei,<o:p></o:p></p>
<p class="MsoNormal"> <o:p></o:p></p>
<p class="MsoNormal">Thank you for reviewing this change.
Initially I understood the whole fragment below (from the
Javadoc for
com.sun.jdi.connect.ListeningConnector.startListening()
method) as a direction for the user how to obtain and
prepare the argument map before passing it in
startListening() method.<o:p></o:p></p>
<p class="MsoNormal"> <o:p></o:p></p>
<pre> 61 * The argument map associates argument name strings \
to instances<o:p></o:p></pre>
<pre> 62 * of {@link Connector.Argument}. The default \
argument map for a<o:p></o:p></pre>
<pre> 63 * connector can be obtained through {@link \
Connector#defaultArguments}.<o:p></o:p></pre>
<pre> 64 * Argument map values can be changed, but map \
entries should not be<o:p></o:p></pre> <pre> 65 * added or \
deleted.<o:p></o:p></pre> <p class="MsoNormal"> <o:p></o:p></p>
<p class="MsoNormal">But I agree that the last sentence
could also mean that the argument map values could be
changes as a result of the method invocation and in this
case the new fragment in the Javadoc is not needed.<o:p></o:p></p>
<p class="MsoNormal"> <o:p></o:p></p>
<p class="MsoNormal">Please review the updated webrev that
does not add new Javadoc fragment and includes other
changes you suggested.<o:p></o:p></p>
<p class="MsoNormal"><span lang="ES"><br>
Webrev: <a
href="http://cr.openjdk.java.net/%7Edtitov/8163083/webrev.07/"
moz-do-not-send="true">http://cr.openjdk.java.net/~dtitov/8163083/webrev.07/</a></span><o:p></o:p></p>
<p class="MsoNormal">Bug: <a
href="https://bugs.openjdk.java.net/browse/JDK-8163083"
target="_blank" \
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8163083</a> \
<o:p></o:p></p> <p class="MsoNormal"> <o:p></o:p></p>
<p class="MsoNormal">Thanks!<o:p></o:p></p>
<p class="MsoNormal">--Daniil<o:p></o:p></p>
<p class="MsoNormal"> <o:p></o:p></p>
<div style="border:none;border-top:solid #B5C4DF
1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b><span
style="font-size:12.0pt;color:black">From: </span></b><span
style="font-size:12.0pt;color:black"><a
href="mailto:serguei.spitsyn@oracle.com"
moz-do-not-send="true"><serguei.spitsyn@oracle.com></a><br>
<b>Organization: </b>Oracle Corporation<br>
<b>Date: </b>Wednesday, September 26, 2018 at 8:12 PM<br>
<b>To: </b>Daniil Titov <a
href="mailto:daniil.x.titov@oracle.com"
moz-do-not-send="true"><daniil.x.titov@oracle.com></a>,
JC Beyler <a href="mailto:jcbeyler@google.com"
moz-do-not-send="true"><jcbeyler@google.com></a><br>
<b>Cc: </b><a href="mailto:alexey.menkov@oracle.com"
moz-do-not-send="true"><alexey.menkov@oracle.com></a>,
<a href="mailto:gary.adams@oracle.com"
moz-do-not-send="true"><gary.adams@oracle.com></a>,
<a href="mailto:serviceability-dev@openjdk.java.net"
moz-do-not-send="true"><serviceability-dev@openjdk.java.net></a><br>
<b>Subject: </b>Re: RFR 8163083:
SocketListeningConnector does not allow invocations
with port 0</span><o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> <o:p></o:p></p>
</div>
<p class="MsoNormal">Hi Daniil,<br>
<br>
It is great that you came up the fix for this issue.<br>
Thanks to Gary for the help too.<br>
<br>
I wonder if we could get away without updating the javadoc<br>
of com/sun/jdi/connect/ListeningConnector.java.<br>
Filing a CSR would not be needed then (simple javadoc
reformatting should not require a CSR).<br>
<br>
So, my question is if this new fragment is really \
needed:<o:p></o:p></p>
<pre><span class="new"> 76 * \
<p></span><o:p></o:p></pre>
<pre><span class="new"> 77 * If the addressing information \
provided in {@code arguments} implies</span><o:p></o:p></pre>
<pre><span class="new"> 78 * the auto detection this \
information might be updated with the address</span><o:p></o:p></pre>
<pre><span class="new"> 79 * of the started \
listener.</span><o:p></o:p></pre> <p class="MsoNormal">The javadoc for \
startListening already has this fragment:<o:p></o:p></p>
<pre> 61 * The argument map associates argument name strings \
to instances<o:p></o:p></pre>
<pre> 62 * of {@link Connector.Argument}. The default \
argument map for a<o:p></o:p></pre>
<pre> 63 * connector can be obtained through {@link \
Connector#defaultArguments}.<o:p></o:p></pre>
<pre> 64 * Argument map values can be changed, but map \
entries should not be<o:p></o:p></pre> <pre> 65 * added or \
deleted.<o:p></o:p></pre> <pre> <o:p></o:p></pre>
<pre>that allows to change the argument map values.<o:p></o:p></pre>
<pre>It looks like, it has to be Okay to add the bound port number \
there.<o:p></o:p></pre> <pre> <o:p></o:p></pre>
<pre>Please, let me know what you think.<o:p></o:p></pre>
<p class="MsoNormal">Some formatting comments to addition to
Jc's comments:<o:p></o:p></p>
<pre><span class="new"> 77 * If the addressing information \
provided in {@code arguments} implies</span><o:p></o:p></pre>
<pre><span class="new"> 78 * the auto detection this \
information might be updated with the address</span><o:p></o:p></pre>
<pre><span class="new"> 79 * of the started \
listener.</span><o:p></o:p></pre> <pre> <o:p></o:p></pre>
<pre> This sentence needs to be split in two:<o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre><span class="new"> 77 * If the addressing information \
provided in {@code arguments} implies</span><o:p></o:p></pre>
<pre><span class="new"> 78 * the auto detection. This \
information might be updated with the address</span><o:p></o:p></pre>
<pre><span class="new"> 79 * of the started \
listener.</span><o:p></o:p></pre> <pre><span class="new">+</span><o:p></o:p></pre>
<pre><span class="new">+ protected void \
updateArgumentMapIfRequired(</span><o:p></o:p></pre> <pre><span class="new">+ \
Map<String, ? extends Connector.Argument> args,TransportService.ListenKey \
listener) {</span><o:p></o:p></pre> <pre><span class="new">+ \
}</span><o:p></o:p></pre> <pre><span class="new">+</span><o:p></o:p></pre>
<pre><span class="new"> The indent has to be 4, not \
8.</span><o:p></o:p></pre> <pre><span class="new"> </span><o:p></o:p></pre>
<pre><span class="new">+ if(isWildcardPort(args)) \
{</span><o:p></o:p></pre>
<pre><span class="new">+ String[] address = \
listener.address().split(":");</span><o:p></o:p></pre>
<pre><span class="new">+ if (address.length > 1) \
{</span><o:p></o:p></pre>
<pre><span class="new">+ \
args.get(ARG_PORT).setValue(address[1]);</span><o:p></o:p></pre>
<pre><span class="new">+ }</span><o:p></o:p></pre>
<pre><span class="new">+ }</span><o:p></o:p></pre>
<pre><span class="new"> </span><o:p></o:p></pre>
<pre><span class="new"> A space is missed after the first \
'if'.</span><o:p></o:p></pre> <pre><span class="new"> </span><o:p></o:p></pre>
<pre><span class="new"> </span><o:p></o:p></pre>
<pre> 50 filter(c \
-><o:p></o:p></pre> <pre> 51 \
c.name().equals("com.sun.jdi.SocketListen")).findFirst().get();<o:p></o:p></pre> <p \
class="MsoNormal" style="margin-bottom:12.0pt"> This is better to be one \
liner.<o:p></o:p></p>
<pre> 57 \
testWithDefaultArgs(connector);<o:p></o:p></pre>
<pre> 58 \
testWithDefaultArgs2(connector);<o:p></o:p></pre>
<pre> 59 \
testWithWildcardPort(connector);<o:p></o:p></pre>
<pre> 60 \
testWithWildcardPort2(connector);<o:p></o:p></pre> <pre> <o:p></o:p></pre>
<pre> A suggestion is to rename above as below to have the names more \
unified:<o:p></o:p></pre> <pre> <o:p></o:p></pre>
<pre> 57 \
testWithDefaultArgs1(connector);<o:p></o:p></pre>
<pre> 58 \
testWithDefaultArgs2(connector);<o:p></o:p></pre>
<pre> 59 \
testWithWildcardPort1(connector);<o:p></o:p></pre>
<pre> 60 \
testWithWildcardPort2(connector);<o:p></o:p></pre> <pre> <o:p></o:p></pre>
<p class="MsoNormal" style="margin-bottom:12.0pt">Thanks,<br>
Serguei<br>
<br>
<br>
<o:p></o:p></p>
<div>
<p class="MsoNormal">On 9/25/18 10:32 AM, Daniil Titov
wrote:<o:p></o:p></p>
</div>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<p class="MsoNormal">HI JC,<o:p></o:p></p>
<p class="MsoNormal"> <o:p></o:p></p>
<p class="MsoNormal">The webrev is updated to address
this. <o:p></o:p></p>
<p class="MsoNormal"> <o:p></o:p></p>
<p class="MsoNormal"><span lang="ES">Webrev: <a
href="http://cr.openjdk.java.net/%7Edtitov/8163083/webrev.06"
moz-do-not-send="true">http://cr.openjdk.java.net/~dtitov/8163083/webrev.06</a></span><o:p></o:p></p>
<p class="MsoNormal">Bug: <a
href="https://bugs.openjdk.java.net/browse/JDK-8163083"
target="_blank" \
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8163083</a><br> <br>
Thanks!<br>
--Daniil<o:p></o:p></p>
<p class="MsoNormal"> <o:p></o:p></p>
<div style="border:none;border-top:solid #B5C4DF
1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b><span
style="font-size:12.0pt;color:black">From: </span></b><span
style="font-size:12.0pt;color:black">JC Beyler <a
href="mailto:jcbeyler@google.com"
moz-do-not-send="true"><jcbeyler@google.com></a><br>
<b>Date: </b>Monday, September 24, 2018 at 8:47 PM<br>
<b>To: </b><a
href="mailto:daniil.x.titov@oracle.com"
\
moz-do-not-send="true"><daniil.x.titov@oracle.com></a><br> <b>Cc: </b><a
href="mailto:alexey.menkov@oracle.com"
moz-do-not-send="true"><alexey.menkov@oracle.com></a>,
<a href="mailto:gary.adams@oracle.com"
moz-do-not-send="true"><gary.adams@oracle.com></a>,
<a href="mailto:serviceability-dev@openjdk.java.net"
\
moz-do-not-send="true"><serviceability-dev@openjdk.java.net></a><br> \
<b>Subject: </b>Re: RFR 8163083: SocketListeningConnector does not allow invocations
with port 0</span><o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">Hi Daniil,<o:p></o:p></p>
<div>
<p class="MsoNormal"> <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">Still looks good to me :)<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">Nit: empty line 83 of the new
test is not required!<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">Jc<o:p></o:p></p>
</div>
</div>
<p class="MsoNormal"> <o:p></o:p></p>
<div>
<div>
<p class="MsoNormal">On Mon, Sep 24, 2018 at 5:54 PM
Daniil Titov <<a
href="mailto:daniil.x.titov@oracle.com"
moz-do-not-send="true">daniil.x.titov@oracle.com</a>>
wrote:<o:p></o:p></p>
</div>
<blockquote style="border:none;border-left:solid #CCCCCC
1.0pt;padding:0in 0in 0in
6.0pt;margin-left:4.8pt;margin-top:5.0pt;margin-right:0in;margin-bottom:5.0pt">
<p class="MsoNormal" style="margin-bottom:12.0pt">Hi
Alex,<br>
<br>
Please review the updated webrev that has new test
moved to test/jdk/comsun/jdi/connect folder.<br>
<br>
Webrev: <a
href="http://cr.openjdk.java.net/%7Edtitov/8163083/webrev.05/"
target="_blank" \
moz-do-not-send="true">http://cr.openjdk.java.net/~dtitov/8163083/webrev.05/</a><br> \
Bug: <a
href="https://bugs.openjdk.java.net/browse/JDK-8163083"
target="_blank" \
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8163083</a><br> <br>
Thanks!<br>
--Daniil<br>
<br>
<br>
On 9/24/18, 2:56 PM, "Alex Menkov" <<a
href="mailto:alexey.menkov@oracle.com"
target="_blank" \
moz-do-not-send="true">alexey.menkov@oracle.com</a>> wrote:<br>
<br>
Daniil,<br>
<br>
Just remembered that SQE requested to not add
new tests to vmTestbase <br>
(see test/hotspot/jtreg/vmTestbase/README.md)<br>
Could you please move the test to correct
location (I suppose it's <br>
test/jdk/com/sun/jdi)<br>
<br>
--alex<br>
<br>
<br>
On 09/24/2018 14:30, Alex Menkov wrote:<br>
> +1<br>
> <br>
> --alex<br>
> <br>
> On 09/24/2018 10:39, Gary Adams wrote:<br>
>> Looks good to me.<br>
>><br>
>> On 9/24/18, 1:25 PM, Daniil Titov
wrote:<br>
>>> Hi Alex and Gary,<br>
>>><br>
>>> Please review the updated patch
that includes suggested changes.<br>
>>><br>
>>> <a
href="http://cr.openjdk.java.net/%7Edtitov/8163083/webrev.04/"
target="_blank" \
moz-do-not-send="true">http://cr.openjdk.java.net/~dtitov/8163083/webrev.04/</a><br> \
>>> Bug: <a
href="https://bugs.openjdk.java.net/browse/JDK-8163083"
target="_blank" \
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8163083</a><br> \
>>><br> >>> Thanks!<br>
>>> --Daniil<br>
>>><br>
>>><br>
>>><br>
>>><br>
>>> On 9/21/18, 3:59 PM, "Alex
Menkov"<<a href="mailto:alexey.menkov@oracle.com"
target="_blank" \
moz-do-not-send="true">alexey.menkov@oracle.com</a>> wrote:<br>
>>><br>
>>> One more note:<br>
>>> please add "@Override"
annotation to the<br>
>>>
SocketListeningConnector.updateArgumentMapIfRequired<br>
>>><br>
>>> --alex<br>
>>><br>
>>> On 09/21/2018 02:26, <a
href="mailto:gary.adams@oracle.com"
target="_blank" \
moz-do-not-send="true">gary.adams@oracle.com</a> wrote:<br>
>>> > Looks good to me.<br>
>>> ><br>
>>> > For the javadoc<br>
>>> ><br>
>>> > 72 *<p><br>
>>> > 73 *
If<code>arguments</code> contains
addressing <br>
>>> information. and<br>
>>> > 74 * only one
connection will be accepted, the {@link <br>
>>> #accept accept} method<br>
>>> > 75 * can be
called immediately without calling this <br>
>>> method.<br>
>>> > 76 *<br>
>>> > 77 * If the addressing
information provided <br>
>>>
in<code>arguments</code><br>
>>> > implies<br>
>>> > 78 * the auto detection
this information might be updated <br>
>>> with the address<br>
>>> > 79 * of the started
listener.<br>
>>> ><br>
>>> > - you could add
a<p> tag if you want to maintain the <br>
>>> spacing in the<br>
>>> > generated javadoc.<br>
>>> > - I've seen other
changesets migrating to {@code ..} from <br>
>>> the older<br>
>>> >
<code>...</code><br>
>>> ><br>
>>> > It would be good to
include some negative testing.<br>
>>> > Not sure if it is
already covered in other tests, e.g.<br>
>>> ><br>
>>> > args1 =
defaultArguments()<br>
>>> >
startListening(args1) // bound port updated<br>
>>> >
startListening(args1) // already listening<br>
>>> ><br>
>>> ><br>
>>> > On 9/20/18 10:59 PM,
Daniil Titov wrote:<br>
>>> >> Please review the
change that fixes the issue in <br>
>>>
com.sun.tools.jdi.SocketListenerConnector.startListening()
method.<br>
>>> >><br>
>>> >> When the argument
map passed to startListening() methods has <br>
>>> the port number unspecified or set
to zero the port is auto detected. <br>
>>> However, the consequent call of
startListening() methods with <br>
>>> unspecified port number fails
rather than starts a new listener on <br>
>>> auto detected port. This happens
since the original argument map <br>
>>> passed to the startListening()
methods is used as a key to store the <br>
>>> mapping to the started listeners.<br>
>>> >><br>
>>> >> The fix ensures that
in cases when the port is auto detected <br>
>>> the argument map is updated with
the bound port number.<br>
>>> >><br>
>>> >> Mach5
vmTestbase_nsk_jdi tests successfully passed.<br>
>>> >><br>
>>> >> Bug:<a
href="https://bugs.openjdk.java.net/browse/JDK-8163083"
target="_blank" \
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8163083</a><br> \
>>> >> Webrev:<a
href="http://cr.openjdk.java.net/%7Edtitov/8163083/webrev.02/"
target="_blank" \
moz-do-not-send="true">http://cr.openjdk.java.net/~dtitov/8163083/webrev.02/</a><br> \
>>> >><br> >>> >> Thanks!<br>
>>> >> --Daniil<br>
>>> >><br>
>>> >><br>
>>> >><br>
>>> ><br>
>>><br>
>>><br>
>>><br>
>><br>
<br>
<br>
<br>
<br>
<o:p></o:p></p>
</blockquote>
</div>
<p class="MsoNormal"><br clear="all">
<o:p></o:p></p>
<div>
<p class="MsoNormal"> <o:p></o:p></p>
</div>
<p class="MsoNormal">-- <o:p></o:p></p>
<div>
<div>
<div>
<p class="MsoNormal"> <o:p></o:p></p>
</div>
<p class="MsoNormal">Thanks,<o:p></o:p></p>
<div>
<p class="MsoNormal">Jc<o:p></o:p></p>
</div>
</div>
</div>
</blockquote>
<p class="MsoNormal"><br>
<br>
<br>
<o:p></o:p></p>
</blockquote>
<p class="MsoNormal"><o:p> </o:p></p>
</blockquote>
<p class="MsoNormal"><br>
<br>
<o:p></o:p></p>
</div>
</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