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

[Alan Bateman]

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.