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.spit...@oracle.com>
*Organization: *Oracle Corporation
*Date: *Wednesday, September 26, 2018 at 8:12 PM
*To: *Daniil Titov <daniil.x.ti...@oracle.com>, JC Beyler
<jcbey...@google.com>
*Cc: *<alexey.men...@oracle.com>, <gary.ad...@oracle.com>,
<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 <jcbey...@google.com> <mailto:jcbey...@google.com>
*Date: *Monday, September 24, 2018 at 8:47 PM
*To: *<daniil.x.ti...@oracle.com> <mailto:daniil.x.ti...@oracle.com>
*Cc: *<alexey.men...@oracle.com>
<mailto:alexey.men...@oracle.com>, <gary.ad...@oracle.com>
<mailto:gary.ad...@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.ti...@oracle.com <mailto:daniil.x.ti...@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.men...@oracle.com
<mailto:alexey.men...@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.men...@oracle.com
<mailto:alexey.men...@oracle.com>> wrote:
>>>
>>> One more note:
>>> please add "@Override" annotation to the
>>> SocketListeningConnector.updateArgumentMapIfRequired
>>>
>>> --alex
>>>
>>> On 09/21/2018 02:26, gary.ad...@oracle.com
<mailto:gary.ad...@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
