Re: Review Request: 7193339 Prepare system classes be defined by a non-null module loader

[serguei.spit...@oracle.com]

I was thinking about the same.
But a CCC request is needed for such a change in public API.
It can be done separately if any other API changes are necessary.

Thanks,
Serguei


On 8/23/12 6:27 PM, David Holmes wrote:

Hi Mandy,

While I understand what you are doing and that it "works" it is far 
from obvious upon code inspection that where you replace null with 
Foo.class.getClassLoader(), that the result would always be null.


Another way to simplify this would be to add a new overload of 
Class.forName():


public static Class forName(String name, boolean initialize)

That way the loader argument could be dropped completely from a number 
of the use-cases.


David

On 24/08/2012 3:43 AM, Mandy Chung wrote:
This change is to bring the jdk modularization changes from jigsaw 
repo [1]

to jdk8. This allows the jdk modularization changes to be exposed for
testing as early as possible and minimize the amount of changes carried
in the jigsaw repo that helps sync up jigsaw with jdk8/jdk9.

Webrev at:
http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7193339/webrev.00/

Summary:
The VM bootstrap class loader (null) has been the defining class loader
for all system classes (i.e. rt.jar and any classes on the 
bootclasspath).


In modular world, the system classes are going to be modularized into
multiple modulesand and many of which will be loaded by its own
(non-null) module loader.

The JDK implementation has the assumption that the defining class loader
of system classes is null and it'll no longer be true when running as
modules. Typical patterns in the JDK code include:

Class.forName(classname, false, null) should be changed to:
Class.forName(classname, false,)

if (loader == null) condition check should be modified to check if the
loader
is responsible for loading system classes.

This is the first set of change for this problem we identified. Similar
change in other components will be made in the future.

Testing: run all jdk core test targets on all platforms.

Thanks
Mandy
P.S. I'm cc'ing servicebility-dev as this patch modifies 2 files in
the java.lang.management.

Re: Review Request: 7193406 - Clean-up JDK Build Warnings in java.util, java.io

[David Holmes]

Hi Dan,

On 24/08/2012 7:46 AM, Dan Xu wrote:

Please review the fix of CR 7193406 at
http://cr.openjdk.java.net/~dxu/7193406/webrev/.

It cleans up warnings in the following java files.

src/share/classes/java/io/FilePermission.java


