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

[Brian Burkhalter]

On Wed, 3 Feb 2021 12:05:48 GMT, Sebastian Stenzel <github.com+1204330+overheadhunter at openjdk.org> wrote:

>> 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](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
>> 
>> 
>> 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.
>
> 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'

>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.

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

PR: https://git.openjdk.java.net/jdk/pull/2363