Hi, I updated the parsing logic for extracting port number in test case. Here is the updated webrev :
http://cr.openjdk.java.net/~asapre/webrev/2017/JDK-8167337/webrev.01/ Thanks, Amit > -----Original Message----- > From: Amit Sapre > Sent: Wednesday, January 18, 2017 2:09 PM > To: Roger Riggs; serviceability-dev@openjdk.java.net; Harsha Wardhana B > Subject: RE: RFR : JDK-8167337 - When jmxremote.port=0, JDP broadcasts > "0" instead of assigned port > > 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. > > >>> > > >> > > > > >
