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>
*Date: *Monday, September 24, 2018 at 8:47 PM
*To: *<daniil.x.ti...@oracle.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,
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
