Re: RFR 8163083: SocketListeningConnector does not allow invocations with port 0

[Alex Menkov]

Looks good.

--alex

On 09/25/2018 10:32, 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