Thank you, JC and David, for reviewing this change!
Best regards, -Daniil From: Jean Christophe Beyler <[email protected]> Date: Friday, April 5, 2019 at 9:40 AM To: David Holmes <[email protected]> Cc: Daniil Titov <[email protected]>, OpenJDK Serviceability <[email protected]>, Thomas Stüfe <[email protected]> Subject: Re: 8221730: jcmd process name matching broken Hi Daniil, Looks good to me too :) Jc On Thu, Apr 4, 2019 at 9:05 PM David Holmes <[email protected]> wrote: Great! Thanks, David On 5/04/2019 2:00 pm, Daniil Titov wrote: > Hi David, > > I figured out what went wrong. I used webrev.ksh with the list of files to > diff. Using "webrev.ksh -m -N" solved the problem. The new webrev is > uploaded as webrev.04. > > Webrev: http://cr.openjdk.java.net/~dtitov/8221730/webrev.04 > Bug: https://bugs.openjdk.java.net/browse/JDK-8221730 > > Thanks, > Daniil > > On 4/4/19, 7:15 PM, "David Holmes" <[email protected]> wrote: > > Hi Daniil, > > On 5/04/2019 12:01 pm, Daniil Titov wrote: > > Hi David, > > > > I didn't use "hg rename". Now I recreated the patch using "hg rename" > for moving > test/hotspot/jtreg/serviceability/dcmd/framework/TestJavaProcess.java to > test/hotspot/jtreg/serviceability/dcmd/framework/process/TestJavaProcess.java. > I also used the latest version of webrev.ksh (25.17) to re-create a webrev > (I uploaded it as webrev.03). However, I still don't see that renaming is > somehow reflected in the webrev. Not sure what might be wrong here. The hg > version is 4.5.3, the OS is Ubuntu 18.04.2 LTS. > > Strange. Here's an example of where the rename shows up in the webrev: > > http://cr.openjdk.java.net/~dholmes/8213233/webrev/ > > Anyway as long as it was done with hg rename that is fine. > > Thanks, > David > ---- > > > > > Webrev: http://cr.openjdk.java.net/~dtitov/8221730/webrev.03/ > > Bug: https://bugs.openjdk.java.net/browse/JDK-8221730 > > > > Thanks! > > > > Best regards, > > Daniil > > > > On 4/4/19, 5:45 PM, "David Holmes" <[email protected]> wrote: > > > > Hi Daniil, > > > > On 5/04/2019 8:17 am, Daniil Titov wrote: > > > Hi David and JC, > > > > > > Thank you for reviewing this change. Please review a new > version of > > > the fix that adds additional tests as David suggested. The > tests are > > > added to > > > > test/hotspot/jtreg/serviceability/dcmd/framework/VMVersionTest.java and > > > test the cases when Java process is started with -jar or -m > (--module) > > > options. > > > > Looks good - thanks for doing that. > > > > > Since unnamed packages are not allowed in the modules, file > > > > test/hotspot/jtreg/serviceability/dcmd/framework/TestJavaProcess.java > > > was moved to > > > > test/hotspot/jtreg/serviceability/dcmd/framework/process/TestJavaProcess.java. > > > > Did you use "hg rename" for that? The webrev doesn't show that > you did. > > > > Thanks, > > David > > > > > > > > Webrev: http://cr.openjdk.java.net/~dtitov/8221730/webrev.02/ > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8221730 > > > > > > Thanks! > > > --Daniil > > > > > > > > > On 4/3/19, 5:23 PM, "David Holmes" <[email protected]> > wrote: > > > > > > Hi Daniil, > > > > > > This seems reasonable, but can we add a suitable > regression test please > > > to verify behaviour before JDK-8205654 and with this fix. > > > > > > Thanks, > > > David > > > > > > On 4/04/2019 5:02 am, Daniil Titov wrote: > > > > Please review the change that makes jcmd process name > matching on Linux platform consistent with pre-existing behavior. > > > > > > > > On other platforms (and on Linux platform before changes > [3]) the jcmd uses system property "sun.rt.javaCommand" (see > sun.jvmstat.monitor. MonitoredVmUtil.mainClass(MonitoredVm, boolean) that is > called from sun.tools.common. ProcessArgumentMatcher at line 96) and treats > the part before the first space as a main class when matching the process > name. However, if the application is started with -jar option this part > contains the path to the jar file. If -m or --module option is used this > part contains the module name and the main class (if the main class was > specified in the command line) in the format <modulename>/<mainclass>. After > changes [3] , on Linux platform the proc filesystem is used to find a Java > process, however, it always matches the process name against the main class > regardless what options were used to launch the application. This created > discrepancies between old and new behavior on Linux platform as well as > between behavior on Linux and other platforms. Th! > > > > e fix changes sun.tool.ProcessHelper (that was > introduced in [3]) to correct these discrepancies. > > > > > > > > > > > > Reference: > > > > [1] Webrev: > http://cr.openjdk.java.net/~dtitov/8221730/webrev.01 > > > > [2] Bug: https://bugs.openjdk.java.net/browse/JDK-8221730 > > > > [3] https://bugs.openjdk.java.net/browse/JDK-8205654 > > > > > > > > > > > > Thanks! > > > > --Daniil > > > > > > > > > > > > > > > > > > > > > > > > > > -- Thanks, Jc