I'm surprised that you need this:

 426 @SuppressWarnings("fallthrough")
 ...
 436 switch (actions) {
 437 case SecurityConstants.FILE_READ_ACTION:
 438 return READ;

If this generates a fallthrough warning then I think whatever tool 
generates that warning needs fixing!



src/share/classes/java/util/ArrayDeque.java
src/share/classes/java/util/Arrays.java
src/share/classes/java/util/Collections.java
src/share/classes/java/util/HashMap.java
src/share/classes/java/util/JumboEnumSet.java
src/share/classes/java/util/PriorityQueue.java
src/share/classes/java/util/PropertyResourceBundle.java


Here:

! @SuppressWarnings({ "unchecked", "rawtypes" })
! Map result = new HashMap(properties);
! lookup = result;

why do you keep the raw type instead of using HashMap<>(properties) ?


src/share/classes/java/util/jar/JarVerifier.java
src/share/classes/java/util/jar/Pack200.java
src/share/classes/sun/util/PreHashedMap.java



And it enables fatal warning flag in the following make file.

make/java/jar/Makefile


I'm not sure what Andrew's objection is to this change as it only 
affects how java/util/jar classes get compiled and is consistent with 
the usage in all the other Makefiles. As far as I can see you can 
override this on the make invocation anyway.


I'm unclear how this is handled in the new build.


In FilePermission.java file, I make one change to its method signature,

public Enumeration elements() ==> public Enumeration
elements()


I am not sure whether it will cause an issue of backward compatibility.
Please advise. Thanks!


As Andrew stated this is a package-private class so I don't see any issue.

Cheers,
David


- Dan

Re: Review Request: 7193406 - Clean-up JDK Build Warnings in java.util, java.io

[Stuart Marks]

On 8/23/12 5:12 PM, Andrew Hughes wrote:

Dan Xu wrote:

Please review the fix of CR 7193406 at
http://cr.openjdk.java.net/~dxu/7193406/webrev/.
And it enables fatal warning flag in the following make file.

 make/java/jar/Makefile


Please don't do this, at least not unconditionally.


Right, we had been enabling fatal warnings in the various makefiles but we've 
stopped doing so since it has started to cause problems. The intent was to 
enable fatal warnings by default in individual makefiles, as those areas of the 
build were cleared of warnings. That way, the introduction of a new warning 
into a warnings-free area **would** break the build.


The problem is that because of implicit compilation, as code is modified, files 
can shift around being compiled by different Makefiles. Thus an apparently 
innocuous change might cause a file with warnings to be built by a different 
javac run from a makefile that has fatal warnings enabled, causing an 
unexpected build breakage.


Anyway, Dan, please don't enable this flag in this (or any other) Makefile. 
Sorry, I thought I had mentioned this to you before.



At the very least, these should not be forced on if the user
has explicitly built with them set to false.  If I set
JAVAC_WARNINGS_FATAL=false, I don't expect part of the build
to ignore this and use -Werror, possibly then causing build
failures.


Yes, it should always be possible to override this by specifying 
JAVAC_WARNINGS_FATAL=false on the make command line. This overriding should 
work if the value is specified directly in a Makefile, e.g.


JAVAC_WARNINGS_FATAL = true

However, there are several cases where the following occurs:

SUBDIRS_MAKEFLAGS += JAVAC_WARNINGS_FATAL=true

and this is **not** overridable on the command line. That's wrong. If these are 
causing problems for you, please do submit patches.


Although we still occasionally run into problems with implicit compilation 
causing unexpected build failures, I don't want to remove the fatal warnings 
settings wholesale. The settings in place now do seem to be keeping at least 
some parts of the build warning-free.


The new build system will change all of this completely, of course. I don't 
think they have a solution yet for applying different flags to different parts 
of the build.



In FilePermission.java file, I make one change to its method
signature,

 public Enumeration elements()  ==>  public Enumeration
 elements()


It's in a package-private class so I doubt it.

Even if it wasn't, a new major release is the perfect time to fix these issues.


Yes, this one is fine because it's a private class.

For warnings fixes we're trying to avoid making any API changes, since those 
have to go through some additional process steps.


s'marks

Re: Review Request: 7193339 Prepare system classes be defined by a non-null module loader

[David Holmes]

Hi Mandy,

While I understand what you are doing and that it "works" it is far from 
obvious upon code inspection that where you replace null with 
Foo.class.getClassLoader(), that the result would always be null.


Another way to simplify this would be to add a new overload of 
Class.forName():


public static Class forName(String name, boolean initialize)

That way the loader argument could be dropped completely from a number 
of the use-cases.


David

On 24/08/2012 3:43 AM, Mandy Chung wrote:

This change is to bring the jdk modularization changes from jigsaw repo [1]
to jdk8. This allows the jdk modularization changes to be exposed for
testing as early as possible and minimize the amount of changes carried
in the jigsaw repo that helps sync up jigsaw with jdk8/jdk9.

Webrev at:
http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7193339/webrev.00/

Summary:
The VM bootstrap class loader (null) has been the defining class loader
for all system classes (i.e. rt.jar and any classes on the bootclasspath).

In modular world, the system classes are going to be modularized into
multiple modulesand and many of which will be loaded by its own
(non-null) module loader.

The JDK implementation has the assumption that the defining class loader
of system classes is null and it'll no longer be true when running as
modules. Typical patterns in the JDK code include:

Class.forName(classname, false, null) should be changed to:
Class.forName(classname, false,)

if (loader == null) condition check should be modified to check if the
loader
is responsible for loading system classes.

This is the first set of change for this problem we identified. Similar
change in other components will be made in the future.

Testing: run all jdk core test targets on all platforms.

Thanks
Mandy
P.S. I'm cc'ing servicebility-dev as this patch modifies 2 files in
the java.lang.management.

Re: Review Request: 7193406 - Clean-up JDK Build Warnings in java.util, java.io

[Andrew Hughes]

- Original Message -
> 
> 
> - Original Message -
> > Please review the fix of CR 7193406 at
> > http://cr.openjdk.java.net/~dxu/7193406/webrev/.
> > 
> 
> snip...
> 
> > And it enables fatal warning flag in the following make file.
> > 
> > make/java/jar/Makefile
> > 
> 
> Please don't do this, at least not unconditionally.
> 
> At the very least, these should not be forced on if the user
> has explicitly built with them set to false.  If I set
> JAVAC_WARNINGS_FATAL=false, I don't expect part of the build
> to ignore this and use -Werror, possibly then causing build
> failures.
> 
> This is especially bad with both on as a new warning may be
> introduced into javac which then breaks everyone's build.
> 
> I already have a patch I intend to upstream which fixes the
> other cases of this I found.  I'd prefer we didn't have another.
> 
> > 
> > In FilePermission.java file, I make one change to its method
> > signature,
> > 
> > public Enumeration elements()  ==> public
> > Enumeration
> > elements()
> > 
> > 
> > I am not sure whether it will cause an issue of backward
> > compatibility.
> > Please advise. Thanks!
> > 
> 
> It's in a package-private class so I doubt it.
> 
> Even if it wasn't, a new major release is the perfect time to fix
> these issues.
> 
> > - Dan
> > 
> 

I should also point out that the annotation:

@SuppressWarnings("unchecked")
private void readObject(ObjectInputStream in) throws IOException,

could be moved down to the actual problematic assignment:

Vector permissions = 
(Vector)gfields.get("permissions", null);

so that warnings aren't suppressed throughout the method.

-- 
Andrew :)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

