[8] Review Request for 8007292 : Add JavaFX internal packages to package.access

Erik Joelsson erik.joelsson at oracle.com
Thu Oct 10 09:57:53 UTC 2013


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




More information about the security-dev mailing list