RFR 9: 8048124 Read hijrah-config-umalqura.properties as a resource
Please review for JDK 9 only. To aid the modularization effort, the configuration of the Hijrah calendar should move the Hijrah calendar data to a resource. At this point, it does not look like there will be other Hijrah calendar variants; the function of calendar.properties to configure variants is unnecessary and is proposed to be removed. Since the other uses of calendars.properties have been eliminated the calendars.properties is removed. Webrev: http://cr.openjdk.java.net/~rriggs/webrev-hijrah-config-8049376/ Issue: https://bugs.openjdk.java.net/browse/JDK-8048124 Thanks, Roger
On 20/10/2014 15:53, roger riggs wrote:
Please review for JDK 9 only.
To aid the modularization effort, the configuration of the Hijrah calendar should move the Hijrah calendar data to a resource.
At this point, it does not look like there will be other Hijrah calendar variants; the function of calendar.properties to configure variants is unnecessary and is proposed to be removed.
Since the other uses of calendars.properties have been eliminated the calendars.properties is removed.
Webrev: http://cr.openjdk.java.net/~rriggs/webrev-hijrah-config-8049376/ Issue: https://bugs.openjdk.java.net/browse/JDK-8048124 This mostly looks good to me. I just wonder about the removal of the doPrivileged in readConfigProperties where you will get null if there is something on the stack with restricted permissions.
A minor comment on the @implNote in HijrahChronology is that it has more than I would expect, it might be simpler to just say that hijrah-config-<calendar-type>.properties is loaded as a resource file. Also the import of java.lang.String looks unnecessary. -Alan.
Hi Alan, Thanks for the review... On 10/20/2014 11:07 AM, Alan Bateman wrote:
On 20/10/2014 15:53, roger riggs wrote:
Please review for JDK 9 only.
To aid the modularization effort, the configuration of the Hijrah calendar should move the Hijrah calendar data to a resource.
At this point, it does not look like there will be other Hijrah calendar variants; the function of calendar.properties to configure variants is unnecessary and is proposed to be removed.
Since the other uses of calendars.properties have been eliminated the calendars.properties is removed.
Webrev: http://cr.openjdk.java.net/~rriggs/webrev-hijrah-config-8049376/ Issue: https://bugs.openjdk.java.net/browse/JDK-8048124 This mostly looks good to me. I just wonder about the removal of the doPrivileged in readConfigProperties where you will get null if there is something on the stack with restricted permissions. What permission would be needed to read the resource? The limited doPrivileged should include the minimum permission.
A minor comment on the @implNote in HijrahChronology is that it has more than I would expect, it might be simpler to just say that hijrah-config-<calendar-type>.properties is loaded as a resource file. Also the import of java.lang.String looks unnecessary. ok, will correct.
Roger
-Alan.
On 20/10/2014 16:11, roger riggs wrote:
What permission would be needed to read the resource? The limited doPrivileged should include the minimum permission. The resources will be be resources.jar so I think read access to that should be sufficient. If you run a small test with -Djava.security.manager that triggers the config file to load then it would help verify that. When we move to the modular image then the resources will be elsewhere in the runtime image so if you really want to use limited doPrivileged here then access to ${java.home}/** should do it. Of course not using limited doPrivileged is a possibility too.
-Alan
Hi, Updated with recommendations. Webrev: http://cr.openjdk.java.net/~rriggs/webrev-hijrah-config-8049376/ Issue: https://bugs.openjdk.java.net/browse/JDK-8048124 Thanks, Roger On 10/20/2014 11:25 AM, Alan Bateman wrote:
On 20/10/2014 16:11, roger riggs wrote:
What permission would be needed to read the resource? The limited doPrivileged should include the minimum permission. The resources will be be resources.jar so I think read access to that should be sufficient. If you run a small test with -Djava.security.manager that triggers the config file to load then it would help verify that. When we move to the modular image then the resources will be elsewhere in the runtime image so if you really want to use limited doPrivileged here then access to ${java.home}/** should do it. Of course not using limited doPrivileged is a possibility too.
-Alan
Hi, A minor issue: there are quite a few instances of parsing integers via Integer.valueOf(String) instead just Integer.parseInt(String): HijrahChronology.java: 864 int year = Integer.valueOf(key); 972 months[i] = Integer.valueOf(numbers[i]); Those should be optimized not to create substrings either 994 ymd[0] = Integer.valueOf(string.substring(0, 4)); 995 ymd[1] = Integer.valueOf(string.substring(5, 7)); 996 ymd[2] = Integer.valueOf(string.substring(8, 10)); Stanimir On Mon, Oct 20, 2014 at 9:17 PM, roger riggs <roger.riggs@oracle.com> wrote:
Hi,
Updated with recommendations.
Webrev: http://cr.openjdk.java.net/~rriggs/webrev-hijrah-config-8049376/ Issue: https://bugs.openjdk.java.net/browse/JDK-8048124
Thanks, Roger
On 10/20/2014 11:25 AM, Alan Bateman wrote:
On 20/10/2014 16:11, roger riggs wrote:
What permission would be needed to read the resource? The limited doPrivileged should include the minimum permission.
The resources will be be resources.jar so I think read access to that should be sufficient. If you run a small test with -Djava.security.manager that triggers the config file to load then it would help verify that. When we move to the modular image then the resources will be elsewhere in the runtime image so if you really want to use limited doPrivileged here then access to ${java.home}/** should do it. Of course not using limited doPrivileged is a possibility too.
-Alan
On 20/10/2014 19:17, roger riggs wrote:
Hi,
Updated with recommendations.
Webrev: http://cr.openjdk.java.net/~rriggs/webrev-hijrah-config-8049376/ Issue: https://bugs.openjdk.java.net/browse/JDK-8048124
"<<ALL FILES>>" seems excessive when I think you only need ${java.home}/- but still better than all. A minor comment is that expression for the try-with-resources uses AccessController with parameters over 4 lines isn't easy to read. It might be a bit clearer with multiple statements, as in: PrivilegedAction<InputStream> action = () -> HijrahChronology .class.getResourceAsStream(resourceName); Permission permission = new FilePermission("<<ALL FILES>>", "read"); try (InputStream in = AccessController.doPrivileged(action, null, permission)) { : }
Hi, Updated with recommendations to make the code more readable and to use the more appropriate Integer.parseInt instead of valueOf. Webrev: http://cr.openjdk.java.net/~rriggs/webrev-hijrah-config-8049376/ Issue: https://bugs.openjdk.java.net/browse/JDK-8048124 Roger On 10/20/2014 2:53 PM, Alan Bateman wrote:
On 20/10/2014 19:17, roger riggs wrote:
Hi,
Updated with recommendations.
Webrev: http://cr.openjdk.java.net/~rriggs/webrev-hijrah-config-8049376/ Issue: https://bugs.openjdk.java.net/browse/JDK-8048124
"<<ALL FILES>>" seems excessive when I think you only need ${java.home}/- but still better than all. yes, reading the java.home property required a doPriv and then a nested doPriv with the properly constructed FilePermission. The code was getting unwieldy.
A minor comment is that expression for the try-with-resources uses AccessController with parameters over 4 lines isn't easy to read. It might be a bit clearer with multiple statements, as in:
PrivilegedAction<InputStream> action = () -> HijrahChronology .class.getResourceAsStream(resourceName); Permission permission = new FilePermission("<<ALL FILES>>", "read"); try (InputStream in = AccessController.doPrivileged(action, null, permission)) { : } Yes, easier to read and parse.
Thanks, Roger
On 20/10/2014 20:11, roger riggs wrote:
Hi,
Updated with recommendations to make the code more readable and to use the more appropriate Integer.parseInt instead of valueOf.
Webrev: http://cr.openjdk.java.net/~rriggs/webrev-hijrah-config-8049376/ Issue: https://bugs.openjdk.java.net/browse/JDK-8048124 This looks okay now although I guess the other clean-ups are really something for a different issue.
You want to want to check Integer.java in the webrev, I assumed that isn't meant to be there. -Alan.
participants (3)
-
Alan Bateman
-
roger riggs
-
Stanimir Simeonoff