RFR 8229888: Zip FS does not retain permissions when updating a Zip file

Tue Dec 17 08:24:15 UTC 2019

Hi Lance,

I checked your patch. I think it is correct, I only have a few style nits which you may or may not address (no need for further webrev, though):

src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java:
Line 1793: Maybe the comment should rather be: // Set the POSIX permissions of the original Zip File if available
Line 1804: The path to the file (need not necessarily be a zip file)
Line 1806: I would indent the comment to the same column as "The" in the row above - makes it more readable. Also, there is a space too much between "the" and "POSIX"
Line 1808: Indention as above
Line 1811: Remove
Line 1816: Insert blank between if and (

test/jdk/jdk/nio/zipfs/ZipFSPermissionsTest.java:
Line 68: Remove
Line 85: Remove
Method teardown, line 97: I think this method is not needed. zipFile is already deleted before each test in method "before()" and in the end you clean up via "suiteCleanUp()". So maybe tearDown can/should be removed?
Line 113: Maybe you should move "Zip file" to the line before while keeping this line as a blank line
Line 115: Same as with 113
Line 120: Space between if and (
Line 181: Same as in 120
Method getPosixAttributes, line 199: I think you should remove the try/catch UnsupportedOperationException. If the test is run on a non-posix fs, the whole test is skipped in setup() anyways.

Cheers
Christoph

From: nio-dev <nio-dev-bounces at openjdk.java.net> On Behalf Of Lance Andersen
Sent: Donnerstag, 12. Dezember 2019 17:48
Cc: nio-dev <nio-dev at openjdk.java.net>
Subject: Re: RFR 8229888: Zip FS does not retain permissions when updating a Zip file

Attached is the updated patch for addressing the permissions based on feedback from Alan:  http://cr.openjdk.java.net/~lancea/8229888/webrev.02/index.html

Best
Lance

On Dec 9, 2019, at 1:28 PM, Lance Andersen <lance.andersen at oracle.com<mailto:lance.andersen at oracle.com>> wrote:

Here is a revised patch: http://cr.openjdk.java.net/~lancea/8229888/webrev.01/index.html.

I spent some time trying leverage createTempFileinSameDirectoryAs but it was getting messy as this method is also used when creating temp files in a zip to zip copy.    I might try to revisit this later but the above seemed more straight forwarded

Best
Lance

On Dec 3, 2019, at 6:45 PM, Lance Andersen <lance.andersen at oracle.com<mailto:lance.andersen at oracle.com>> wrote:

Thank you for the comments Alan.

On Dec 3, 2019, at 4:04 PM, Alan Bateman <Alan.Bateman at oracle.com<mailto:Alan.Bateman at oracle.com>> wrote:

On 03/12/2019 00:59, Lance Andersen wrote:

HI all,

Please review the proposed fix for an issue with Zip FS where the permissions set on the Zip file are not retained when the Zip file is updated.

The webrev can be found at: http://cr.openjdk.java.net/~lancea/8229888/webrev.00/index.html
Testing the underlying FileStore to see if supports "posix" may be problematic or costly here as it requires finding the mount entry file the underlying file store. You should find that it is much more efficient to just call getPosixFileAttributes
Did you mean:  Files.getPosixFilePermissions

and catch UOE (using exceptions for control flow is not nice of course but you will avoid a lot of issues). In passing, I should mention that the non-stringy way to to check if POSIX file permissions are support is to invoke on supportsFileAttributeView(PosixFIleAttributeView.class).

Would the following be more efficient than catching the IOE as I saw it used in TempFileHelper:

--------
 boolean isPosix = FileSystems.getDefault().supportedFileAttributeViews().contains("posix");
--------

One other approach to look at is just letting Files.move copy the file attributes. It will copy file permissions and attempt to copy all other attributes (it only fails if it can't copy the basic file attributes to the file in its new location). Another point is that the Files.newOutputStream to create the file next to the original zip file can be created with initial file attributes. Combined with Files.move it means there is another way to do this. I only mention is because I assume there will be follow on issues to fix up other file attributes of the zip file that are lost when it is re-created.

If I am understanding this suggestion, it would be to modify the call to Files.createTempFile in the method createTempFileinSameDirectoryAs to pass the  original zip file permissions as an file attribute ?

Best
Lance

-Alan

<oracle_sig_logo.gif><http://oracle.com/us/design/oracle-email-sig-198324.gif>

<http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
Lance.Andersen at oracle.com<mailto:Lance.Andersen at oracle.com>

<oracle_sig_logo.gif><http://oracle.com/us/design/oracle-email-sig-198324.gif>

<http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
Lance.Andersen at oracle.com<mailto:Lance.Andersen at oracle.com>

[cid:image001.gif at 01D5B4B9.AC6B4BC0]<http://oracle.com/us/design/oracle-email-sig-198324.gif>

<http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
Lance.Andersen at oracle.com<mailto:Lance.Andersen at oracle.com>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/nio-dev/attachments/20191217/764d5d91/attachment-0001.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: image001.gif
Type: image/gif
Size: 658 bytes
Desc: image001.gif
URL: <https://mail.openjdk.java.net/pipermail/nio-dev/attachments/20191217/764d5d91/image001-0001.gif>