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

[Langer, Christoph]

Hi,

I’ve amended the jdk.zipfs module documentation in src/jdk.zipfs/share/classes/module-info.java to document the new behavior (e.g. support of PosixFileAttributeView) as requested by Alan. I’ve also updated the CSR.

Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8213031.4/
CSR: https://bugs.openjdk.java.net/browse/JDK-8213082

Please review both, CSR and changeset and let me know if this is good now or what else needs to be addressed.

Thanks and best regards,
Christoph

From: Volker Simonis <volker.simonis at gmail.com>
Sent: Freitag, 21. Dezember 2018 17:34
To: Langer, Christoph <christoph.langer at sap.com>
Cc: Java Core Libs <core-libs-dev at openjdk.java.net>; security-dev <security-dev at openjdk.java.net>; SHEN, XUEMING <xueming.shen at oracle.com>; Alan Bateman <Alan.Bateman at oracle.com>; nio-dev <nio-dev at openjdk.java.net>
Subject: Re: RFR 8213031: (zipfs) Add support for POSIX file permissions

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<mailto: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/20190107/72c1b7fd/attachment.htm>