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
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
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 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
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
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 : 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
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
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
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 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
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
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