Skip to content

Commit

Permalink
Fix some build issues from previous commit for dotnet#77835 again (3)
Browse files Browse the repository at this point in the history 
• Fix indentation (should have used spaces)
• Import Microsoft.Win32.SafeHandles in FileSystem.CopyFile.OSX.cs file
• Fix for SA1205 'Partial elements should declare an access modifier'
• Fix typo in FileSystem.CopyFile.OtherUnix.cs of 'startedCopyFile'
• Fix missing openDst parameter code in StartCopyFile
• Fix not checking the nullability in StandardCopyFile, which led to 'Possible null reference argument for parameter'
  • Loading branch information
@hamarb123
hamarb123 committed Dec 5, 2022
1 parent e490d5e commit 1e43a91
Show file tree
Hide file tree
Showing 3 changed files with 139 additions and 134 deletions.
241 changes: 121 additions & 120 deletions src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OSX.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,143 +2,144 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System.Runtime.InteropServices;
using Microsoft.Win32.SafeHandles;

namespace System.IO
{
partial class FileSystem
{
public static partial void CopyFile(string sourceFullPath, string destFullPath, bool overwrite)
{
//Attempt to clone the file:
internal static partial class FileSystem
{
public static partial void CopyFile(string sourceFullPath, string destFullPath, bool overwrite)
{
//Attempt to clone the file:

//Get the full path of the source path
string fullSource = TryGetLinkTarget(sourceFullPath) ?? sourceFullPath;
//Get the full path of the source path
string fullSource = TryGetLinkTarget(sourceFullPath) ?? sourceFullPath;

//Start the file copy and prepare for finalization
StartedCopyFileState startedCopyFile = StartCopyFile(fullSource, destFullPath, overwrite, openDst: false);
//Start the file copy and prepare for finalization
StartedCopyFileState startedCopyFile = StartCopyFile(fullSource, destFullPath, overwrite, openDst: false);

//Attempt counter just in case we somehow loop infinite times e.g. on a
//filesystem that doesn't actually delete files but pretends it does.
//Declare error variable here since it can be used after some jumping around.
int attempts = 0;
int error;
//Attempt counter just in case we somehow loop infinite times e.g. on a
//filesystem that doesn't actually delete files but pretends it does.
//Declare error variable here since it can be used after some jumping around.
int attempts = 0;
int error;

try
{
//Don't need to re-read the link on our first attempt
bool failOnRereadDoesntChange = false;
if (overwrite)
{
//Ensure file is deleted on first try.
//Get a lock to the dest file for compat reasons, and then delete it.
using SafeFileHandle? dstHandle = OpenCopyFileDstHandle(destFullPath, true, startedCopyFile, false);
File.Delete(destFullPath);
}
goto tryAgain;
try
{
//Don't need to re-read the link on our first attempt
bool failOnRereadDoesntChange = false;
if (overwrite)
{
//Ensure file is deleted on first try.
//Get a lock to the dest file for compat reasons, and then delete it.
using SafeFileHandle? dstHandle = OpenCopyFileDstHandle(destFullPath, true, startedCopyFile, false);
File.Delete(destFullPath);
}
goto tryAgain;

//We may want to re-read the link to see if its path has changed
tryAgainWithReadLink:
if (++attempts >= 5) goto throwError;
string fullSource2 = TryGetLinkTarget(sourceFullPath) ?? sourceFullPath;
if (fullSource != fullSource2)
{
//Path has changed
startedCopyFile.Dispose();
startedCopyFile = StartCopyFile(fullSource, destFullPath, overwrite, openDst: false);
}
else if (failOnRereadDoesntChange)
{
//Path hasn't changed and we want to throw the error we got earlier
goto throwError;
}
failOnRereadDoesntChange = false;
//We may want to re-read the link to see if its path has changed
tryAgainWithReadLink:
if (++attempts >= 5) goto throwError;
string fullSource2 = TryGetLinkTarget(sourceFullPath) ?? sourceFullPath;
if (fullSource != fullSource2)
{
//Path has changed
startedCopyFile.Dispose();
startedCopyFile = StartCopyFile(fullSource, destFullPath, overwrite, openDst: false);
}
else if (failOnRereadDoesntChange)
{
//Path hasn't changed and we want to throw the error we got earlier
goto throwError;
}
failOnRereadDoesntChange = false;

//Attempt to clone the file
tryAgain:
unsafe
{
if (Interop.@libc.copyfile(fullSource, destFullPath, null, Interop.@libc.COPYFILE_CLONE_FORCE) == 0)
{
return;
}
}
//Attempt to clone the file
tryAgain:
unsafe
{
if (Interop.@libc.copyfile(fullSource, destFullPath, null, Interop.@libc.COPYFILE_CLONE_FORCE) == 0)
{
return;
}
}

//Check the error
error = Marshal.GetLastWin32Error();
const int ENOTSUP = 45;
const int EEXIST = 17;
const int ENOENT = 2;
bool directoryExist = false;
if ((error == ENOTSUP && FileOrDirectoryExists(destFullPath)) || error == EEXIST)
{
//This means the destination existed, try again with the destination deleted if appropriate
error = EEXIST;
if (Directory.Exists(destFullPath))
{
directoryExist = true;
goto throwError;
}
if (overwrite)
{
//Get a lock to the dest file for compat reasons, and then delete it.
using SafeFileHandle dstHandle = OpenCopyFileDstHandle(destFullPath, true, startedCopyFile);
File.Delete(destFullPath);
goto tryAgainWithReadLink;
}
}
else if (error == ENOTSUP)
{
//This probably means cloning is not supported, try the standard implementation
goto fallback;
}
else if (error == ENOENT)
{
//This can happen if the source is a symlink and it has been changed to a different file, and the first has been deleted or renamed, for example.
//failOnRereadDoesntChange means we want to fail if the link didn't change, indicating the source actually doesn't exist.
failOnRereadDoesntChange = true;
goto tryAgainWithReadLink;
}
//Check the error
error = Marshal.GetLastWin32Error();
const int ENOTSUP = 45;
const int EEXIST = 17;
const int ENOENT = 2;
bool directoryExist = false;
if ((error == ENOTSUP && FileOrDirectoryExists(destFullPath)) || error == EEXIST)
{
//This means the destination existed, try again with the destination deleted if appropriate
error = EEXIST;
if (Directory.Exists(destFullPath))
{
directoryExist = true;
goto throwError;
}
if (overwrite)
{
//Get a lock to the dest file for compat reasons, and then delete it.
using SafeFileHandle dstHandle = OpenCopyFileDstHandle(destFullPath, true, startedCopyFile);
File.Delete(destFullPath);
goto tryAgainWithReadLink;
}
}
else if (error == ENOTSUP)
{
//This probably means cloning is not supported, try the standard implementation
goto fallback;
}
else if (error == ENOENT)
{
//This can happen if the source is a symlink and it has been changed to a different file, and the first has been deleted or renamed, for example.
//failOnRereadDoesntChange means we want to fail if the link didn't change, indicating the source actually doesn't exist.
failOnRereadDoesntChange = true;
goto tryAgainWithReadLink;
}

//Throw an appropriate error
throwError:
if (directoryExist)
//Throw an appropriate error
throwError:
if (directoryExist)
{
throw new IOException(SR.Format(SR.Arg_FileIsDirectory_Name, path));
}
throw Interop.GetExceptionForIoErrno(new ErrorInfo(error));
throw Interop.GetExceptionForIoErrno(new ErrorInfo(error));

//Fallback to the standard unix implementation for when cloning is not supported
fallback:
//Fallback to the standard unix implementation for when cloning is not supported
fallback:

//Open the dst handle
startedCopyFile.dst = OpenCopyFileDstHandle(destFullPath, overwrite, startedCopyFile, true);
//Open the dst handle
startedCopyFile.dst = OpenCopyFileDstHandle(destFullPath, overwrite, startedCopyFile, true);

//Copy the file using the standard unix implementation
StandardCopyFile(startedCopyFile);
}
finally
{
startedCopyFile.Dispose();
}
//Copy the file using the standard unix implementation
StandardCopyFile(startedCopyFile);
}
finally
{
startedCopyFile.Dispose();
}

//Attempts to read the path's link target, or returns null even if the path doesn't exist
static string? TryGetLinkTarget(string path)
{
try
{
return ResolveLinkTargetString(sourceFullPath, true, false);
}
catch
{
return null;
}
}
//Attempts to read the path's link target, or returns null even if the path doesn't exist
static string? TryGetLinkTarget(string path)
{
try
{
return ResolveLinkTargetString(sourceFullPath, true, false);
}
catch
{
return null;
}
}

//Checks if a file or directory exists without caring which it was
static bool FileOrDirectoryExists(string path)
{
return Interop.Sys.Stat(fullPath, out _) >= 0;
}
}
}
//Checks if a file or directory exists without caring which it was
static bool FileOrDirectoryExists(string path)
{
return Interop.Sys.Stat(fullPath, out _) >= 0;
}
}
}
}
20 changes: 10 additions & 10 deletions src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.CopyFile.OtherUnix.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,15 @@