PGP Key: 248BDC07 (https://keys.indymedia.org/)
Fingerprint = EC5A 1F5E C0AD 1D15 8F1F  8F91 3B96 A578 248B DC07

Re: rmi network traffic

[Darryl Mocek]

Hi fuyou001,

   if you're using RMI in a J2SE project and it is OK, but it isn't 
when you use the web project, then I suggest you look at the Jetty/Resin 
implementation as it may be the culprit.  Specifically, I'd look at the 
ClassLoader in use.  See this URL 
(http://java.sun.com/javase/6/docs/api/java/rmi/server/RMIClassLoader.html#getDefaultProviderInstance()). 
If the ClassLoader is a URLClassLoader, then the loader's |getURLs| 
method may be called, which may return a list of URls of the 
application's class path.  It may also be that the value of the 
|java.rmi.server.codebase property is being returned, so you should look 
at that as well. The URL gives more details on the issue.


||See below for information from the bug report.

Darryl
|

RMI's marshalling format annotates serialized class descriptors with a
"codebase URL path" in case the class needs to be dynamically loaded
by the receiving party.  ... The value used for this annotation is
controlled by the RMIClassLoader provider's getClassAnotation method:

http://java.sun.com/javase/6/docs/api/java/rmi/server/RMIClassLoader.html#getClassAnnotation(java.lang.Class)

which the default provider implements as described here:

http://java.sun.com/javase/6/docs/api/java/rmi/server/RMIClassLoader.html#getDefaultProviderInstance()

*Basically, if the class's defining loader is the system class loader
or one of its ancestors, or if it is not a URLClassLoader, then the
value of the "java.rmi.server.codebase" system property is used;
otherwise, a string form of the result of invoking
URLClassLoader.getURLs on the loader is used.*

In this case, I gather that the ClassLoader is not configured to
be considered the system class loader, but it is a URLClassLoader
whose getURLs method returns the URLs of the application's class path,
which in this case is quite lengthy.



On 08/21/2012 07:23 PM, fuyou wrote:

Hi all

 I  deploy a RMI Service ,write a RMI Client(J2SE project) to access it
and is ok .but the some code in web project,I use wireshark to find that
every RMI request sent from our client to our server containing the list of
all jars in the classpath for the application, My problem is similar to
bugs.sun.com 

java -version
java version "1.6.0_33"
Java(TM) SE Runtime Environment (build 1.6.0_33-b03)
Java HotSpot(TM) Server VM (build 20.8-b03, mixed mode)

This issues may be caused by Servlet Container(etc Jetty/Resin)
  classloader
How to slove it ?

Sorry for disturbing again.
Thanks a lot!

Re: Review Request: 7193406 - Clean-up JDK Build Warnings in java.util, java.io

[Andrew Hughes]

- Original Message -
> Please review the fix of CR 7193406 at
> http://cr.openjdk.java.net/~dxu/7193406/webrev/.
> 

snip...

> And it enables fatal warning flag in the following make file.
> 
> make/java/jar/Makefile
> 

Please don't do this, at least not unconditionally.

At the very least, these should not be forced on if the user
has explicitly built with them set to false.  If I set
JAVAC_WARNINGS_FATAL=false, I don't expect part of the build
to ignore this and use -Werror, possibly then causing build
failures.

This is especially bad with both on as a new warning may be
introduced into javac which then breaks everyone's build.

I already have a patch I intend to upstream which fixes the
other cases of this I found.  I'd prefer we didn't have another.

> 
> In FilePermission.java file, I make one change to its method
> signature,
> 
> public Enumeration elements()  ==> public Enumeration
> elements()
> 
> 
> I am not sure whether it will cause an issue of backward
> compatibility.
> Please advise. Thanks!
> 

It's in a package-private class so I doubt it.

Even if it wasn't, a new major release is the perfect time to fix these issues.

> - Dan
> 

-- 
Andrew :)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

PGP Key: 248BDC07 (https://keys.indymedia.org/)
Fingerprint = EC5A 1F5E C0AD 1D15 8F1F  8F91 3B96 A578 248B DC07

Re: Review Request: 7193339 Prepare system classes be defined by a non-null module loader

[Mandy Chung]

On 8/23/2012 2:26 PM, Dmitry Samersoff wrote:

Mandy,

1. You replace null with getClassLoader() calls in couple of places.
getClassLoader requires special permissions -
RuntimePermission("getClassLoader")

so probably you need to use doPrivileget() there.


No, the caller's class loader (all are system classes) is null and so no 
permission check is required [1].



Did you test your changes with SecurityManager/No permissions
for the test ?


I ran the jck tests that cover this permission check.  The jck tests 
uncovered these classes to be fixed during jigsaw development.




2. Did you consider moving

sm.checkPermission(SecurityConstants.GET_CLASSLOADER_PERMISSION);

inside ClassLoader.needsClassLoaderPermissionCheck(ccl, cl); ?


Yes I did and I decide to leave this sm.checkPermission call in the 
method body as it's closer to the spec and more readable.




3. ManagementFactory.java

Could you explain the reason of changes (except 588) ?


The platform MXBeans (java.lang.management) are currently on the 
bootclasspath loaded by the null loader.  In the modular world, j.l.m. 
will possibly be put in the "management" module along with JMX so that 
an application only requires the management module to be present when it 
uses JMX and platform MXBean.  In that case, the management module will 
be loaded by a non-null module loader.  This check (loader != null) 
would fail in the modular world even loading platform MXBeans.   We 
changed this check and other places in the JDK to call 
VM.isSystemDomainLoader method and the real check for modules can be 
made in this single convenient method in the jigsaw repo.



-Dmitry


Thanks
Mandy
 [1] 
http://docs.oracle.com/javase/7/docs/api/java/lang/Class.html#getClassLoader()




On 2012-08-23 23:33, Mandy Chung wrote:

On 8/23/2012 11:58 AM, Alan Bateman wrote:

On 23/08/2012 18:43, Mandy Chung wrote:

This change is to bring the jdk modularization changes from jigsaw
repo [1]
to jdk8.  This allows the jdk modularization changes to be exposed for
testing as early as possible and minimize the amount of changes carried
in the jigsaw repo that helps sync up jigsaw with jdk8/jdk9.

Webrev at:
   http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7193339/webrev.00/


This looks good to me and it's good to have these changes in jdk8.

One suggestion for ReflectUtil is to add a private static boolean
isAncestor method as I think that would make the checks in
needsPackageAccessCheck easier to read. Also in ClassLoader you could
use just use needsClassLoaderPermissionCheck(from,to).


Done.  This is a good cleanup I considered to do too but missed in the
previous webrev.

http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7193339/webrev.01/

Thanks for the review.
Mandy

Re: Review Request: 7193339 Prepare system classes be defined by a non-null module loader

[serguei.spit...@oracle.com]

Looks good.

Thanks,
Serguei


On 8/23/12 12:33 PM, Mandy Chung wrote:

On 8/23/2012 11:58 AM, Alan Bateman wrote:

On 23/08/2012 18:43, Mandy Chung wrote:
This change is to bring the jdk modularization changes from jigsaw 
repo [1]

to jdk8.  This allows the jdk modularization changes to be exposed for
testing as early as possible and minimize the amount of changes carried
in the jigsaw repo that helps sync up jigsaw with jdk8/jdk9.

Webrev at:
http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7193339/webrev.00/


This looks good to me and it's good to have these changes in jdk8.

One suggestion for ReflectUtil is to add a private static boolean 
isAncestor method as I think that would make the checks in 
needsPackageAccessCheck easier to read. Also in ClassLoader you could 
use just use needsClassLoaderPermissionCheck(from,to).




Done.  This is a good cleanup I considered to do too but missed in the 
previous webrev.


http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7193339/webrev.01/

Thanks for the review.
Mandy

Review Request: 7193406 - Clean-up JDK Build Warnings in java.util, java.io

[Dan Xu]

Please review the fix of CR 7193406 at 
http://cr.openjdk.java.net/~dxu/7193406/webrev/.


It cleans up warnings in the following java files.

   src/share/classes/java/io/FilePermission.java
   src/share/classes/java/util/ArrayDeque.java
   src/share/classes/java/util/Arrays.java
   src/share/classes/java/util/Collections.java
   src/share/classes/java/util/HashMap.java
   src/share/classes/java/util/JumboEnumSet.java
   src/share/classes/java/util/PriorityQueue.java
   src/share/classes/java/util/PropertyResourceBundle.java
   src/share/classes/java/util/jar/JarVerifier.java
   src/share/classes/java/util/jar/Pack200.java
   src/share/classes/sun/util/PreHashedMap.java



And it enables fatal warning flag in the following make file.

   make/java/jar/Makefile


In FilePermission.java file, I make one change to its method signature,

   public Enumeration elements()  ==> public Enumeration
   elements()


I am not sure whether it will cause an issue of backward compatibility. 
Please advise. Thanks!


- Dan

Re: Review Request: 7193339 Prepare system classes be defined by a non-null module loader

[Dmitry Samersoff]

Mandy,

1. You replace null with getClassLoader() calls in couple of places.
   getClassLoader requires special permissions -
   RuntimePermission("getClassLoader")

   so probably you need to use doPrivileget() there.

   Did you test your changes with SecurityManager/No permissions
   for the test ?


2. Did you consider moving

sm.checkPermission(SecurityConstants.GET_CLASSLOADER_PERMISSION);

inside ClassLoader.needsClassLoaderPermissionCheck(ccl, cl); ?


3. ManagementFactory.java

Could you explain the reason of changes (except 588) ?

-Dmitry


On 2012-08-23 23:33, Mandy Chung wrote:
> On 8/23/2012 11:58 AM, Alan Bateman wrote:
>> On 23/08/2012 18:43, Mandy Chung wrote:
>>> This change is to bring the jdk modularization changes from jigsaw
>>> repo [1]
>>> to jdk8.  This allows the jdk modularization changes to be exposed for
>>> testing as early as possible and minimize the amount of changes carried
>>> in the jigsaw repo that helps sync up jigsaw with jdk8/jdk9.
>>>
>>> Webrev at:
>>>   http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7193339/webrev.00/
>>>
>> This looks good to me and it's good to have these changes in jdk8.
>>
>> One suggestion for ReflectUtil is to add a private static boolean
>> isAncestor method as I think that would make the checks in
>> needsPackageAccessCheck easier to read. Also in ClassLoader you could
>> use just use needsClassLoaderPermissionCheck(from,to).
>>
> 
> Done.  This is a good cleanup I considered to do too but missed in the
> previous webrev.
> 
> http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7193339/webrev.01/
> 
> Thanks for the review.
> Mandy


-- 
Dmitry Samersoff
Java Hotspot development team, SPB04
* There will come soft rains ...

Re: Review Request: 7193339 Prepare system classes be defined by a non-null module loader

[Alan Bateman]

On 23/08/2012 20:33, Mandy Chung wrote:

:

Done.  This is a good cleanup I considered to do too but missed in the 
previous webrev.


http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7193339/webrev.01/

Thanks for the review.
Mandy

That looks much better, so looks good to me.

-Alan

Re: Review Request: 7193339 Prepare system classes be defined by a non-null module loader

[Mandy Chung]

On 8/23/2012 12:48 PM, serguei.spit...@oracle.com wrote:

Mandy,

It looks good.


Thanks.


Just a question below.

|| *src/share/classes/java/lang/management/ManagementFactory.java*
596 String intfName = mxbeanInterface.getName();
599 " is not an instance of " + mxbeanInterface);

Did you want this?:
596 String intfName = cls.getName();
599 " is not an instance of " + cls);


