Re: [8] Review Request for 8007292 : Add JavaFX internal packages to package.access
On 2013-10-10 19:45, Sean Mullan wrote: On 10/10/2013 05:57 AM, Erik Joelsson wrote: Adding makefiles to make/tools is not needed for the new build. Either remove those changes or make a complete port to the old build. I'm not pushing for porting this to the old build at this point since missing this will not cause the old build to stop working, it will just diverge the resulting bits a bit more. I have ported the changes to the old build, please review. New webrev: http://cr.openjdk.java.net/~mullan/webrevs/8007292/webrev.02/ The only modified file is make/java/security/Makefile Thanks, looks good now! /Erik
Re: [8] Review Request for 8007292 : Add JavaFX internal packages to package.access
Hello,
While you discuss TWR hazards, be aware that closing a sink (OutputStream)
as part of a finally branch which hides exceptions is dangerous in some
I/O scenarios because hardware write errors could be delayed until the
close. NFS for example has this tendency, but also some virtual
filesystems do a lot of commit work on close. So typically you add a close
inside the try as well.
Greetings
Bernd
Am 10.10.2013, 04:36 Uhr, schrieb Joseph Darcy joe.da...@oracle.com:
It is a hazard (I thought I had published a blog entry on this very
tropic, but apparently not). The most robust pattern is
try(OriginalResource r1 = new OriginalResource;
WrappingResource r2 = new WrappingResource(r1);
AnnotherWrappingResource r3 = new WrappingResource(r2)) { ...}
One thing to watch out for in this pattern is a non-idempotent close.
Calling close on r3 will presumably propagate a close call to r2, and
then r2 to r1. So give the desguaring the try-with-resource and the
expected behavior of the wrapping in a normal termination situation,
close on r3 gets called once, close on r2 gets called twice (first from
the close on r3, second close from try-with-resources), and close on r1
gets called three times.
HTH,
-Joe
--
http://bernd.eckenfels.net
Re: [8] Review Request for 8007292 : Add JavaFX internal packages to package.access
Adding makefiles to make/tools is not needed for the new build. Either
remove those changes or make a complete port to the old build. I'm not
pushing for porting this to the old build at this point since missing
this will not cause the old build to stop working, it will just diverge
the resulting bits a bit more.
/Erik
On 2013-10-09 19:19, Sean Mullan wrote:
Updated webrev:
http://cr.openjdk.java.net/~mullan/webrevs/8007292/webrev.01/
Let me know if there are any more comments, otherwise I will plan to
push tomorrow.
Thanks,
Sean
On 10/09/2013 09:20 AM, Sean Mullan wrote:
On 10/09/2013 05:14 AM, Erik Joelsson wrote:
Hello Sean,
On 2013-10-09 06:33, David Holmes wrote:
Hi Sean,
Not a full review.
On 9/10/2013 5:52 AM, Sean Mullan wrote:
Please review the fix for the following bug:
https://bugs.openjdk.java.net/browse/JDK-8007292
This bug requires build changes and a new build tool to add
additional
restricted packages to the java.security file which are not part of
OpenJDK. These packages are only added when doing a build
including the
open and closed sources.
The restricted packages and new test are in the closed sources and
will
be reviewed separately.
webrev: http://cr.openjdk.java.net/~mullan/webrevs/8007292/webrev.00/
Based on your description and the ifndef OPENJDK it sounds to me that
this doesn't belong in the OpenJDK makefiles.
I agree in principle, but the pattern on how to handle this isn't well
established yet and something we need to improve on. In this case we
would need to make changes on the open side anyway to provide hooks for
overrides of the behavior. I'm willing to accept this for now.
Thanks. Also, keep in mind that this hook allows each vendor,etc to add
additional proprietary or internal packages to the restricted packages
properties for their own build. So I think it is useful in general in
that respect.
That aside I would think the CP+RM could be changed to a MV.
Agreed.
Right. Will do.
I would prefer the TOOL_ADDTO... line to be moved to
jdk/makefiles/Tools.gmk with the other similar definitions, even if it
is only used here atm.
Ok.
In the tool this code doesn't show correct use of try-with-resources:
51 try (BufferedReader br = new BufferedReader(new
FileReader(args[0]));
52 BufferedWriter bw = new BufferedWriter(new
FileWriter(args[1]))) {
The FileReader and FileWriter should also be covered by TWR:
try (FileReader fr = new FileReader(args[0]);
BufferedReader br = new BufferedReader(fr);
FileWriter fw = new FileWriter(args[1]);
BufferedWriter bw = new BufferedWriter(fw)) {
I'm not familiar with the try-with-resources, but calling close on a
BufferedReader/writer will close the underlying reader/writer so
nothing
will be left open, will it not?
That's what I thought as well. David?
Finally do we still use make/tools/Makefile in the new build?
No, we don't. If you want to add old build support for this, you would
also need to add usage of the tool to the correct old makefile.
I don't think it's necessary to add this to the old build at this point.
I'll post another webrev later in the day with these updates.
Thanks,
Sean
Re: [8] Review Request for 8007292 : Add JavaFX internal packages to package.access
On 2013-10-09 15:20, Sean Mullan wrote: Finally do we still use make/tools/Makefile in the new build? No, we don't. If you want to add old build support for this, you would also need to add usage of the tool to the correct old makefile. I don't think it's necessary to add this to the old build at this point. I don't really get this. You have made some changes to the old build system (changes in make/tools/Makefile and the addition of make/tools/addtorestrictedpkgs/Makefile), but you are not actually using this. I think you should either drop these changes as well, or fix it so that the old build is usable too. Doing it half-way seems just confusing. Also, are we -- in general -- okay with making changes only to the new build system? As long as both systems are still there, I thought the idea was to keep them in sync. Even though we want to remove the old build, we don't know when that happens, and we *don't* want the systems do diverge. Or are there precedence for this kind of changes? /Magnus
Re: [8] Review Request for 8007292 : Add JavaFX internal packages to package.access
On 10/10/2013 05:57 AM, Erik Joelsson wrote: Adding makefiles to make/tools is not needed for the new build. Either remove those changes or make a complete port to the old build. I'm not pushing for porting this to the old build at this point since missing this will not cause the old build to stop working, it will just diverge the resulting bits a bit more. I have ported the changes to the old build, please review. New webrev: http://cr.openjdk.java.net/~mullan/webrevs/8007292/webrev.02/ The only modified file is make/java/security/Makefile Thanks, Sean
Re: [8] Review Request for 8007292 : Add JavaFX internal packages to package.access
On 10/10/13 10:45 AM, Sean Mullan wrote: On 10/10/2013 05:57 AM, Erik Joelsson wrote: Adding makefiles to make/tools is not needed for the new build. Either remove those changes or make a complete port to the old build. I'm not pushing for porting this to the old build at this point since missing this will not cause the old build to stop working, it will just diverge the resulting bits a bit more. I have ported the changes to the old build, please review. New webrev: http://cr.openjdk.java.net/~mullan/webrevs/8007292/webrev.02/ The only modified file is make/java/security/Makefile Makefile looks good to me. As for when the old makefiles are removed, we don't know for sure yet, but the time is getting closer as there are fewer stopper issues with the removal. Tim
Re: [8] Review Request for 8007292 : Add JavaFX internal packages to package.access
Hello Sean,
On 2013-10-09 06:33, David Holmes wrote:
Hi Sean,
Not a full review.
On 9/10/2013 5:52 AM, Sean Mullan wrote:
Please review the fix for the following bug:
https://bugs.openjdk.java.net/browse/JDK-8007292
This bug requires build changes and a new build tool to add additional
restricted packages to the java.security file which are not part of
OpenJDK. These packages are only added when doing a build including the
open and closed sources.
The restricted packages and new test are in the closed sources and will
be reviewed separately.
webrev: http://cr.openjdk.java.net/~mullan/webrevs/8007292/webrev.00/
Based on your description and the ifndef OPENJDK it sounds to me that
this doesn't belong in the OpenJDK makefiles.
I agree in principle, but the pattern on how to handle this isn't well
established yet and something we need to improve on. In this case we
would need to make changes on the open side anyway to provide hooks for
overrides of the behavior. I'm willing to accept this for now.
That aside I would think the CP+RM could be changed to a MV.
Agreed.
I would prefer the TOOL_ADDTO... line to be moved to
jdk/makefiles/Tools.gmk with the other similar definitions, even if it
is only used here atm.
In the tool this code doesn't show correct use of try-with-resources:
51 try (BufferedReader br = new BufferedReader(new
FileReader(args[0]));
52 BufferedWriter bw = new BufferedWriter(new
FileWriter(args[1]))) {
The FileReader and FileWriter should also be covered by TWR:
try (FileReader fr = new FileReader(args[0]);
BufferedReader br = new BufferedReader(fr);
FileWriter fw = new FileWriter(args[1]);
BufferedWriter bw = new BufferedWriter(fw)) {
I'm not familiar with the try-with-resources, but calling close on a
BufferedReader/writer will close the underlying reader/writer so nothing
will be left open, will it not?
Finally do we still use make/tools/Makefile in the new build?
No, we don't. If you want to add old build support for this, you would
also need to add usage of the tool to the correct old makefile.
/Erik
Re: [8] Review Request for 8007292 : Add JavaFX internal packages to package.access
On 10/09/2013 05:14 AM, Erik Joelsson wrote:
Hello Sean,
On 2013-10-09 06:33, David Holmes wrote:
Hi Sean,
Not a full review.
On 9/10/2013 5:52 AM, Sean Mullan wrote:
Please review the fix for the following bug:
https://bugs.openjdk.java.net/browse/JDK-8007292
This bug requires build changes and a new build tool to add additional
restricted packages to the java.security file which are not part of
OpenJDK. These packages are only added when doing a build including the
open and closed sources.
The restricted packages and new test are in the closed sources and will
be reviewed separately.
webrev: http://cr.openjdk.java.net/~mullan/webrevs/8007292/webrev.00/
Based on your description and the ifndef OPENJDK it sounds to me that
this doesn't belong in the OpenJDK makefiles.
I agree in principle, but the pattern on how to handle this isn't well
established yet and something we need to improve on. In this case we
would need to make changes on the open side anyway to provide hooks for
overrides of the behavior. I'm willing to accept this for now.
Thanks. Also, keep in mind that this hook allows each vendor,etc to add
additional proprietary or internal packages to the restricted packages
properties for their own build. So I think it is useful in general in
that respect.
That aside I would think the CP+RM could be changed to a MV.
Agreed.
Right. Will do.
I would prefer the TOOL_ADDTO... line to be moved to
jdk/makefiles/Tools.gmk with the other similar definitions, even if it
is only used here atm.
Ok.
In the tool this code doesn't show correct use of try-with-resources:
51 try (BufferedReader br = new BufferedReader(new
FileReader(args[0]));
52 BufferedWriter bw = new BufferedWriter(new
FileWriter(args[1]))) {
The FileReader and FileWriter should also be covered by TWR:
try (FileReader fr = new FileReader(args[0]);
BufferedReader br = new BufferedReader(fr);
FileWriter fw = new FileWriter(args[1]);
BufferedWriter bw = new BufferedWriter(fw)) {
I'm not familiar with the try-with-resources, but calling close on a
BufferedReader/writer will close the underlying reader/writer so nothing
will be left open, will it not?
That's what I thought as well. David?
Finally do we still use make/tools/Makefile in the new build?
No, we don't. If you want to add old build support for this, you would
also need to add usage of the tool to the correct old makefile.
I don't think it's necessary to add this to the old build at this point.
I'll post another webrev later in the day with these updates.
Thanks,
Sean
Re: [8] Review Request for 8007292 : Add JavaFX internal packages to package.access
Updated webrev:
http://cr.openjdk.java.net/~mullan/webrevs/8007292/webrev.01/
Let me know if there are any more comments, otherwise I will plan to
push tomorrow.
Thanks,
Sean
On 10/09/2013 09:20 AM, Sean Mullan wrote:
On 10/09/2013 05:14 AM, Erik Joelsson wrote:
Hello Sean,
On 2013-10-09 06:33, David Holmes wrote:
Hi Sean,
Not a full review.
On 9/10/2013 5:52 AM, Sean Mullan wrote:
Please review the fix for the following bug:
https://bugs.openjdk.java.net/browse/JDK-8007292
This bug requires build changes and a new build tool to add additional
restricted packages to the java.security file which are not part of
OpenJDK. These packages are only added when doing a build including the
open and closed sources.
The restricted packages and new test are in the closed sources and will
be reviewed separately.
webrev: http://cr.openjdk.java.net/~mullan/webrevs/8007292/webrev.00/
Based on your description and the ifndef OPENJDK it sounds to me that
this doesn't belong in the OpenJDK makefiles.
I agree in principle, but the pattern on how to handle this isn't well
established yet and something we need to improve on. In this case we
would need to make changes on the open side anyway to provide hooks for
overrides of the behavior. I'm willing to accept this for now.
Thanks. Also, keep in mind that this hook allows each vendor,etc to add
additional proprietary or internal packages to the restricted packages
properties for their own build. So I think it is useful in general in
that respect.
That aside I would think the CP+RM could be changed to a MV.
Agreed.
Right. Will do.
I would prefer the TOOL_ADDTO... line to be moved to
jdk/makefiles/Tools.gmk with the other similar definitions, even if it
is only used here atm.
Ok.
In the tool this code doesn't show correct use of try-with-resources:
51 try (BufferedReader br = new BufferedReader(new
FileReader(args[0]));
52 BufferedWriter bw = new BufferedWriter(new
FileWriter(args[1]))) {
The FileReader and FileWriter should also be covered by TWR:
try (FileReader fr = new FileReader(args[0]);
BufferedReader br = new BufferedReader(fr);
FileWriter fw = new FileWriter(args[1]);
BufferedWriter bw = new BufferedWriter(fw)) {
I'm not familiar with the try-with-resources, but calling close on a
BufferedReader/writer will close the underlying reader/writer so nothing
will be left open, will it not?
That's what I thought as well. David?
Finally do we still use make/tools/Makefile in the new build?
No, we don't. If you want to add old build support for this, you would
also need to add usage of the tool to the correct old makefile.
I don't think it's necessary to add this to the old build at this point.
I'll post another webrev later in the day with these updates.
Thanks,
Sean
Re: [8] Review Request for 8007292 : Add JavaFX internal packages to package.access
cc'ing Joe Darcy. :)
Joe: there is a try-with-resources question for you below ...
On 9/10/2013 11:20 PM, Sean Mullan wrote:
On 10/09/2013 05:14 AM, Erik Joelsson wrote:
On 2013-10-09 06:33, David Holmes wrote:
In the tool this code doesn't show correct use of try-with-resources:
51 try (BufferedReader br = new BufferedReader(new
FileReader(args[0]));
52 BufferedWriter bw = new BufferedWriter(new
FileWriter(args[1]))) {
The FileReader and FileWriter should also be covered by TWR:
try (FileReader fr = new FileReader(args[0]);
BufferedReader br = new BufferedReader(fr);
FileWriter fw = new FileWriter(args[1]);
BufferedWriter bw = new BufferedWriter(fw)) {
I'm not familiar with the try-with-resources, but calling close on a
BufferedReader/writer will close the underlying reader/writer so nothing
will be left open, will it not?
That's what I thought as well. David?
It maybe that I am overly pedantic with this but the issue is that with
the original code if the BufferedReader/Writer constructors throw an
exception then the FileReader/Writer that was already created would not
be closed. The revised code accounts for this.
Joe: what is best-practice here? I see a lot of examples of t-w-r where
there is a set of chained I/O streams and only the outermost one is a
t-w-r resource. And that seems wrong to me.
Thanks,
David
Finally do we still use make/tools/Makefile in the new build?
No, we don't. If you want to add old build support for this, you would
also need to add usage of the tool to the correct old makefile.
I don't think it's necessary to add this to the old build at this point.
I'll post another webrev later in the day with these updates.
Thanks,
Sean
Re: [8] Review Request for 8007292 : Add JavaFX internal packages to package.access
On 10/9/2013 6:18 PM, David Holmes wrote:
cc'ing Joe Darcy. :)
Joe: there is a try-with-resources question for you below ...
On 9/10/2013 11:20 PM, Sean Mullan wrote:
On 10/09/2013 05:14 AM, Erik Joelsson wrote:
On 2013-10-09 06:33, David Holmes wrote:
In the tool this code doesn't show correct use of try-with-resources:
51 try (BufferedReader br = new BufferedReader(new
FileReader(args[0]));
52 BufferedWriter bw = new BufferedWriter(new
FileWriter(args[1]))) {
The FileReader and FileWriter should also be covered by TWR:
try (FileReader fr = new FileReader(args[0]);
BufferedReader br = new BufferedReader(fr);
FileWriter fw = new FileWriter(args[1]);
BufferedWriter bw = new BufferedWriter(fw)) {
I'm not familiar with the try-with-resources, but calling close on a
BufferedReader/writer will close the underlying reader/writer so
nothing
will be left open, will it not?
That's what I thought as well. David?
It maybe that I am overly pedantic with this but the issue is that
with the original code if the BufferedReader/Writer constructors throw
an exception then the FileReader/Writer that was already created would
not be closed. The revised code accounts for this.
Joe: what is best-practice here? I see a lot of examples of t-w-r
where there is a set of chained I/O streams and only the outermost one
is a t-w-r resource. And that seems wrong to me.
It is a hazard (I thought I had published a blog entry on this very
tropic, but apparently not). The most robust pattern is
try(OriginalResource r1 = new OriginalResource;
WrappingResource r2 = new WrappingResource(r1);
AnnotherWrappingResource r3 = new WrappingResource(r2)) { ...}
One thing to watch out for in this pattern is a non-idempotent close.
Calling close on r3 will presumably propagate a close call to r2, and
then r2 to r1. So give the desguaring the try-with-resource and the
expected behavior of the wrapping in a normal termination situation,
close on r3 gets called once, close on r2 gets called twice (first from
the close on r3, second close from try-with-resources), and close on r1
gets called three times.
HTH,
-Joe
[8] Review Request for 8007292 : Add JavaFX internal packages to package.access
Please review the fix for the following bug: https://bugs.openjdk.java.net/browse/JDK-8007292 This bug requires build changes and a new build tool to add additional restricted packages to the java.security file which are not part of OpenJDK. These packages are only added when doing a build including the open and closed sources. The restricted packages and new test are in the closed sources and will be reviewed separately. webrev: http://cr.openjdk.java.net/~mullan/webrevs/8007292/webrev.00/ Thanks, Sean
Re: [8] Review Request for 8007292 : Add JavaFX internal packages to package.access
Hi Sean,
Not a full review.
On 9/10/2013 5:52 AM, Sean Mullan wrote:
Please review the fix for the following bug:
https://bugs.openjdk.java.net/browse/JDK-8007292
This bug requires build changes and a new build tool to add additional
restricted packages to the java.security file which are not part of
OpenJDK. These packages are only added when doing a build including the
open and closed sources.
The restricted packages and new test are in the closed sources and will
be reviewed separately.
webrev: http://cr.openjdk.java.net/~mullan/webrevs/8007292/webrev.00/
Based on your description and the ifndef OPENJDK it sounds to me that
this doesn't belong in the OpenJDK makefiles.
That aside I would think the CP+RM could be changed to a MV.
In the tool this code doesn't show correct use of try-with-resources:
51 try (BufferedReader br = new BufferedReader(new
FileReader(args[0]));
52 BufferedWriter bw = new BufferedWriter(new
FileWriter(args[1]))) {
The FileReader and FileWriter should also be covered by TWR:
try (FileReader fr = new FileReader(args[0]);
BufferedReader br = new BufferedReader(fr);
FileWriter fw = new FileWriter(args[1]);
BufferedWriter bw = new BufferedWriter(fw)) {
Finally do we still use make/tools/Makefile in the new build?
David
-
Thanks,
Sean
