Hi Peter,
Good idea to add a package constructor in Handler.
It looks much cleaner than the configure method.
Good tests too - thanks for adding that!
To access files with JPRT there is a simpler manner (though what
you have looks good).
JPRT sets a system property "test.src" which point at the
directory where the sources are located.
So you could have used something like:
System.setProperty(CONFIG_FILE_PROPERTY,
new File(System.getProperty("test.src", "."),
this.getClass().getSimpleName()+".props")
.getAbsolutePath());
best regards,
-- daniel
On 12/19/13 4:49 PM, Peter Levart wrote:
On 12/18/2013 11:55 PM, Mandy Chung wrote:
On 12/18/2013 9:03 AM, Peter Levart wrote:
Hi Mandy, Daniel,
Here's yet another variant that reduces the doPrivileged code to just
Handler's setters. This way no LogManager methods are invoked under
elevated privilege:
http://cr.openjdk.java.net/~plevart/jdk8-tl/jul.Handler.sealed/webrev.06/
This version looks good. I like the refactoring to have the subclass
to call the common code Handler.configure method. It may be better to
have the configure method (or a new one) that takes the default Level
and default Formatter instead of the package-private getters.
I don't see why the handler constructors are designed to call the
overridden methods rather than the initialization and if a subclass
has its custom field, it should initialize its custom fields in its
constructor implementation. Anyway this would be a separate clean
up task from this one.
Can you also add a sanity test to verify that these handlers can be
constructed successfully with a security manager installed?
Hi Mandy, Daniel,
I didn't like the package-protected getters either. So here's another
variant that replaces Handler.configure() method with a
package-protected constructor which is chained from JDK subclasses:
http://cr.openjdk.java.net/~plevart/jdk8-tl/jul.Handler.sealed/webrev.07/
I filed another bug that is fixed by this patch:
https://bugs.openjdk.java.net/browse/JDK-8030801
And I created a test (see webrev.07) that almost passes when run against
unchanged JDK 8 (the failure is just at the end when calling new
SocketHandler(host, port) - access denied
("java.util.logging.LoggingPermission" "control")). If I comment-out the
System.setSecurityManager() from the test, it passes with unchanged
code. This is to verify the test itself. When run against the patched
JDK 8, it passes even when SecurityManager is active - this verifies two
things:
- the behaviour of patched code is analogous to unpatched code as far as
defaults and configured handler properties is concerned and it conforms
to javadoc
- the patched code does not require any new permissions - it actually
requires less, because it fixes bug 8030801.
All java/util/logging jtreg tests pass with patched code. I hope that
"localhost" is a resolvable name on all environments and that new
ServerSocket(0) creates a server socket bound at least to the IP address
that "localhost" resolves to. Is this reasonable to assume?
Regards, Peter