It doesn't make any difference and so leave it as it is will be fine.

Mandy

Re: Review Request: 7193339 Prepare system classes be defined by a non-null module loader

[serguei.spit...@oracle.com]

Mandy,

It looks good.
Just a question below.

|| *src/share/classes/java/lang/management/ManagementFactory.java*

596 String intfName = mxbeanInterface.getName();
599 " is not an instance of " + mxbeanInterface);

Did you want this?:
596 String intfName = cls.getName();
599 " is not an instance of " + cls);


Thanks,
Serguei


On 8/23/12 10:43 AM, Mandy Chung wrote:
This change is to bring the jdk modularization changes from jigsaw 
repo [1]

to jdk8.  This allows the jdk modularization changes to be exposed for
testing as early as possible and minimize the amount of changes carried
in the jigsaw repo that helps sync up jigsaw with jdk8/jdk9.

Webrev at:
http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7193339/webrev.00/

Summary:
The VM bootstrap class loader (null) has been the defining class loader
for all system classes (i.e. rt.jar and any classes on the 
bootclasspath).


In modular world, the system classes are going to be modularized into
multiple modulesand and many of which will be loaded by its own
(non-null) module loader.

The JDK implementation has the assumption that the defining class loader
of system classes is null and it'll no longer be true when running as
modules. Typical patterns in the JDK code include:

