Hi Serguei,

>> How these changes are going to work for options "--module=" and 
>> "--module-path"
>> if there is this check at the beginning of each iteration:
>> parts[i].equals("--module")

Let me describe in details these cases you mentioned.

Case 1:
---------
   part[i] is "--module"
   part[i+1] is "myModule/MyClass"

Case 2:
---------
  part[i] is "--module=myModule/MyClass "

Case 3:
---------
   part[i] is "--module-path"
   part[i+1] is "mods"

For case 1 we have the condition on line 94 that is evaluated to TRUE and we 
return the next part on line 95  (part[i+1]) as a module and class name.

For case 2 the condition on line 94 is evaluated to FALSE so we continue to 
line 99. On line 99 the condition is evaluated to TRUE so on line 100 we 
return the value part of the option (everything after "--module=") as a module 
and class name. Please note the condition on line 94 uses 
equals() while on the line 99 it uses startsWith().

For case 3 the condition on lines 94 and 99 are evaluated to FALSE so we 
continue to lines 105/106 and  isModuleWhiteSpaceOption(parts[i]) on line 106
returns TRUE. As a result, we skip this and the next parts and continue to the 
beginning of the loop at line 92.





  92            for (int i = 1; i < parts.length && mainClass == null; i++) {
    93              if (i < parts.length - 1) {
    94                  if (parts[i].equals("-m") || 
parts[i].equals("--module") || parts[i].equals("-jar")) {
    95                      return parts[i + 1];
    96                  }
    97              }
    98
    99              if ((parts[i].startsWith("--module="))) {
   100                  return parts[i].substring("--module=".length());
   101              }
   102
   103              // If this is a classpath or a module whitespace option 
then skip the next part
   104              // (the classpath or the option value itself)
   105              if (parts[i].equals("-cp") || parts[i].equals("-classpath") 
||  parts[i].equals("--class-path") ||
   106                      isModuleWhiteSpaceOption(parts[i])) {
   107                  i++;
   108                  continue;
   109              }
   110             


>> Also, this line has unneeded extra "()":  
>> +   if ((parts[i].startsWith("--module="))) {

Yes, you are right! Thanks! I will remove it before pushing or in the new 
version of a webrev if we will come to it.


Thank you!
-Daniil

From: "serguei.spit...@oracle.com" <serguei.spit...@oracle.com>
Date: Wednesday, June 12, 2019 at 12:39 PM
To: Daniil Titov <daniil.x.ti...@oracle.com>, David Holmes 
<david.hol...@oracle.com>
Cc: OpenJDK Serviceability <serviceability-dev@openjdk.java.net>
Subject: Re: FW: 8225543: Jcmd fails to attach to the Java process on Linux 
using the main class name if whitespace options were used to launch the process

Hi Daniil,

I'm confused with the change:
         for (int i = 1; i < parts.length && mainClass == null; i++) {
             if (i < parts.length - 1) {
                 if (parts[i].equals("-m") || parts[i].equals("--module") || 
parts[i].equals("-jar")) {  <= ??????
                     return parts[i + 1];
                 }
             }
-            // If this is a classpath or a module path option then skip the 
next part
-            // (the classpath or the module path itself)
+
+            if ((parts[i].startsWith("--module="))) {                      <= 
??????
+                return parts[i].substring("--module=".length());
+            }
+
+            // If this is a classpath or a module whitespace option then skip 
the next part
+            // (the classpath or the option value itself)
             if (parts[i].equals("-cp") || parts[i].equals("-classpath") ||  
parts[i].equals("--class-path") ||
-                    parts[i].equals("-p") || parts[i].equals("--module-path")) 
{
+                    isModuleWhiteSpaceOption(parts[i])) {

. . .

+    private static boolean isModuleWhiteSpaceOption(String option) {
+        return option.equals("-p") ||
+                option.equals("--module-path") || 

How these changes are going to work for options "--module=" and "--module-path"
if there is this check at the beginning of each iteration:
  parts[i].equals("--module")
?

Also, this line has unneeded extra "()":
  
+            if ((parts[i].startsWith("--module="))) {


Thanks,
Serguei 

On 6/12/19 10:50, Daniil Titov wrote:
Hi David and Serguei,

Could you please have a look at this change? This fix is related with the 
changes [3] that you reviewed back in February.

Mach5 sun/tools/jcmd, serviceability/dcmd/framework, jdk_svc, 
hotspot_serviceability, tier1, tier2 and tier3 tests passed.

Thanks a lot!
--Daniil

On 6/10/19, 4:56 PM, "serviceability-dev on behalf of Daniil Titov" 
mailto:serviceability-dev-boun...@openjdk.java.netonbehalfofdaniil.x.ti...@oracle.com
 wrote:

    Please review the change that fixes jcmd process name matching on Linux 
platform.
    
    After changes [3] , on Linux platform the proc filesystem is used to find a 
Java process, however, sun.tools.ProcessHelper  class, 
    introduced in that change, didn't take into account the presence of module 
whitespace options ( e.g. --patch-module) or the 
    fact that some values of Java options could contain whitespaces.
    
    The latter problem is fixed by keeping '\0' as a separator for command line 
arguments read from /proc/{pid}/cmdline rather than 
    replacing it with a whitespace. The former problem is fixed by making 
sun.tools.ProcessHelper  aware of these module whitespace
     options (--add-opens, --add-exports,--add-reads, --add-modules, 
--patch-module,--limit-modules, or --upgrade-module-path) as well
     as of the case when a source-file mode is used or when --module options is 
used in "--<name>=<value>" format.
    
    Testing: sun/tools/jcmd and serviceability/dcmd/framework tests succeeded 
in Mach5. jdk_svc, tier1, tier2, and tier3 tests are in progress. 
    
    
    [1] Webrev: http://cr.openjdk.java.net/~dtitov/8225543/webrev.01/ 
    [2] Bug: https://bugs.openjdk.java.net/browse/JDK-8225543 
    [3]  https://bugs.openjdk.java.net/browse/JDK-8205654 
    
    Thanks!
    --Daniil
    
    
    
    






Reply via email to