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