RFR: 8264744: (fs) Use file cloning in Linux and macOS versions of FileChannel transfer and Files copy methods [v7]

[Brian Burkhalter]

On Fri, 12 Aug 2022 08:42:55 GMT, Alan Bateman <alanb at openjdk.org> wrote:

>> Brian Burkhalter has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 10 commits:
>> 
>>  - 8264744: Refactor {Bsd,Linux}CopyFile::clone into {Bsd,Linux}FileSystem::clone
>>  - Merge
>>  - 8264744: Remove CloneFile classes; refactor to use {Bsd,Linux}CopyFile
>>  - Merge
>>  - 8264744: Fix typo desintation -> destination
>>  - 8264744: Refactor into UnixCopyFile hierarchy
>>  - Merge
>>  - 8264744: Address review comments aside from refactoring
>>  - 8264744: Refactor into provider+dispatcher to reduce amount of JNI code
>>  - 8264744: (fc, fs) Support file cloning in Unix versions of FileChannel transfer and Files copy methods
>
> src/java.base/linux/classes/sun/nio/fs/LinuxFileSystem.java line 74:
> 
>> 72:     }
>> 73: 
>> 74:     private static UnixException catEx(UnixException x, UnixException y) {
> 
> What is the purpose of adding a suppressed exception to a UnixException? A UnixException will get re-mapped to a File SystemException so I assume the suppressed exception will be ignored.

It was a mistake; fixed in 2157a1480229fa697c824ae814f11a309627d768.

> src/java.base/linux/classes/sun/nio/fs/LinuxFileSystem.java line 100:
> 
>> 98:      */
>> 99:     protected int clone(UnixPath src, UnixPath dst, boolean followLinks)
>> 100:         throws IOException {
> 
> Formatting nit here too.

Fixed in 2157a1480229fa697c824ae814f11a309627d768.

> src/java.base/linux/classes/sun/nio/fs/LinuxFileSystem.java line 111:
> 
>> 109:         int dstFD = 0;
>> 110:         try {
>> 111:             dstFD = open(dst, O_CREAT | O_WRONLY, 0666);
> 
> The mode should be attrs.mode() rather than 0666, which means the clone method need an additional parameter.

Fixed in 2157a1480229fa697c824ae814f11a309627d768.

> src/java.base/macosx/classes/sun/nio/fs/BsdFileSystem.java line 88:
> 
>> 86:     protected int clone(UnixPath src, UnixPath dst, boolean followLinks)
>> 87:         throws IOException {
>> 88:         int flags = followLinks ? 0 : CLONE_NOFOLLOW;
> 
> Can you put the "{" onto its own line so that it's a bit easier to distinguish the method declaration from the body.

Fixed in 2157a1480229fa697c824ae814f11a309627d768.

> src/java.base/macosx/classes/sun/nio/fs/BsdNativeDispatcher.java line 72:
> 
>> 70:     static int clonefile(UnixPath src, UnixPath dst, int flags)
>> 71:         throws UnixException {
>> 72:         if (src.getFileSystem() == dst.getFileSystem()) {
> 
> Why does this check if getFileSystem() returns the same file system? There is only one instance of UnixFileSystem so this will always be true.
> 
> Will the "throws UnixException {" fit on the previous line to fixing the odd formatting.

Fixed in 2157a1480229fa697c824ae814f11a309627d768.

> src/java.base/macosx/native/libnio/fs/BsdNativeDispatcher.c line 232:
> 
>> 230: JNIEXPORT jint JNICALL
>> 231: Java_sun_nio_fs_BsdNativeDispatcher_clonefile0(JNIEnv* env, jclass this,
>> 232:     jlong srcAddress, jlong dstAddress, int flags)
> 
> flags should be a "jint" (not int).

Fixed in 2157a1480229fa697c824ae814f11a309627d768.

> src/java.base/macosx/native/libnio/fs/BsdNativeDispatcher.c line 283:
> 
>> 281: 
>> 282:     if (setattrlist(path, &attrList, (void*)buf,
>> 283:         count*attrsize, options) != 0) {
> 
> I assume this is an accidental edit.

Right; fixed in 2157a1480229fa697c824ae814f11a309627d768.

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

PR: https://git.openjdk.org/jdk/pull/9486