RFR: 8322565 (zipfs) Files.setPosixPermissions should preserve 'external file attributes' bits [v2]

[Lance Andersen]

On Tue, 9 Jan 2024 10:22:40 GMT, Eirik Bjørsnøs <eirbjo at openjdk.org> wrote:

>> This PR suggests that `Files.setPosixPermissions`as implemented by `ZipFileSystem` should preserve the leading seven bits of the 'external file attributes' field. These bits contain the 'file type', 'setuid', 'setgid', and 'sticky' bits. These are unrelated to permissions and should not be modified by this operation.
>> 
>> The fix is to update `Entry.readCEN` to read all 16 bits instead of just the trailing 12 and to update `ZipFileSystem.setPermissions` to preserve the leading 7 bits when updating the trailing 9 permission-related bits of the `Entry.posixPerms` field.
>> 
>> The PR adds a new test `TestPosix.preserveRemainingBits()` which verifies that the leading 7 bits are not affected by `Files.setPosixPermissions`. This test also verifies that operations not related to POSIX, such as Files.setLastModifiedTime does not affect the 'external file attributes' value.
>> 
>> Note that this PR does not aim to preserve the leading seven bits for the case when `Files.setPosixPermissions` is called with a `null` permission set. (The implementation currently interprets this as a signal that the 'external file attributes'  should not be populated and the 'version made by' OS will be MSDOS instead of Unix)
>
> Eirik Bjørsnøs has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:
> 
>  - Verify that ZipFileSystem preserves 'external file attribute' bits when performing operations unrelated to POSIX, such as Files.setLastModifiedTime.
>  - Merge branch 'master' into zipfs-preserve-external-file-attrs
>  - Preserve non-permission 'external file attributes' bits when setting posix permissions

Thank you for the PR.  Overall it looks good a few couple nit comments.

Extra credit to convert this from testng to a junit test but not a must

test/jdk/jdk/nio/zipfs/TestPosix.java line 761:

> 759:         Files.write(zipFile, zip);
> 760: 
> 761:         // Verify that a read/synch roundtrip preserves the external file attributes

typo 'synch'

test/jdk/jdk/nio/zipfs/TestPosix.java line 770:

> 768: 
> 769:         // Verify calling Files.setPosixFilePermissions with current permission set
> 770:         try (FileSystem fs = FileSystems.newFileSystem(zipFile, ENV_POSIX)) {

This should be a separate test IMHO

-------------

Marked as reviewed by lancea (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17170#pullrequestreview-1813747144
PR Review Comment: https://git.openjdk.org/jdk/pull/17170#discussion_r1447701846
PR Review Comment: https://git.openjdk.org/jdk/pull/17170#discussion_r1447718468