RFR 8150840: Add an internal system property to control the default level of System.Logger when java.logging is not present.
[Resending with a link to the patch] Hi, Please find below a patch for: https://bugs.openjdk.java.net/browse/JDK-8150840 8150840: Add an internal system property to control the default level of System.Logger when java.logging is not present. http://cr.openjdk.java.net/~dfuchs/webrev_8150840/webrev.00 This patch also introduces a better separation between the SimpleConsoleLogger (created by the DefaultLoggerFinder when java.logging is not there), and the SurrogateLogger, which emulates the behavior of java.util.logging.Logger when java.logging is present but there is no custom configuration (used to be PlatformLogger.DefaultLoggerProxy). best regards, -- daniel
Hi Daniel, Good idea. SimpleConsolerLogger.java: Some of the property accesses could use the existing property actions instead of anonymous inner classes. static Level getDefaultLevel() { String levelName = AccessController.doPrivileged( new sun.security.action.GetPropertyAction("jdk.system.logger.level", "INFO")); ... Roger On 3/4/2016 6:48 AM, Daniel Fuchs wrote:
[Resending with a link to the patch]
Hi,
Please find below a patch for:
https://bugs.openjdk.java.net/browse/JDK-8150840 8150840: Add an internal system property to control the default level of System.Logger when java.logging is not present.
http://cr.openjdk.java.net/~dfuchs/webrev_8150840/webrev.00
This patch also introduces a better separation between the SimpleConsoleLogger (created by the DefaultLoggerFinder when java.logging is not there), and the SurrogateLogger, which emulates the behavior of java.util.logging.Logger when java.logging is present but there is no custom configuration (used to be PlatformLogger.DefaultLoggerProxy).
best regards,
-- daniel
Can they be method refs, or is this one of those cases where it could be early boot where none of that stuff works yet? -- - DML
On Mar 4, 2016, at 10:19 AM, Roger Riggs <Roger.Riggs@Oracle.com> wrote:
Hi Daniel,
Good idea.
SimpleConsolerLogger.java: Some of the property accesses could use the existing property actions instead of anonymous inner classes.
static Level getDefaultLevel() { String levelName = AccessController.doPrivileged( new sun.security.action.GetPropertyAction("jdk.system.logger.level", "INFO")); ...
Roger
On 3/4/2016 6:48 AM, Daniel Fuchs wrote: [Resending with a link to the patch]
Hi,
Please find below a patch for:
https://bugs.openjdk.java.net/browse/JDK-8150840 8150840: Add an internal system property to control the default level of System.Logger when java.logging is not present.
http://cr.openjdk.java.net/~dfuchs/webrev_8150840/webrev.00
This patch also introduces a better separation between the SimpleConsoleLogger (created by the DefaultLoggerFinder when java.logging is not there), and the SurrogateLogger, which emulates the behavior of java.util.logging.Logger when java.logging is present but there is no custom configuration (used to be PlatformLogger.DefaultLoggerProxy).
best regards,
-- daniel
I thought about that also but it is one of those cases where where it is 'too early' for method refs. Roger On 3/4/2016 11:56 AM, David Lloyd wrote:
Can they be method refs, or is this one of those cases where it could be early boot where none of that stuff works yet? -- - DML
On Mar 4, 2016, at 10:19 AM, Roger Riggs <Roger.Riggs@Oracle.com> wrote:
Hi Daniel,
Good idea.
SimpleConsolerLogger.java: Some of the property accesses could use the existing property actions instead of anonymous inner classes.
static Level getDefaultLevel() { String levelName = AccessController.doPrivileged( new sun.security.action.GetPropertyAction("jdk.system.logger.level", "INFO")); ...
Roger
On 3/4/2016 6:48 AM, Daniel Fuchs wrote: [Resending with a link to the patch]
Hi,
Please find below a patch for:
https://bugs.openjdk.java.net/browse/JDK-8150840 8150840: Add an internal system property to control the default level of System.Logger when java.logging is not present.
http://cr.openjdk.java.net/~dfuchs/webrev_8150840/webrev.00
This patch also introduces a better separation between the SimpleConsoleLogger (created by the DefaultLoggerFinder when java.logging is not there), and the SurrogateLogger, which emulates the behavior of java.util.logging.Logger when java.logging is present but there is no custom configuration (used to be PlatformLogger.DefaultLoggerProxy).
best regards,
-- daniel
Hi Roger, Yes that's a good remark: Applied it to SimpleConsoleLogger.java. http://cr.openjdk.java.net/~dfuchs/webrev_8150840/webrev.01/ -- daniel On 04/03/16 17:19, Roger Riggs wrote:
Hi Daniel,
Good idea.
SimpleConsolerLogger.java: Some of the property accesses could use the existing property actions instead of anonymous inner classes.
static Level getDefaultLevel() { String levelName = AccessController.doPrivileged( new sun.security.action.GetPropertyAction("jdk.system.logger.level", "INFO")); ...
Roger
On 3/4/2016 6:48 AM, Daniel Fuchs wrote:
[Resending with a link to the patch]
Hi,
Please find below a patch for:
https://bugs.openjdk.java.net/browse/JDK-8150840 8150840: Add an internal system property to control the default level of System.Logger when java.logging is not present.
http://cr.openjdk.java.net/~dfuchs/webrev_8150840/webrev.00
This patch also introduces a better separation between the SimpleConsoleLogger (created by the DefaultLoggerFinder when java.logging is not there), and the SurrogateLogger, which emulates the behavior of java.util.logging.Logger when java.logging is present but there is no custom configuration (used to be PlatformLogger.DefaultLoggerProxy).
best regards,
-- daniel
On Mar 4, 2016, at 9:05 AM, Daniel Fuchs <daniel.fuchs@oracle.com> wrote:
http://cr.openjdk.java.net/~dfuchs/webrev_8150840/webrev.01/
Looks okay in general. I’m not a fan of using GetPropertyAction. While it’s convenient as the class already exists, method refs and anonymous class makes what it does more explicit at the callsite. No big deal. Does -Djava.util.logging.SimpleFormatter.format=… have any effect if java.logging is absent (when used together with jdk.system.logger.level)? It’s one of the test cases in SimpleConsoleLoggerTest. I would expect java.util.logging.* properties are used fro java.util.logging configuration only. JUL_FORMAT_PROP_KEY is defined in SimpleConsoleLogger. If I read it correctly, it’s only used for the limited doPrivileged. 472 new PropertyPermission(JUL_FORMAT_PROP_KEY, "read")); I was initially confused what SimpleConsoleLogger is done with java.util.logging formatting. If JUL_FORMAT_PROP_KEY is not referenced anywhere else, perhaps just remove the constant variable and have a comment to explain this getSimpleFormat method is shared with JUL? Mandy
Hi Mandy, On 06/03/16 00:29, Mandy Chung wrote:
On Mar 4, 2016, at 9:05 AM, Daniel Fuchs <daniel.fuchs@oracle.com> wrote:
http://cr.openjdk.java.net/~dfuchs/webrev_8150840/webrev.01/
Looks okay in general.
I’m not a fan of using GetPropertyAction. While it’s convenient as the class already exists, method refs and anonymous class makes what it does more explicit at the callsite. No big deal.
In this case I find that it nicely simplifies the code.
Does -Djava.util.logging.SimpleFormatter.format=… have any effect if java.logging is absent (when used together with jdk.system.logger.level)?
No - that was one of the purpose of splitting the SimpleConsoleLogger class in two (that is: introducing the SurrogateLogger subclass). The java.util.logging.* properties are only used with the SurrogateLogger - which is only used when java.logging is present and has no custom configuration and until the application triggers the initialization of the LogManager.
It’s one of the test cases in SimpleConsoleLoggerTest. I would expect java.util.logging.* properties are used fro java.util.logging configuration only.
Yes. That's what happens. The SimpleConsoleLoggerTest verifies that. I mean it verifies that these properties (java.logging.*) are only used by the SurrogateLogger subclass, and jdk.system.logger.* are only used by the (unsubclassed) SimpleConsoleLogger. Ideally there should be an abstract ConsoleLogger class with two concrete trivial implementations (SimpleConsoleLogger and SurrogateLogger) but that would be introducing yet another class so I left it with the simple single inheritance branch.
JUL_FORMAT_PROP_KEY is defined in SimpleConsoleLogger. If I read it correctly, it’s only used for the limited doPrivileged. 472 new PropertyPermission(JUL_FORMAT_PROP_KEY, "read"));
Yes. And in the SurrogateLogger subclass.
I was initially confused what SimpleConsoleLogger is done with java.util.logging formatting.
I see. Logically it belongs to SurrogateLogger but I didn't want to trigger the loading of the class if it's not used.
If JUL_FORMAT_PROP_KEY is not referenced anywhere else, perhaps just remove the constant variable and have a comment to explain this getSimpleFormat method is shared with JUL?
I'd rather keep the constant since it's used in two places. But adding a comment to clear up any confusion is certainly a good idea. best regards, -- daniel
Mandy
Hi Mandy, Here is the new webrev - I have added comments for the string constants as well as for the permissions passed through to the limited doPrivileged call: http://cr.openjdk.java.net/~dfuchs/webrev_8150840/webrev.02/index.html I hope that it makes it more clear that the java.util.logging.* properties are only used when java.logging is present, and the jdk.system.logger.* properties when java.logging is not present. best regards, -- daniel On 14/03/16 15:03, Daniel Fuchs wrote:
Hi Mandy,
On 06/03/16 00:29, Mandy Chung wrote:
On Mar 4, 2016, at 9:05 AM, Daniel Fuchs <daniel.fuchs@oracle.com> wrote:
http://cr.openjdk.java.net/~dfuchs/webrev_8150840/webrev.01/
Looks okay in general.
I’m not a fan of using GetPropertyAction. While it’s convenient as the class already exists, method refs and anonymous class makes what it does more explicit at the callsite. No big deal.
In this case I find that it nicely simplifies the code.
Does -Djava.util.logging.SimpleFormatter.format=… have any effect if java.logging is absent (when used together with jdk.system.logger.level)?
No - that was one of the purpose of splitting the SimpleConsoleLogger class in two (that is: introducing the SurrogateLogger subclass). The java.util.logging.* properties are only used with the SurrogateLogger - which is only used when java.logging is present and has no custom configuration and until the application triggers the initialization of the LogManager.
It’s one of the test cases in SimpleConsoleLoggerTest. I would expect java.util.logging.* properties are used fro java.util.logging configuration only.
Yes. That's what happens. The SimpleConsoleLoggerTest verifies that. I mean it verifies that these properties (java.logging.*) are only used by the SurrogateLogger subclass, and jdk.system.logger.* are only used by the (unsubclassed) SimpleConsoleLogger.
Ideally there should be an abstract ConsoleLogger class with two concrete trivial implementations (SimpleConsoleLogger and SurrogateLogger) but that would be introducing yet another class so I left it with the simple single inheritance branch.
JUL_FORMAT_PROP_KEY is defined in SimpleConsoleLogger. If I read it correctly, it’s only used for the limited doPrivileged. 472 new PropertyPermission(JUL_FORMAT_PROP_KEY, "read"));
Yes. And in the SurrogateLogger subclass.
I was initially confused what SimpleConsoleLogger is done with java.util.logging formatting.
I see. Logically it belongs to SurrogateLogger but I didn't want to trigger the loading of the class if it's not used.
If JUL_FORMAT_PROP_KEY is not referenced anywhere else, perhaps just remove the constant variable and have a comment to explain this getSimpleFormat method is shared with JUL?
I'd rather keep the constant since it's used in two places. But adding a comment to clear up any confusion is certainly a good idea.
best regards,
-- daniel
Mandy
On Mar 24, 2016, at 7:52 AM, Daniel Fuchs <daniel.fuchs@oracle.com> wrote:
Hi Mandy,
Here is the new webrev - I have added comments for the string constants as well as for the permissions passed through to the limited doPrivileged call:
http://cr.openjdk.java.net/~dfuchs/webrev_8150840/webrev.02/index.html
I hope that it makes it more clear that the java.util.logging.* properties are only used when java.logging is present, and the jdk.system.logger.* properties when java.logging is not present.
It looks okay. “jdk.logger.packages” is to specify which packages to be filtered and maybe better to rename it to “jdk.logger.filtered.packages”? About usage of limited doPrivileged, it’d be useful to have some guideline of using limited doPrivileged - I brought this up to Jeff when it was introduced and will ping him again. Like this example (in SimpleConsoleLogger.java) 476 String format = AccessController.doPrivileged( 477 new GetPropertyAction(key), null, 482 new PropertyPermission(DEFAULT_FORMAT_PROP_KEY, "read"), 487 new PropertyPermission(JUL_FORMAT_PROP_KEY, "read")); The doPrivileged block simply reads a system property and one single security-sensitive operation. It’s nothing wrong using limited doPrivileged in this case but I think it’s unnecessary. Limited doPrivileged would benefit to limiting a block of code that is hard to tell what privileges are elevated. getSimpleFormat should probably check the given key argument is either DEFAULT_FORMAT_PROP_KEY or JUL_FORMAT_PROP_KEY; otherwise throws IAE. Otherwise, this method would pass if key is unexpected key if security manager is absent but fails if security manager is present. Mandy
On 24/03/16 18:40, Mandy Chung wrote:
On Mar 24, 2016, at 7:52 AM, Daniel Fuchs <daniel.fuchs@oracle.com> wrote:
Hi Mandy,
Here is the new webrev - I have added comments for the string constants as well as for the permissions passed through to the limited doPrivileged call:
http://cr.openjdk.java.net/~dfuchs/webrev_8150840/webrev.02/index.html
I hope that it makes it more clear that the java.util.logging.* properties are only used when java.logging is present, and the jdk.system.logger.* properties when java.logging is not present.
It looks okay. “jdk.logger.packages” is to specify which packages to be filtered and maybe better to rename it to “jdk.logger.filtered.packages”?
I am thinking about removing this property. This was added before StackWalker was used. Now that we have StackWalker, the inferCaller code will skip classes that implement System.Logger - and this may be enough for our purposes... It's something I still need to ponder a bit more, so I'd suggest handling the renaming or removing in a followup fix.
About usage of limited doPrivileged, it’d be useful to have some guideline of using limited doPrivileged - I brought this up to Jeff when it was introduced and will ping him again.
Like this example (in SimpleConsoleLogger.java)
476 String format = AccessController.doPrivileged( 477 new GetPropertyAction(key), null, 482 new PropertyPermission(DEFAULT_FORMAT_PROP_KEY, "read"), 487 new PropertyPermission(JUL_FORMAT_PROP_KEY, "read"));
The doPrivileged block simply reads a system property and one single security-sensitive operation. It’s nothing wrong using limited doPrivileged in this case but I think it’s unnecessary. Limited doPrivileged would benefit to limiting a block of code that is hard to tell what privileges are elevated.
This is precisely one of these cases where limited doPrivileged should be used: otherwise you could abuse this method to read an arbitrary property.
getSimpleFormat should probably check the given key argument is either DEFAULT_FORMAT_PROP_KEY or JUL_FORMAT_PROP_KEY; otherwise throws IAE. Otherwise, this method would pass if key is unexpected key if security manager is absent but fails if security manager is present.
Oh - right - good idea. Replace the limited doPrivileged with explicit argument checking :-) Let's do that. Thanks for the comments! -- daniel
Mandy
On 24/03/16 18:56, Daniel Fuchs wrote:
getSimpleFormat should probably check the given key argument is either DEFAULT_FORMAT_PROP_KEY or JUL_FORMAT_PROP_KEY; otherwise throws IAE. Otherwise, this method would pass if key is unexpected key if security manager is absent but fails if security manager is present.
Oh - right - good idea. Replace the limited doPrivileged with explicit argument checking :-) Let's do that.
Here is the new webrev: http://cr.openjdk.java.net/~dfuchs/webrev_8150840/webrev.03/index.html Only changes are in SimpleConsoleLogger.Formatting::getSimpleFormat. best regards, -- daniel
On Mar 25, 2016, at 5:12 AM, Daniel Fuchs <daniel.fuchs@oracle.com> wrote:
On 24/03/16 18:56, Daniel Fuchs wrote:
getSimpleFormat should probably check the given key argument is either DEFAULT_FORMAT_PROP_KEY or JUL_FORMAT_PROP_KEY; otherwise throws IAE. Otherwise, this method would pass if key is unexpected key if security manager is absent but fails if security manager is present.
Oh - right - good idea. Replace the limited doPrivileged with explicit argument checking :-) Let's do that.
Here is the new webrev:
http://cr.openjdk.java.net/~dfuchs/webrev_8150840/webrev.03/index.html
Only changes are in SimpleConsoleLogger.Formatting::getSimpleFormat.
Looks okay. Minor nit: why not using String::equals? No need to have a new webrev. Mandy
On 25/03/16 15:57, Mandy Chung wrote:
Here is the new webrev:
http://cr.openjdk.java.net/~dfuchs/webrev_8150840/webrev.03/index.html
Only changes are in SimpleConsoleLogger.Formatting::getSimpleFormat.
Looks okay. Minor nit: why not using String::equals? No need to have a new webrev.
Oh - just a matter of taste I guess. Objects.equals will always work :-) It's not like this code is performance sensitive: it will be called only once. Thanks Mandy! -- daniel
participants (4)
-
Daniel Fuchs
-
David Lloyd
-
Mandy Chung
-
Roger Riggs