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
>>
>>
>>
>
