Hi Andrew, you might have a point here. I'll revisit that.
Thanks Christoph > -----Original Message----- > From: Andrew Luo <[email protected]> > Sent: Freitag, 26. Oktober 2018 20:43 > To: Langer, Christoph <[email protected]>; core-libs-dev <core-libs- > [email protected]>; nio-dev <[email protected]>; Xueming > Shen <[email protected]> > Cc: Schmelter, Ralf <[email protected]> > Subject: RE: RFR 8213031: Enhance jdk.nio.zipfs to support Posix File > Permissions > > I'm not a committer/reviewer but noticed something: > > In ZipUtils.java: > > private static final Map<Integer,Set<PosixFilePermission>> permCache = > new WeakHashMap<>(); > > This is static, and is concurrently modified, so will it cause issues if > multiple > threads are operating on ZIP filesystems (even if they are different > objects/ZIP files)? > > Thanks, > > -Andrew > > -----Original Message----- > From: core-libs-dev <[email protected]> On Behalf > Of Langer, Christoph > Sent: Friday, October 26, 2018 8:13 AM > To: core-libs-dev <[email protected]>; nio-dev <nio- > [email protected]>; Xueming Shen <[email protected]> > Cc: Schmelter, Ralf <[email protected]> > Subject: RFR 8213031: Enhance jdk.nio.zipfs to support Posix File Permissions > > Hi, > > please review this enhancement of jdk.nio.zipfs to support Posix file > permissions. > > Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8213031.0/ > Bug: https://bugs.openjdk.java.net/browse/JDK-8213031 > > I had already posted this change as part of a larger fix for "6194856: Zip > Files > lose ALL ownership and permissions of the files" [1]. With the original > proposal I was also addressing the java.util.zip API and the jar tool. > However, > I've decided to split this endeavor into 2 parts. While I still want to > follow up > on java.util.zip and jar, I'd like to bring the enhancement to jdk.zipfs in > first. > Both places don't have dependencies to each other and since zipfs does not > change an external API, I guess the bar here is lower. Maybe it is even a > candidate for down-porting to jdk11u in the future. > > After my fix, zipfs will support the PosixFileAttributeView. Posix file > attributes can't be fully supported since owner and group are not handled in > zip files. So methods supposed to get these attributes will return an > UnsupportedOperationException. But the posix permissions will now be > correctly handled, that is stored into and read from the zip file. > > @Sherman: > Following suggestions from your review, I removed the test with the binary > zip file. I initially thought it is a good idea to test on a well-known file. > However, testWriteAndReadArchiveWithPosixPerms tests both, writing a zip > file and reading it again so I guess test coverage is quite complete here. > Furthermore, I made some public declarations in ZipUtils package private > which should suffice. > I also tried to address your performance concerns with permsToFlags and > permsFromFlags. In permsToFlags I will now simply iterate to the set of > permissions and add the flags to the return value. It's probably more > performant than the streaming approach and doesn't look much worse in the > code. In permsFromFlags I added a cache of permission sets which should > avoid constant calls to the streaming. However, as the return value needs to > be mutable, I have to clone the cached permissions. Maybe it would have > the same or even better performance if we iteratively fill a new set of > permissions each time permsToFlags gets called. What do you think? > > Do we need a CSR for this patch? > > Thanks & Best regards > Christoph > > [1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2018- > October/055971.html