Class.forName(classname, false, null) should be changed to:
   Class.forName(classname, false,)

if (loader == null) condition check should be modified to check if the 
loader

is responsible for loading system classes.

This is the first set of change for this problem we identified. Similar
change in other components will be made in the future.

Testing: run all jdk core test targets on all platforms.

Thanks
Mandy
P.S. I'm cc'ing servicebility-dev as this patch modifies 2 files in
the java.lang.management.

Re: Review Request: 7193339 Prepare system classes be defined by a non-null module loader

[Mandy Chung]

On 8/23/2012 11:58 AM, Alan Bateman wrote:

On 23/08/2012 18:43, Mandy Chung wrote:
This change is to bring the jdk modularization changes from jigsaw 
repo [1]

to jdk8.  This allows the jdk modularization changes to be exposed for
testing as early as possible and minimize the amount of changes carried
in the jigsaw repo that helps sync up jigsaw with jdk8/jdk9.

Webrev at:
http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7193339/webrev.00/


This looks good to me and it's good to have these changes in jdk8.

One suggestion for ReflectUtil is to add a private static boolean 
isAncestor method as I think that would make the checks in 
needsPackageAccessCheck easier to read. Also in ClassLoader you could 
use just use needsClassLoaderPermissionCheck(from,to).




Done.  This is a good cleanup I considered to do too but missed in the 
previous webrev.


