Hello,
Looks like basic check on Jdp packet includes a check for JMX_SERVICE_URL
https://java.se.oracle.com/source/xref/jdk9-dev/jdk/test/sun/management/jdp/JdpTestCase.java#184
I feel we don't need any further check on jmx service url.
> -----Original Message-----
> From: Roger Riggs
> Sent: Tuesday, January 17, 2017 10:05 PM
> To: serviceability-dev@openjdk.java.net
> Subject: Re: RFR : JDK-8167337 - When jmxremote.port=0, JDP broadcasts
> "0" instead of assigned port
>
> Hi,
>
> yes, but the pattern looks for the ":" before and the "/" after the
> zero.
> It would not match the port ":000000/" ; in this test code the URL is
> assumed/known to be relatively well formed.
>
> Roger
I kept the focus on what needs to be tested. This however doesn't mean we
shouldn't validate the url format.
I would prefer to do that in a separate test case all together.
>
>
> On 1/17/2017 11:26 AM, Harsha Wardhana B wrote:
> > Hi Roger,
> >
> > Your approach is more elegant. However checking for ":0/" may not
> work
> > as we can have non-zero port number that can end in 0.
> >
> > Regards
> >
> > Harsha
> >
> >
> > On Tuesday 17 January 2017 09:39 PM, Roger Riggs wrote:
> >> Hi Harsha,
> >>
> >> On 1/16/2017 1:21 AM, Harsha Wardhana B wrote:
> >>> Hi Amit,
> >>>
> >>> In JdpJmxRemoteDynamicPortTestCase:48 needs null/empty check for
> jmx
> >>> url.
> >>>
> >>> JdpJmxRemoteDynamicPortTestCase:49, array length needs to checked
> >>> before accessing index at token[6].
> >>>
> >>> It is possible that port number need not always be present at given
> >>> index and hence we may have to follow different approach to extract
> >>> port number. Please check if approach below works.
> >>>
> >>> <code>
> >>>
> >>> int idx = jmxurl.indexOf(':');
> >>> while (idx != -1) {
> >>> jmxurl = jmxurl.substring(idx+1);
> >>> idx = jmxurl.indexOf(':');
> >>> }
> >> This loop would very eagerly find the last ":" in the string even it
> >> was well past the host/port field.
> >> String.lastIndex would be equivalent.
> >>>
> >>> if(jmxurl.indexOf('/') == -1) {
> >>> throw new RuntimeException("Test failed : Invalid
> >>> JMXServiceURL");
> >>> }
> >> It would be more efficient to compare the index of the '/' after the
> >> last ":" than to re-create new substrings.
> >> int colon = jmxurl.lastIndexOf(':');
> >> int slash = jmxurl.indexOf('/', colon); int port =
> >> Integer.parseInt(jmxurl, colon + 1, slash, 10);
> >>
> >>>
> >>> String portStr = jmxurl.substring(0,jmxurl.indexOf('/'));
> >>> int port = Integer.parseInt(portStr);
> >>> if( port == 0 ) {
> >>> throw new RuntimeException("Test failed : Zero port for
> >>> jmxremote");
> >>> }
> >> Or It might be just as effective to just to check if ":0/" is
> present.
> >> if (jmxurl.contains(":0/")) {...}
> >>
> >> $.02, Roger
> >>
> >>
> >>>
> >>> </code>
> >>>
> >>> Regards
> >>>
> >>> Harsha
> >>>
> >>>
> >>> On Monday 16 January 2017 11:16 AM, Amit Sapre wrote:
> >>>> Thanks Dmitry for the review.
> >>>>
> >>>> Can I have one more reviewer for this fix ?
> >>>>
> >>>> Thanks,
> >>>> Amit
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: Dmitry Samersoff
> >>>>> Sent: Sunday, January 15, 2017 4:49 PM
> >>>>> To: Amit Sapre; serviceability-dev
> >>>>> Subject: Re: RFR : JDK-8167337 - When jmxremote.port=0, JDP
> >>>>> broadcasts "0" instead of assigned port
> >>>>>
> >>>>> Amit,
> >>>>>
> >>>>> Changes looks good to me.
> >>>>>
> >>>>> -Dmitry
> >>>>>
> >>>>>
> >>>>> On 2017-01-13 09:17, Amit Sapre wrote:
> >>>>>> Hello,
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> Please review the fix for JDK-8167337
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> Bug Id : https://bugs.openjdk.java.net/browse/JDK-8167337
> >>>>>>
> >>>>>> Webrev :
> >>>>>> http://cr.openjdk.java.net/~asapre/webrev/2017/JDK-
> 8167337/webrev
> >>>>>> .00/
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> Thanks,
> >>>>>>
> >>>>>> Amit
> >>>>>>
> >>>>>
> >>>>> --
> >>>>> Dmitry Samersoff
> >>>>> Oracle Java development team, Saint Petersburg, Russia
> >>>>> * I would love to change the world, but they won't give me the
> >>>>> sources.
> >>>
> >>
> >
>
