JDK-8260966 (fs) Consolidate Linux and macOS implementations of UserDefinedFileAttributeView

[overheadhunter]

I want to discuss the changes of this PR in three sections:

Deduplications

The main focus was to deduplicate code:

• LinuxUserDefinedAttributeView + BsdUserDefinedAttributeView → UnixUserDefinedAttributeView. Due to different supported length of attribute names, I made added an abstract method UnixUserDefinedAttributeView. maxNameLength() and kept the other two.
• LinuxNativeDispatcher + BsdNativeDispatcher → UnixNativeDispatcher. I basically just moved the Linux implementation up to Unix. BSD-specific adjustments are now done in C.
• LinuxFileStore.isExtendedAttributesEnabled() + BsdFileStore.isExtendedAttributesEnabled() → UnixFileStore.isExtendedAttributesEnabled()
• For the latter I hade to combine UnixConstants.ENODATA and UnixConstants.ENOATTR to a platform-agnostic UnixConstants.XATTR_NOT_FOUND ⚠️ Warning: While ENODATA is no longer used JDK-internally, it might have been used via reflection in downstream projects, thus causing regressions. Maintainers should decide, if we might want to keep ENODATA additionally to the new XATTR_NOT_FOUND.
• Deduplicated code inside of *NativeDispatcher's methods copyExtendedAttributes(int ofd, int nfd) and list()

Cleanup / Improvements

• On AIX (or other Unix Platforms not explicitly supporting xattr), calling related native methods will result in ENOTSUP
• NativeBuffer implements AutoCloseable and used try-with-resource statements where applicable
• changed loop to recursive function for creation of larger buffer, if flistxattr fails due to ERANGE error (max recursion depth is 5, I don't expect performance benefits from loop unrolling, therefore chose reduced cyclomatic complexity over performance)
• Added UnixNativeDispatcher.SUPPORTS_XATTR capability and added a shortcut to UnixFileStore. isExtendedAttributesEnabled(...) to skip I/O that is known to fail
• Removed legacy dlsym for xattr-related Linux system calls (fixes JDK-8260691)

Fixes

• Files.copy(src, dst, StandardCopyOption.COPY_ATTRIBUTES) now correctly copies xattr on macOS/BSD (invocation was missing before)
• no longer access dst.arrayOffset() without checking if dst.hasArray() during read(). Code is now symmetric with write()
• fixed potential resource leak of nb during write(), if file.openForAttributeAccess(...) fails

Please note, that while the tests succeed on macOS, so far I didn't manage to run jtreg:test/jdk/java/nio/file/attribute/ on a Linux CI system due to lack of knowledge or will power 🙈 Edit: Tests succeed on Linux, too.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Issues

• JDK-8260966: (fs) Consolidate Linux and macOS implementations of UserDefinedFileAttributeView
• JDK-8260691: (fs) LinuxNativeDispatcher should link to xattr functions

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2363/head:pull/2363
$ git checkout pull/2363

[bridgekeeper]

👋 Welcome back overheadhunter! 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]

@overheadhunter The following label will be automatically applied to this pull request:

• nio

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

[mlbridge]

Webrevs

• 00: Full (9479059)

[overheadhunter]

Ok I managed to get jtreg running in a Linux container now. Looks good:

==============================
Test summary
==============================
   TEST                                              TOTAL  PASS  FAIL ERROR   
   jtreg:test/jdk/java/nio/file/attribute               10    10     0     0   
==============================
TEST SUCCESS

Finished building target 'run-test' in configuration 'linux-x86_64-server-release'

[overheadhunter]

/issue add JDK-8260691

[openjdk]

@overheadhunter
Adding additional issue to issue list: 8260691: (fs) LinuxNativeDispatcher should link to xattr functions.

[mlbridge]

Mailing list message from Alan Bateman on nio-dev:

On 02/02/2021 21:27, Sebastian Stenzel wrote:

I want to discuss the changes of this PR in three sections:

Okay, one general remark here the overall patch is harder to review now
because there are several things going on. I had hoped that the
"modernization" of the *UserDefinedFileAttributeView would be separated
out as there are several discussion points there. Also moving/combining
files and significantly changing them at the same time means the diffs
are harder to read. There is also some re-formatting that introduces
some noise too. I'm not opposed to doing all the steps in one, it will
just take long to review (and careful review is required due to the
potential for buffer overflow in some of this code).

