RFR 8213031: Enhance jdk.nio.zipfs to support Posix File Permissions
andrewluotechnologies at outlook.com
Fri Oct 26 18:42:42 UTC 2018
I'm not a committer/reviewer but noticed something:
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)?
From: core-libs-dev <core-libs-dev-bounces at openjdk.java.net> On Behalf Of Langer, Christoph
Sent: Friday, October 26, 2018 8:13 AM
To: core-libs-dev <core-libs-dev at openjdk.java.net>; nio-dev <nio-dev at openjdk.java.net>; Xueming Shen <xueming.shen at oracle.com>
Cc: Schmelter, Ralf <ralf.schmelter at sap.com>
Subject: RFR 8213031: Enhance jdk.nio.zipfs to support Posix File Permissions
please review this enhancement of jdk.nio.zipfs to support Posix file permissions.
I had already posted this change as part of a larger fix for "6194856: Zip Files lose ALL ownership and permissions of the files" . 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.
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
More information about the core-libs-dev