RFR: 8226575: OperatingSystemMXBean should be made container aware

[Daniil Titov]

Hi Mandy and Bob,

Please review a new version of the webrev [1] that moves doPrivileged calls in 
jdk.internal.platform.cgroupv1.SubSystem to separate methods that throw
 IOException, as Mandy suggested.

Mach5 tests are still running.


[1] Webrev: http://cr.openjdk.java.net/~dtitov/8226575/webrev.06/ 
[2] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8226575      
[3] CSR: https://bugs.openjdk.java.net/browse/JDK-8228428 

Thank you,
Daniil

On 12/9/19, 1:55 PM, "Mandy Chung" <mandy.chung at oracle.com> wrote:

    
    
    On 12/9/19 10:51 AM, Daniil Titov wrote:
    > Hi Mandy and Bob,
    >
    >> Why did you not change the exception caught in SubSystem.java:getStringValue to PrivilegedActionException from IOException
    >> so it’s consistent with the other get functions?
    > In this method both Files.newBufferedReader and return bufferedReader.readLine could throw IOException so for simplicity I just put
    > the whole code block in doPrivileged. On the other side I don't believe that BufferedReader.readline() requires  FilePermission checks ( and tests proved that)
    > so we could change this implementation to the following:
    >
    >      public static String getStringValue(SubSystem subsystem, String parm) {
    >          if (subsystem == null) return null;
    >
    >         try (BufferedReader bufferedReader =
    >                       AccessController.doPrivileged((PrivilegedExceptionAction<BufferedReader>) () -> {
    >                           return Files.newBufferedReader(Paths.get(subsystem.path(), parm));
    >                       })) {
    >              return bufferedReader.readLine();
    >          } catch (PrivilegedActionException | IOException  e) {
    >              return null;
    >          }
    >      }
    >
    > Could you please advise are you OK with it or you would like to proceed with the approach Mandy suggested to unwrap
    > PrivilegedActionException exception and throw the cause instead?
    >
    
    I think it's simpler to read and understand if the doPrivileged call is 
    moved out as a separate method that will throw IOException as the 
    expected functionality as suggested above.
    
    For SubSystem::getStringValue, one suggestion would be:
    
    diff --git 
    a/src/java.base/linux/classes/jdk/internal/platform/cgroupv1/SubSystem.java 
    b/src/java.base/linux/classes/jdk/internal/platform/cgroupv1/SubSystem.java
    --- 
    a/src/java.base/linux/classes/jdk/internal/platform/cgroupv1/SubSystem.java
    +++ 
    b/src/java.base/linux/classes/jdk/internal/platform/cgroupv1/SubSystem.java
    @@ -29,7 +29,11 @@
      import java.io.IOException;
      import java.math.BigInteger;
      import java.nio.file.Files;
    +import java.nio.file.Path;
      import java.nio.file.Paths;
    +import java.security.AccessController;
    +import java.security.PrivilegedActionException;
    +import java.security.PrivilegedExceptionAction;
      import java.util.ArrayList;
      import java.util.List;
      import java.util.Optional;
    @@ -90,9 +94,8 @@
          public static String getStringValue(SubSystem subsystem, String 
    parm) {
              if (subsystem == null) return null;
    
    -        try(BufferedReader bufferedReader = 
    Files.newBufferedReader(Paths.get(subsystem.path(), parm))) {
    -            String line = bufferedReader.readLine();
    -            return line;
    +        try {
    +            return subsystem.readStringValue(parm);
              }
              catch (IOException e) {
                  return null;
    @@ -100,6 +103,24 @@
    
          }
    
    +    private String readStringValue(String param) throws IOException {
    +        PrivilegedExceptionAction<BufferedReader> pea = () -> 
    Files.newBufferedReader(Paths.get(path(), param));
    +        try (BufferedReader bufferedReader = 
    AccessController.doPrivileged(pea)) {
    +            String line = bufferedReader.readLine();
    +            return line;
    +        } catch (PrivilegedActionException e) {
    +            Throwable x = e.getCause();
    +            if (x instanceof IOException)
    +                throw (IOException)x;
    +            if (x instanceof RuntimeException)
    +                throw (RuntimeException)x;
    +            if (x instanceof Error)
    +                throw (Error)x;
    +
    +            throw new InternalError(x);
    +        }
    +    }
    +