On Tue, 9 Jan 2024 10:22:40 GMT, Eirik Bjørsnøs <eir...@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