http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7193339/webrev.01/

Thanks for the review.
Mandy

Re: Review Request: 7193339 Prepare system classes be defined by a non-null module loader

[Alan Bateman]

On 23/08/2012 18:43, Mandy Chung wrote:
This change is to bring the jdk modularization changes from jigsaw 
repo [1]

to jdk8.  This allows the jdk modularization changes to be exposed for
testing as early as possible and minimize the amount of changes carried
in the jigsaw repo that helps sync up jigsaw with jdk8/jdk9.

Webrev at:
  http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7193339/webrev.00/


This looks good to me and it's good to have these changes in jdk8.

One suggestion for ReflectUtil is to add a private static boolean 
isAncestor method as I think that would make the checks in 
needsPackageAccessCheck easier to read. Also in ClassLoader you could 
use just use needsClassLoaderPermissionCheck(from,to).


-Alan.

Review Request: 7193339 Prepare system classes be defined by a non-null module loader

[Mandy Chung]

This change is to bring the jdk modularization changes from jigsaw repo [1]
to jdk8.  This allows the jdk modularization changes to be exposed for
testing as early as possible and minimize the amount of changes carried
in the jigsaw repo that helps sync up jigsaw with jdk8/jdk9.

Webrev at:
  http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7193339/webrev.00/

Summary:
The VM bootstrap class loader (null) has been the defining class loader
for all system classes (i.e. rt.jar and any classes on the bootclasspath).

