Hi, Mandy.
Thanks for looking at this webrev again.
> 273 current = parse(System.getProperty("java.version"));
>
> System.getProperty will do a permission check and it needs to be called
> within a doPrivileged block.
Nice catch! I've added the permission check and the associated @throws.
> 154 * @see <a href="http://openjdk.java.net/jeps/223";>JEP 223: New
> Version-String Scheme</a>
>
> Does the javadoc have the essential information from this JEP? Wonder if
> this @see is necessary.
I thought it might be nice to include the JEP reference as it contains
additional
data about versioning beyond the JavaDoc for this method; however, it's not
critical to the understanding or use of this class.
For some reason I'd thought that there was precedence in including JEP numbers
as @see references, but I find no evidence of that. I've removed the @see.
These are the diffs to address both of your comments:
$ diff Version.java_save Version.java
28a29,30
> import java.security.AccessController;
> import java.security.PrivilegedAction;
154,155d155
< * @see <a href="http://openjdk.java.net/jeps/223";>JEP 223: New
Version-String Scheme</a>
< *
268a269,274
> * @throws SecurityException
> * If a security manager exists and its {@link
> * SecurityManager#checkPropertyAccess(String)
> * checkPropertyAccess} method does not allow access to the
> * system property "java.version"
> *
272,273c278,285
< if (current == null)
< current = parse(System.getProperty("java.version"));
---
> if (current == null) {
> current = parse(AccessController.doPrivileged(
> new PrivilegedAction<>() {
> public String run() {
> return System.getProperty("java.version");
> }
> }));
> }
Regards,
iris
-----Original Message-----
From: Mandy Chung
Sent: Tuesday, January 12, 2016 2:26 PM
To: Iris Clark
Cc: Mandy Chung; Joe Darcy; Magnus Ihse Bursie; Roger Riggs; Alan Bateman;
core-libs-dev; verona-...@openjdk.java.net
Subject: Re: RFR: 8072379: Implement jdk.Version and jdk.OracleVersion
> On Jan 11, 2016, at 1:44 PM, Iris Clark <iris.cl...@oracle.com> wrote:
>
> Hi, Joe, Roger, Alan, Magnus, and Mandy.
>
> At the end of December (shortly before the Christmas/Winter break and
> my vacation), I provided responses to your messages and an updated
> webrev:
>
> http://cr.openjdk.java.net/~iris/verona/8072379/webrev.2/
It’s good to see that jdk.OracleVersion has been removed and use jdk.Version to
obtain the fourth element of the version number.
This patch looks good in general. Minor comment:
273 current = parse(System.getProperty("java.version"));
System.getProperty will do a permission check and it needs to be called within
a doPrivileged block.
154 * @see <a href="http://openjdk.java.net/jeps/223";>JEP 223: New
Version-String Scheme</a>
Does the javadoc have the essential information from this JEP? Wonder if this
@see is necessary.
Alan already comment on the “jdk” package that needs to find the proper module
to export it (that’s a future RFE) and modules.xml should be updated.
Mandy