# Deduplications
The main focus was to deduplicate code:
- `LinuxUserDefinedAttributeView` + `BsdUserDefinedAttributeView` ? `UnixUserDefinedAttributeView`. Due to different supported length of attribute names, I made added an abstract method `UnixUserDefinedAttributeView. maxNameLength()` and kept the other two.
- `LinuxNativeDispatcher` + `BsdNativeDispatcher` ? `UnixNativeDispatcher`. I basically just moved the Linux implementation up to Unix. BSD-specific adjustments are now done in C.
- `LinuxFileStore.isExtendedAttributesEnabled()` + `BsdFileStore.isExtendedAttributesEnabled()` ? `UnixFileStore.isExtendedAttributesEnabled()`
- For the latter I hade to combine `UnixConstants.ENODATA` and `UnixConstants.ENOATTR` to a platform-agnostic `UnixConstants.XATTR_NOT_FOUND` ?? Warning: While `ENODATA` is no longer used JDK-internally, it might have been used via reflection in downstream projects, thus causing regressions. Maintainers should decide, if we might want to keep `ENODATA` additionally to the new `XATTR_NOT_FOUND`

I would prefer if we left ENODATA so that it can be used in Linux
specific code.

- Deduplicated code inside of *NativeDispatcher's methods `copyExtendedAttributes(int ofd, int nfd)` and `list()`

# Cleanup / Improvements
- On AIX (or other Unix Platforms not explicitly supporting xattr), calling related native methods will result in `ENOTSUP`
- `NativeBuffer implements AutoCloseable` and used try-with-resource statements where applicable
- changed loop to recursive function for creation of larger buffer, if `flistxattr` fails due to `ERANGE` error (max recursion depth is 5, I don't expect performance benefits from loop unrolling, therefore chose reduced cyclomatic complexity over performance)
- Added `UnixNativeDispatcher.SUPPORTS_XATTR` capability and added a shortcut to `UnixFileStore. isExtendedAttributesEnabled(...)` to skip I/O that is known to fail
- Removed legacy `dlsym` for xattr-related Linux system calls (fixes JDK-8260691)

# Fixes
- `Files.copy(src, dst, StandardCopyOption.COPY_ATTRIBUTES)` now correctly copies xattr on macOS/BSD ([invocation was missing before](https://github.com/openjdk/jdk/blob/jdk-17%2B7/src/java.base/macosx/classes/sun/nio/fs/BsdFileSystem.java#L70-L72))
- no longer [access `dst.arrayOffset()` without checking](https://github.com/openjdk/jdk/blob/jdk-17+7/src/java.base/linux/classes/sun/nio/fs/LinuxUserDefinedFileAttributeView.java#L201) if `dst.hasArray()` during `read()`. Code is now symmetric with `write()`
- fixed [potential resource leak of `nb` during `write()`](https://github.com/openjdk/jdk/blob/jdk-17+7/src/java.base/linux/classes/sun/nio/fs/LinuxUserDefinedFileAttributeView.java#L258), if `file.openForAttributeAccess(...)` fails

At a high-level then I think most of these suggestion changes are okay
but I expect it will take several iterations to get agreement on all
changes.

-Alan.

[mlbridge]

Mailing list message from Sebastian Stenzel on nio-dev:

Am 03.02.2021 um 15:03 schrieb Alan Bateman <Alan.Bateman at oracle.com>:

Also moving/combining files and significantly changing them at the same time means the diffs are harder to read.

Just to save you some time: the PR has three separate commits, the first of them purely focused on moving stuff between files, actual code changes are in the two later commits.

I forgot to mention that.

[mlbridge]

Mailing list message from Alan Bateman on nio-dev:

On 03/02/2021 15:49, Sebastian Stenzel wrote:

Just to save you some time: the PR has three separate commits, the first of them purely focused on moving stuff between files, actual code changes are in the two later commits.

I forgot to mention that.

Ack! I will try to look at it more closely in the next few days.

-Alan

[bplb]

From what I can tell these changes look all right, but I think it would be easier to understand them if they were broken up into separate PRs. One problem here is that apparently this PR was created from a branch which already had the three commits on it. This causes there to be only one webrev encompassing all the changes in the three commits at once. Had the PR been created from a branch containing only the first commit and the other commits added one by one, then at least there would be an incremental webrev showing the changes made from one commit to the next.

[mlbridge]

Mailing list message from Sebastian Stenzel on nio-dev:

Ok, then I would propose to close and recreate the PR.

I see two options: Push the first commit, wait for a review, push further commits.

Or: Have only the first commit in this new PR, create a second PR. This requires you to create another ticket for cleanup tasks.

[overheadhunter]

Closing this in favour of #2604