+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/
Bug: https://bugs.openjdk.java.net/browse/JDK-8163083
Thanks!
--Daniil
On 9/21/18, 3:59 PM, "Alex Menkov"<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 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/
>>
>> Thanks!
>> --Daniil
>>
>>
>>
>
