RFR 8213031: Enhance jdk.nio.zipfs to support Posix File Permissions

[Langer, Christoph]

Hi Andrew,

you might have a point here. I'll revisit that.

Thanks
Christoph

> -----Original Message-----
> From: Andrew Luo <andrewluotechnologies at outlook.com>
> Sent: Freitag, 26. Oktober 2018 20:43
> To: Langer, Christoph <christoph.langer at sap.com>; 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: 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 <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
> 
> 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