8321274: Rename ZipEntry.extraAttributes to ZipEntry.externalFileAttributes

[eirbjo]

Please consider this PR which suggests we rename ZipEntry.extraAttributes to ZipEntry.externalFileAttributes.

This field was introduced in JDK-8218021, originally under the name ZipEntry.posixPerms. JDK-8250968 later renamed the field to ZipEntry.extraAttributes and extended its semantics to hold the full two-byte value of the external file attributes field, as defined by APPNOTE.TXT

The name extraAttributes is misleading. It has nothing to do with the extra field (an unrelated structure defined in APPNOTE.TXT), although the name indicates it does.

To prevent confusion and make life easier for future maintainers, I suggest we rename this field to ZipEntry.externalFileAttributes and update related methods, parameters and comments accordingly.

While this change is a straightforward renaming, reviewers should consider whether it carries its weight, especially considering it might complicate future backports.

As a note to reviewers, this PR includes the following intended updates:

• Rename ZipEntry.extraAttributes and any references to this field to ZipEntry.externalFileAttributes
• Update JavaUtilZipFileAccess to similarly rename methods to setExternalFileAttributes and getExternalFileAttributes
• Rename the field JarSigner.extraAttrsDetected to JarSigner.externalFileAttrsDetected
• Rename a local variable in JarSigner.writeEntry to externalFileAttributes
• Rename s.s.t.jarsigner.Main.extraAttrsDetected to externalFileAttributesDetected
• Rename resource string key names in s.s.t.jarsigner.Resources from extra.attributes.detected to external.file.attributes.detected
• Rename method SymlinkTest.verifyExtraAttrs to verifyExternalFileAttributes, also updated two references to 'extra attributes' in this method
• Updated copyright in all affected files

If the resource file changes should be dropped and instead handled via msgdop updates, let me know and I can revert the non-default files.

I did a search across the code base to find 'extraAttrs', 'extra.attr' after these updates, and found nothing related to zip/jar. The zip and jar tests run clean:

make test TEST="test/jdk/java/util/jar"
make test TEST="test/jdk/java/util/zip"

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8321274: Rename ZipEntry.extraAttributes to ZipEntry.externalFileAttributes (Enhancement - P4)

Reviewers