In modular world, the system classes are going to be modularized into
multiple modulesand and many of which will be loaded by its own
(non-null) module loader.

The JDK implementation has the assumption that the defining class loader
of system classes is null and it'll no longer be true when running as
modules. Typical patterns in the JDK code include:

Class.forName(classname, false, null) should be changed to:
   Class.forName(classname, false,)

if (loader == null) condition check should be modified to check if the loader
is responsible for loading system classes.

This is the first set of change for this problem we identified. Similar
change in other components will be made in the future.

Testing: run all jdk core test targets on all platforms.

Thanks
Mandy
P.S. I'm cc'ing servicebility-dev as this patch modifies 2 files in
the java.lang.management.

Re: Please Review: 6984084 (str) n times repetition of character constructor for java.lang.String

[Ulf Zibis]

Hopefully some day we would have fixed:
Bug 6373386  - Method chaining for 
instance methods that return void


That would prevent us from such bloated sub classes.

Hopefully such "workarounds" would not force later incompatibilities

-Ulf


Am 23.08.2012 02:35, schrieb David Holmes:

On 22/08/2012 11:52 AM, David Holmes wrote:

StringBuilder.java

/**
+ * @throws IllegalArgumentException if n < 0 {@inheritDoc}

This is wrong as you've already written the inherited doc "if n < 0".

+ * @since 1.8
+ */
+ public StringBuilder append(int n, CharSequence cs) {
+ super.append(n, cs);
+ return this;
+ }

But why are you overriding this in the first place ??? There seems to be
no change in functionality or specification.


I missed the covariant change to the return type from AbstractStringBuilder to 
StringBuilder.

I also suggested in email that the above could simply be:

  return super.append(n, cs);

but that would also need a cast to StringBuilder. So what Jim has is fine.

David
-

hg: jdk8/tl/jdk: 7191587: (se) SelectionKey.interestOps does not defer changing the interest set to the next select [macosx]

[alan . bateman]

Changeset: de5a85353f4d
Author:alanb
Date:  2012-08-23 13:07 +0100
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/de5a85353f4d

7191587: (se) SelectionKey.interestOps does not defer changing the interest set 
to the next select [macosx]
Reviewed-by: alanb
Contributed-by: Jason T Greene 

! src/macosx/classes/sun/nio/ch/KQueueArrayWrapper.java
! src/macosx/classes/sun/nio/ch/KQueueSelectorImpl.java

Re: Improve registering signal handlers in java.lang.Terminator.setup()

[David Holmes]

On 23/08/2012 6:37 PM, Frank Ding wrote:

Hi David,
Sorry that Jonathan has already committed the code with bug id 7193463.
Could you please dup the 2 bugs?


Alan has already taken care of it.

David


Best regards,
Frank

On 8/23/2012 4:27 PM, David Holmes wrote:

On 23/08/2012 6:05 PM, Frank Ding wrote:

Thanks all. I created sun bug 7193463 to track the code change.


We already have 7189865 for this.

David


Best regards,
Frank

On 8/22/2012 10:13 AM, David Holmes wrote:

On 22/08/2012 12:11 PM, Frank Ding wrote:

Hi Alan and David,
So is it OK to commit the patch in Terminator class? I think Neil will
continue proposing the change in cvmi mailing list.


As Alan already said:

"Both David and I have already reviewed Frank's change and okay with
it."

Cheers,
David


Many thanks to all.

Best regards,
Frank

On 8/17/2012 4:58 PM, Alan Bateman wrote:

On 16/08/2012 16:32, Neil Richards wrote:

:

However, I still consider that VM modification would be logically
orthogonal to Frank's suggested change, and suggest that his change
could continue to be approved for contribution at this point,
regardless
of the separate VM-related discussion in this area.


Both David and I have already reviewed Frank's change and okay
with it.

