RFR 8163083: SocketListeningConnector does not allow invocations with port 0

[JC Beyler]

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 at 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/
> Bug: https://bugs.openjdk.java.net/browse/JDK-8163083
>
> Thanks!
> --Daniil
>
>
> On 9/24/18, 2:56 PM, "Alex Menkov" <alexey.menkov at 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/
>     >>> Bug: https://bugs.openjdk.java.net/browse/JDK-8163083
>     >>>
>     >>> Thanks!
>     >>> --Daniil
>     >>>
>     >>>
>     >>>
>     >>>
>     >>> On 9/21/18, 3:59 PM, "Alex Menkov"<alexey.menkov at oracle.com>
> wrote:
>     >>>
>     >>>      One more note:
>     >>>      please add "@Override" annotation to the
>     >>>      SocketListeningConnector.updateArgumentMapIfRequired
>     >>>
>     >>>      --alex
>     >>>
>     >>>      On 09/21/2018 02:26, gary.adams at 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
>     >>>      >>
>     >>>      >>
>     >>>      >>
>     >>>      >
>     >>>
>     >>>
>     >>>
>     >>
>
>
>
>

-- 

Thanks,
Jc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20180924/a9cc0015/attachment.html>