• Lance Andersen (@LanceAndersen - Reviewer)
• Jaikiran Pai (@jaikiran - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/16952/head:pull/16952
$ git checkout pull/16952

Update a local copy of the PR:
$ git checkout pull/16952
$ git pull https://git.openjdk.org/jdk.git pull/16952/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 16952

View PR using the GUI difftool:
$ git pr show -t 16952

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/16952.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back eirbjo! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@eirbjo The following labels will be automatically applied to this pull request:

• core-libs
• security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

[eirbjo]

/issue 8321274

[openjdk]

@eirbjo This issue is referenced in the PR title - it will now be updated.

[mlbridge]

Webrevs

• 04: Full (292d6801)
• 03: Full - Incremental (243887b4)
• 02: Full - Incremental (10db9803)
• 01: Full - Incremental (481ae754)
• 00: Full (87db8646)

[LanceAndersen]

Thank you for the contribution Eirik. I will take a peek at this once we are able to push changes for JDK 23

[bridgekeeper]

@eirbjo This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[bridgekeeper]

@eirbjo This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

[jaikiran]

Hello Eirik, I think this is a reasonable change. I haven't had a chance to review some of these PRs due to some other priority tasks. I have these PRs on my TODO list. So if you want to pursue this further, please go ahead and reopen this and I'll review this in the coming days.

[eirbjo]

/open

[openjdk]

@eirbjo This pull request is now open

[LanceAndersen]

I think the change is OK, any reason to not make it externalFileAttributes?

[openjdk]

@eirbjo This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8321274: Rename ZipEntry.extraAttributes to ZipEntry.externalFileAttributes

Reviewed-by: lancea, jpai

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 18 new commits pushed to the master branch:

• a347957: 8335291: Problem list all SA core file tests on macosx-aarch64 due to JDK-8318754
• 153b12b: 8331560: Refactor Hotspot container detection code so that subsystem delegates to controllers
• 685e587: 8334816: compiler/c2/irTests/TestIfMinMax.java fails after 8334629
• dd74e7f: 8335147: Serial: Refactor TenuredGeneration::promote
• a537e87: 8335530: Java file extension missing in AuthenticatorTest
• 4060b35: 8335298: Fix -Wzero-as-null-pointer-constant warning in G1CardSetContainers
• 9046d7a: 8335390: C2 MergeStores: wrong result with Unsafe
• 318d9ac: 8335369: Fix -Wzero-as-null-pointer-constant warnings in ImmutableOopMapBuilder
• 5fe07b3: 5021949: JSplitPane setEnabled(false) shouldn't be partially functional
• ee4720a: 8333306: gc/arguments/TestParallelGCErgo.java fails when largepage are enabled
• ... and 8 more: https://git.openjdk.org/jdk/compare/bb18498d71dddf49db9bdfac886aed9ae123651d...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[eirbjo]

I think the change is OK, any reason to not make it externalFileAttributes?

None other than length / verbosity. But since this refers to the attributes of the external file, dropping any name element also loses some semantics, introducing a potential for confusion. Perhaps verbosity is the sensible choice here.

If you agree to the above, I can update the PR to rename to the following names instead:

• ZipFile.externalFileAttributes
• JavaUtilZipFileAccess.java.[set|get]ExternalFileAttributes
• JarSigner.externalFileAttributesDetected
• jarsigner/Main.externalFileAttributesDetected
• external.file.attributes.detected in Resources.java

WDYT?

[LanceAndersen]

I think the change is OK, any reason to not make it externalFileAttributes?

None other than length / verbosity. But since this refers to the attributes of the external file, dropping any name element also loses some semantics, introducing a potential for confusion. Perhaps verbosity is the sensible choice here.

If you agree to the above, I can update the PR to rename to the following names instead:

• ZipFile.externalFileAttributes
• JavaUtilZipFileAccess.java.[set|get]ExternalFileAttributes
• JarSigner.externalFileAttributesDetected
• jarsigner/Main.externalFileAttributesDetected
• external.file.attributes.detected in Resources.java

WDYT?

I think the proposed change above makes things clearer. Perhaps also make the same change in zipfs as well while you are at it?

[eirbjo]

I think the proposed change above makes things clearer. Perhaps also make the same change in zipfs as well while you are at it?

I have pushed the rename to "ZipEntry.externalFileAttributes". Also renamed ZipFileSystem.Entry.posixPerms to ZipFileSystem.Entry.externalFileAttributes. Hopefully this makes things clearer.

A side note: The Posix support in ZipFileSystem is somewhat odd in that it supports the notion of a null permission set. So setting the permissions attribute to null clears all the attributes, not just the permisson ones. But even so, I think using the full name here is also an improvement, since it signals that the field may also carry preexisting file type, setuid, setgid, sticky bits.

[eirbjo]

/label add nio

[eirbjo]

This PR was approved by Lance in March, but it seems I forgot to integrate.

I plan to integrate this in the next few days unless I hear objections.

[jaikiran]

Hello Eirik, it will be better to merge with the latest master branch and run the tier tests before integrating.

[liach]

I believe this field only captures the 2 bytes at higher indices of External File Attribute; it's not the complete 4-byte external file attributes and this value is not captured unless "Version made by" field's larger index byte (byte 5) is 3. So this name may still be imperfect.

[eirbjo]

I believe this field only captures the 2 bytes at higher indices of External File Attribute; it's not the complete 4-byte external file attributes and this value is not captured unless "Version made by" field's larger index byte (byte 5) is 3. So this name may still be imperfect.

The purpose of this PR was mainly to reduce the risk of confusion with the "extra field", which is an entirely unrelated concept.

While I agree that the name externalFileAttributes might not express the full semantics of the field perfectly, I'm honestly not sure which name would, given that APPNOTE.TXT is pretty opaque in describing what these two bytes express, and as you point out, the field only carries data when "Version Made By" indicates Unix.

Instead of trying to find a name which covers the full semantics, I suggest that we keep externalFileAttributes and instead seek to improve the comments relevant to this field:

• The field comment currently says // File type, setuid, setgid, sticky, POSIX permissions, we could prepend something clarifying that the field only carries data for Unix entries.
• Line 699 says // read all bits in this field, including sym link attributes, this could be improved to clarify that only the higher two "unix-bytes" are read ("all bits in this field" is a bit unclear now)
• Line 700 masks off the Unix bytes with 0xFFFF, this isn't strictly necessary since CENATX_PERMS already only reads the two bytes we want. Perhaps clearer just to drop this masking.
• ZipUtils.CENATX_PERMS reads the full 16 bits of the Unix external file attributes, not just the permissions. The current name is confusing.

Since I don't want to delay the integration of the this PR any further, I suggest we tackle the above clarifications in a follow-up PR. Would that be ok with you, @liach ?

[eirbjo]

Hello Eirik, it will be better to merge with the latest master branch and run the tier tests before integrating.

Life happened, but I have now merged with the latest master and GHA runs green.

Did you want to run additional testing before integration @jaikiran ?

[jaikiran]

Hello Eirik, I'll run some tests and review this PR this coming week.

[jaikiran]

Hello Eirik, the latest changes in this PR (292d6801) look good to me. However, these changes cause some (expected) compilation failures in some of the internal classes within some Oracle specific JDK classes. Those tests aren't accessible for external contributors. I will be updating that code to address those failures. That would mean that the integration of this PR will have to be co-ordinated.

Can you issue a /integrate delegate command on this PR so that it then allows me to do the actual integration along with the Oracle side of changes?

[eirbjo]

/integrate delegate

[openjdk]

@eirbjo Integration of this pull request has been delegated and may be completed by any project committer using the /integrate pull request command.

[eirbjo]

Can you issue a /integrate delegate command on this PR so that it then allows me to do the actual integration along with the Oracle side of changes?

Done. And big thanks for your help getting this long-lasting change across the finish line!

[jaikiran]

/integrate

[openjdk]

Going to push as commit d51141e.
Since your change was applied there have been 26 commits pushed to the master branch:

• fac74b1: 8334229: Optimize InterpreterOopMap layout
• f9b4ea1: 8334220: Optimize Klass layout after JDK-8180450
• f7af450: 8335110: Fix instruction name and API spec inconsistencies in CodeBuilder
• 8a664a4: 8334734: Remove specialized readXxxEntry methods from ClassReader
• 3a2d426: 8334726: Remove accidentally exposed individual methods from Class-File API
• f187c92: 8335370: Fix -Wzero-as-null-pointer-constant warning in jvmti_common.hpp
• 1ef34c1: 8335475: ClassBuilder incorrectly calculates max_locals in some cases
• 27982c8: 8327854: Test java/util/stream/test/org/openjdk/tests/java/util/stream/WhileOpStatefulTest.java failed with RuntimeException
• a347957: 8335291: Problem list all SA core file tests on macosx-aarch64 due to JDK-8318754
• 153b12b: 8331560: Refactor Hotspot container detection code so that subsystem delegates to controllers
• ... and 16 more: https://git.openjdk.org/jdk/compare/bb18498d71dddf49db9bdfac886aed9ae123651d...master

Your commit was automatically rebased without conflicts.

[openjdk]

@jaikiran Pushed as commit d51141e.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.