RFR 8163083: SocketListeningConnector does not allow invocations with port 0
Alex Menkov
alexey.menkov at oracle.com
Tue Sep 25 18:08:35 UTC 2018
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 <jcbeyler at google.com>
> *Date: *Monday, September 24, 2018 at 8:47 PM
> *To: *<daniil.x.titov at oracle.com>
> *Cc: *<alexey.menkov at oracle.com>, <gary.adams at oracle.com>,
> <serviceability-dev at 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.titov at oracle.com
> <mailto: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/
> <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.menkov at oracle.com
> <mailto: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/
> <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.menkov at oracle.com <mailto: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
> <mailto: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/
> <http://cr.openjdk.java.net/%7Edtitov/8163083/webrev.02/>
> >>> >>
> >>> >> Thanks!
> >>> >> --Daniil
> >>> >>
> >>> >>
> >>> >>
> >>> >
> >>>
> >>>
> >>>
> >>
>
>
>
> --
>
> Thanks,
>
> Jc
>
More information about the serviceability-dev
mailing list