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

[Alan Bateman]

On Wed, 10 Aug 2022 17:31:16 GMT, Brian Burkhalter <bpb at openjdk.org> wrote:

>> Add file cloning to `java.nio.channels.FileChannel::transferTo` and `java.nio.file.Files.copy(Path,Path)`.
>
> 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 add 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.

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.

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.

src/java.base/linux/native/libnio/fs/LinuxNativeDispatcher.c line 42:

> 40: 
> 41: #ifndef FICLONE
> 42: #define FICLONE      1074041865

Are we sure this value is the same on all architectures?

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.

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.

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

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.

src/java.base/unix/classes/sun/nio/fs/UnixCopyFile.java line 279:

> 277:         throws IOException
> 278:     {
> 279:         boolean copied = false;

The "copied" flag is a sign that the copyFile method needs to be refactor to split it into the copy of the contents and the file attributes. Also on first glance, re-opening both the source and destination files after a clone is a bug. We should (where possible) only open the source file for reading and the destination file for writing once.

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

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