namespace System.IO
{
partial class FileSystem
{
public static partial void CopyFile(string sourceFullPath, string destFullPath, bool overwrite)
{
//Start the file copy and prepare for finalization
using StartedCopyFileState startedCopyFile = StartCopyFile(sourceFullPath, destFullPath, overwrite);
internal static partial class FileSystem
{
public static partial void CopyFile(string sourceFullPath, string destFullPath, bool overwrite)
{
//Start the file copy and prepare for finalization
using StartedCopyFileState startedCopyFile = StartCopyFile(sourceFullPath, destFullPath, overwrite);

//Copy the file using the standard unix implementation
StandardCopyFile(startCopyFile);
}
}
//Copy the file using the standard unix implementation
StandardCopyFile(startedCopyFile);
}
}
}
12 changes: 8 additions & 4 deletions src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,16 +52,17 @@ public void Dispose()
}
}

private static StartedCopyFileState StartCopyFile(string sourceFullPath, string destFullPath, bool overwrite)
private static StartedCopyFileState StartCopyFile(string sourceFullPath, string destFullPath, bool overwrite, bool openDst = true)
{
//The return value is expected to be Disposed by the caller (unless this method throws) once the copy is complete.
//Begins 'CopyFile' by locking and creating the relevant file handles.
//If 'openDst' is false, it doesn't open the destination file handle, nor check anything to do with it (used in macOS implementation).

StartedCopyFileState startedCopyFile = default;
try
{
startedCopyFile.src = SafeFileHandle.OpenReadOnly(sourceFullPath, FileOptions.None, out startedCopyFile.fileLength, out startedCopyFile.filePermissions);
startedCopyFile.dst = OpenCopyFileDstHandle(destFullPath, overwrite, startedCopyFile, true);
if (openDst) startedCopyFile.dst = OpenCopyFileDstHandle(destFullPath, overwrite, startedCopyFile, true);
}
catch
{
Expand Down Expand Up @@ -111,8 +112,11 @@ private static void StandardCopyFile(StartedCopyFileState startedCopyFile)
{
//Copy the file in a way that works on all Unix Operating Systems.
//The 'startedCopyFile' parameter should take the output from 'StartCopyFile'.
//'startedCopyFile' should be disposed by the caller.
Interop.CheckIo(Interop.Sys.CopyFile(startedCopyFile.src, startedCopyFile.dst, startedCopyFile.fileLength));
//'startedCopyFile' should be disposed by the caller. Assumes src and dst
//are non-null, caller must check this, return values from StartCopyFile
//are non-null (except dst when openDst is true), and from return values from
//OpenCopyFileDstHandle are non-null (except possibly when openNewFile is false).
Interop.CheckIo(Interop.Sys.CopyFile(startedCopyFile.src!, startedCopyFile.dst!, startedCopyFile.fileLength));
}

//CopyFile is defined in either FileSystem.CopyFile.OSX.cs or FileSystem.CopyFile.OtherUnix.cs
Expand Down

0 comments on commit 1e43a91

Please sign in to comment.