I'll leave it to you whether you want to propose a change to
JVM_RegisterSignal.

-Alan

Re: Improve registering signal handlers in java.lang.Terminator.setup()

[Frank Ding]

Hi David,
  Sorry that Jonathan has already committed the code with bug id 
7193463.  Could you please dup the 2 bugs?


Best regards,
Frank

On 8/23/2012 4:27 PM, David Holmes wrote:

On 23/08/2012 6:05 PM, Frank Ding wrote:

Thanks all. I created sun bug 7193463 to track the code change.


We already have 7189865 for this.

David


Best regards,
Frank

On 8/22/2012 10:13 AM, David Holmes wrote:

On 22/08/2012 12:11 PM, Frank Ding wrote:

Hi Alan and David,
So is it OK to commit the patch in Terminator class? I think Neil will
continue proposing the change in cvmi mailing list.


As Alan already said:

"Both David and I have already reviewed Frank's change and okay with 
it."


Cheers,
David


Many thanks to all.

Best regards,
Frank

On 8/17/2012 4:58 PM, Alan Bateman wrote:

On 16/08/2012 16:32, Neil Richards wrote:

:

However, I still consider that VM modification would be logically
orthogonal to Frank's suggested change, and suggest that his change
could continue to be approved for contribution at this point,
regardless
of the separate VM-related discussion in this area.

Both David and I have already reviewed Frank's change and okay 
with it.


I'll leave it to you whether you want to propose a change to
JVM_RegisterSignal.

-Alan

hg: jdk8/tl/jdk: 7193463: Improve registering signal handlers in java.lang.Terminator.setup()

[luchsh]

Changeset: e7b53fe85540
Author:dingxmin
Date:  2012-08-23 16:28 +0800
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/e7b53fe85540

7193463: Improve registering signal handlers in java.lang.Terminator.setup()
Reviewed-by: dholmes, alanb

! src/solaris/classes/java/lang/Terminator.java
! src/windows/classes/java/lang/Terminator.java

Re: Improve registering signal handlers in java.lang.Terminator.setup()

[David Holmes]

On 23/08/2012 6:05 PM, Frank Ding wrote:

Thanks all. I created sun bug 7193463 to track the code change.


We already have 7189865 for this.

David


Best regards,
Frank

On 8/22/2012 10:13 AM, David Holmes wrote:

On 22/08/2012 12:11 PM, Frank Ding wrote:

Hi Alan and David,
So is it OK to commit the patch in Terminator class? I think Neil will
continue proposing the change in cvmi mailing list.


As Alan already said:

"Both David and I have already reviewed Frank's change and okay with it."

Cheers,
David


Many thanks to all.

Best regards,
Frank

On 8/17/2012 4:58 PM, Alan Bateman wrote:

On 16/08/2012 16:32, Neil Richards wrote:

:

However, I still consider that VM modification would be logically
orthogonal to Frank's suggested change, and suggest that his change
could continue to be approved for contribution at this point,
regardless
of the separate VM-related discussion in this area.


Both David and I have already reviewed Frank's change and okay with it.

I'll leave it to you whether you want to propose a change to
JVM_RegisterSignal.

-Alan

Re: Improve registering signal handlers in java.lang.Terminator.setup()

[Frank Ding]

Thanks all.  I created sun bug 7193463 to track the code change.

Best regards,
Frank

On 8/22/2012 10:13 AM, David Holmes wrote:

On 22/08/2012 12:11 PM, Frank Ding wrote:

Hi Alan and David,
So is it OK to commit the patch in Terminator class? I think Neil will
continue proposing the change in cvmi mailing list.


As Alan already said:

"Both David and I have already reviewed Frank's change and okay with it."

Cheers,
David


Many thanks to all.

Best regards,
Frank

On 8/17/2012 4:58 PM, Alan Bateman wrote:

On 16/08/2012 16:32, Neil Richards wrote:

:

However, I still consider that VM modification would be logically
orthogonal to Frank's suggested change, and suggest that his change
could continue to be approved for contribution at this point, 
regardless

of the separate VM-related discussion in this area.


Both David and I have already reviewed Frank's change and okay with it.

I'll leave it to you whether you want to propose a change to
JVM_RegisterSignal.

-Alan