RFR 8213031: (zipfs) Add support for POSIX file permissions

[Volker Simonis]

Hi Christoph,

thanks for updating the change. I think it is in a good state now and ready
to go!

Also the documentation in the CSR for this issue (
https://bugs.openjdk.java.net/browse/JDK-8213082) is greatly appreciated
and answers all the questions which have been raised so far. So if there
are still any open questions I'd recommend that any potential reviewer
consults the CSR at https://bugs.openjdk.java.net/browse/JDK-8213082 first.

Thank you and best regards,
Volker


On Fri, Dec 21, 2018 at 3:31 PM Langer, Christoph <christoph.langer at sap.com>
wrote:

> Hi all,
>
> here comes the updated webrev:
> http://cr.openjdk.java.net/~clanger/webrevs/8213031.3/
>
> I've rebased the change to the current state of the JDK depot. Thanks to
> Volker, the test has been enhanced and now also tests more copy operations
> (from zip file system to zip file system and from zip file system to
> default file system and back).
>
> The points below were also addressed:
>
> > ZipFileAttributeView.java
> >  - can you please throw an AssertionError() for the default branch in
> > the switch expression in the "ZipFileAttributeView.name()".
>
> The default branch will return "basic". Looking at the code it should not
> be jumped to anyway.
>
> > ZipFileSystem.java
> >  - please also put @Override annotations on the method implementations
> > from ZipFileAttributes (e.g. "compressedSize()", "crc()", etc...) and
> > a comment at the place where the implementation of the
> > "PosixFileAttributes" methods starts.
>
> Done, I also reordered the methods - first "basic" then "posix" then "zip".
>
> > ZipUtils.java
> > - just a question: isn't it possible to reuse
> > sun.nio.fs.UnixFileModeAttribute.toUnixMode() and the corresponding
> > constants from sun.nio.fs.UnixConstants instead of redefining them
> > here? You could export them from java.base to jdk.zipfs but the
> > problem is probably that at least sun.nio.fs.UnixConstants is a
> > generated class which only gets generated on Unix systems, right ?
>
> You've already answered yourself 😊 These classes only exist on Unix type
> JDKs.
>
> > ZipFileAttributes.java
> > - it seems a little odd that you've added "setPermissions()" to
> > ZipFileAttributes. All the attribute classes (i.e.
> > BasicFileAttributes, PosixFileAttributes) have no setters. Isn't it
> > possible to implement "ZipFileAttributeView.setPermissions()" by
> > calling "path.setPermissions()" (as this is done in
> > "ZipFileAttributeView.setTimes()") and handle everything in
> > ZipFileSyste (as this is done with "setTimes()").
>
> Good catch & thanks for providing the better implementation.
>
>
> I think this version is quite final now and thoroughly tested. I hope to
> get some valid reviews soon...
>
> Thanks
> Christoph
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20181221/33b8260b/attachment.htm>