> On Nov 27, 2016, at 6:12 PM, Alan Bateman <alan.bate...@oracle.com> wrote: > > On 26/11/2016 08:54, Wang Weijun wrote: > >> Please take a review at >> >> http://cr.openjdk.java.net/~weijun/8170364/webrev.00/ >> >> The compatibility layer introduced in the new FilePermission implementation >> requires one FilePermission to imply another with either a relative path or >> an absolute path. This is solved with a private field npath2 inside. When >> this field is set, the permission's name is modified a little (JDK-8168127) >> so that when adding to a FilePermissionCollection, it is not confused with >> one without the field. Unfortunately, this modified name is reused in the >> merge to create a new "merged" FilePermission which contains a malformed >> path now. >> >> This fix creates a new non-public method to create this "merged" >> FilePermission. >> >> I checked other places and seems the name is not used to create a new >> FilePermission. >> > Ideally FilePermission (and all permissions) would have final fields, I hope > that can be fixed some day.
Me too, but we have that huge init() method now. > In the mean-time then having withNewActions create a new FilePermission and > then mutate it looks a bit ugly, can't you just introduce a new non-public > constructor to create it with the effective mask? The private "clone" constructor is now used in 3 places, all with a mutation right after (lines 267, 280, 993). I've taken care that only newly created objects are mutated this way. Creating 3 new constructors looks like too many duplicated codes. > > In passing then maybe the comment at L1063-1065 can be re-examined and it > looks to be stale (the specific bootstrapping issue mentioned was fixed in > 2015). I can try. > > The test is one specific scenario and I wonder if you've considered beefing > this up to other scenarios and other targets so that it's more broadly > testing the merging scenarios. I can add 2 more lines on checking the absolute path, and maybe with delete, execute and readlink (1+1+1+1, 2+2, 3+1 etc). The merge is only about the name and any name is the same, so it seems unnecessary to try other names. Thanks Max > > -Alan > >
