Skip to content

Cleanup NativeItemExists() #6929

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
daxian-dbw merged 20 commits into from
Jun 14, 2018
Merged

Cleanup NativeItemExists() #6929

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
d364c67
Remove unneeded p/invokes
iSazonov May 23, 2018
01d2685
Create NativeItemExists based on File.GetAttributes(path)
iSazonov May 24, 2018
1637ad1
Update NativeItemExists w/o throw
iSazonov May 24, 2018
058ac67
Replace NativeItemExists in GetPathItems
iSazonov May 23, 2018
286d714
Replace NativeItemExists in RemoveDirectoryInfoItem
iSazonov May 23, 2018
4e941d6
Replace NativeItemExists in ItemExists
iSazonov May 23, 2018
b515681
Replace NativeItemExists in GetProperty
iSazonov May 23, 2018
5d85a4d
Replace NativeItemExists in SetProperty
iSazonov May 23, 2018
23e44ac
Replace CheckItemExists with NativeItemExists
iSazonov May 24, 2018
c6573fb
Update NativeFileExists and NativeDirectoryExists
iSazonov May 24, 2018
fdb1667
Remove old NativeItemExists
iSazonov May 24, 2018
cf419bf
[Feature] Remove 'Native' from method names
iSazonov May 24, 2018
67c5d33
Fix return in ItemExists
iSazonov May 30, 2018
c37761a
Fix module loading
iSazonov Jun 5, 2018
0f25099
Recover ItemExists() behavior for File not found exception
iSazonov Jun 5, 2018
757d545
Use "discard" variable
iSazonov Jun 7, 2018
db25895
Use File.Exists in GetFileSecurityZone
iSazonov Jun 7, 2018
2c0fa8f
Remove unneeded #if !UNIX
iSazonov Jun 9, 2018
9098964
Revert "Use File.Exists in GetFileSecurityZone"
iSazonov Jun 9, 2018
3963c46
Remove unneeded comment
iSazonov Jun 9, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Microsoft.PowerShell.Security/security/Utils.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ internal static string GetFilePathOfExistingFile(PSCmdlet cmdlet,
string path)
{
string resolvedProviderPath = cmdlet.SessionState.Path.GetUnresolvedProviderPathFromPSPath(path);
if (Utils.NativeFileExists(resolvedProviderPath))
if (Utils.FileExists(resolvedProviderPath))
{
return resolvedProviderPath;
}
Expand Down
18 changes: 0 additions & 18 deletions src/System.Management.Automation/CoreCLR/CorePsPlatform.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -551,16 +551,6 @@ internal static string NonWindowsGetHostName()
return Unix.NativeMethods.GetFullyQualifiedName() ?? string.Empty;
}

internal static bool NonWindowsIsFile(string path)
{
return Unix.NativeMethods.IsFile(path);
}

internal static bool NonWindowsIsDirectory(string path)
{
return Unix.NativeMethods.IsDirectory(path);
}

internal static bool NonWindowsIsSameFileSystemItem(string pathOne, string pathTwo)
{
return Unix.NativeMethods.IsSameFileSystemItem(pathOne, pathTwo);
Expand Down Expand Up @@ -773,14 +763,6 @@ internal static extern int CreateHardLink([MarshalAs(UnmanagedType.LPStr)]string
[return: MarshalAs(UnmanagedType.LPStr)]
internal static extern string GetUserFromPid(int pid);

[DllImport(psLib, CharSet = CharSet.Ansi, SetLastError = true)]
[return: MarshalAs(UnmanagedType.I1)]
internal static extern bool IsFile([MarshalAs(UnmanagedType.LPStr)]string filePath);

[DllImport(psLib, CharSet = CharSet.Ansi, SetLastError = true)]
[return: MarshalAs(UnmanagedType.I1)]
internal static extern bool IsDirectory([MarshalAs(UnmanagedType.LPStr)]string filePath);

[DllImport(psLib, CharSet = CharSet.Ansi, SetLastError = true)]
[return: MarshalAs(UnmanagedType.I1)]
internal static extern bool IsSameFileSystemItem([MarshalAs(UnmanagedType.LPStr)]string filePathOne,
Expand Down
2 changes: 1 addition & 1 deletion src/System.Management.Automation/engine/CommandSearcher.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -578,7 +578,7 @@ private CommandInfo GetInfoFromPath(string path)

do // false loop
{
if (!Utils.NativeFileExists(path))
if (!Utils.FileExists(path))
{
CommandDiscovery.discoveryTracer.TraceError("The path does not exist: {0}", path);
break;
Expand Down
6 changes: 3 additions & 3 deletions src/System.Management.Automation/engine/Modules/AnalysisCache.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -650,7 +650,7 @@ private void Cleanup()
var keys = Entries.Keys;
foreach (var key in keys)
{
if (!Utils.NativeFileExists(key))
if (!Utils.FileExists(key))
{
ModuleCacheEntry unused;
removedSomething |= Entries.TryRemove(key, out unused);
Expand Down Expand Up @@ -690,7 +690,7 @@ private void Serialize(string filename)

try
{
if (Utils.NativeFileExists(filename))
if (Utils.FileExists(filename))
{
var fileLastWriteTime = new FileInfo(filename).LastWriteTime;
if (fileLastWriteTime > this.LastReadTime)
Expand Down Expand Up @@ -987,7 +987,7 @@ internal static AnalysisCacheData Get()
{
try
{
if (Utils.NativeFileExists(s_cacheStoreLocation))
if (Utils.FileExists(s_cacheStoreLocation))
{
return Deserialize(s_cacheStoreLocation);
}
Expand Down
16 changes: 8 additions & 8 deletions src/System.Management.Automation/engine/Modules/ModuleCmdletBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,7 @@ internal bool LoadUsingModulePath(PSModuleInfo parentModule, bool found, IEnumer
{
qualifiedPath = Path.Combine(qualifiedPath, fileBaseName);
}
else if (Utils.NativeDirectoryExists(qualifiedPath))
else if (Utils.DirectoryExists(qualifiedPath))
{
// if it points to a directory, add the basename back onto the path...
qualifiedPath = Path.Combine(qualifiedPath, Path.GetFileName(fileBaseName));
Expand Down Expand Up @@ -924,7 +924,7 @@ private IEnumerable<PSModuleInfo> GetModuleForRootedPaths(string[] modulePaths,
foreach (string resolvedModulePath in modulePathCollection)
{
string moduleName = Path.GetFileName(resolvedModulePath);
bool isDirectory = Utils.NativeDirectoryExists(resolvedModulePath);
bool isDirectory = Utils.DirectoryExists(resolvedModulePath);

// If the given path is a valid module file, we will load the specific file
if (!isDirectory && ModuleIntrinsics.IsPowerShellModuleExtension(Path.GetExtension(moduleName)))
Expand Down Expand Up @@ -1624,7 +1624,7 @@ internal PSModuleInfo LoadModuleManifest(
return loadedModule;
}
// remove the module if force is specified (and if module is already loaded)
else if (Utils.NativeFileExists(rootedPath))
else if (Utils.FileExists(rootedPath))
{
RemoveModule(loadedModule);
}
Expand Down Expand Up @@ -4111,7 +4111,7 @@ private ExternalScriptInfo FindLocalizedModuleManifest(string path)

String filePath = stringBuilder.ToString();

if (Utils.NativeFileExists(filePath))
if (File.Exists(filePath))
Copy link
Member

@daxian-dbw daxian-dbw Jun 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use Utils.FileExists ?

Copy link
Collaborator Author

@iSazonov iSazonov Jun 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Utils.FileExists behavior is the same (no exception is thrown). So it is better to use the standard method.

Copy link
Member

@daxian-dbw daxian-dbw Jun 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to make all the Utils.NativeFileExists -> File/Directory.Exists changes in a separate PR. So for now, just do the simple replacement of Utils.NativeFileExists -> Utils.FileExists.

{
localizedFile = filePath;
break;
Expand Down Expand Up @@ -4278,7 +4278,7 @@ private bool GetListOfFilesFromData(
// which we can't really do b/c the file doesn't exist.
fixedFileName = psHome + "\\" + Path.GetFileName(s);
}
else if (verifyFilesExist && !Utils.NativeFileExists(fixedFileName))
else if (verifyFilesExist && !Utils.FileExists(fixedFileName))
{
string message = StringUtil.Format(SessionStateStrings.PathNotFound, fixedFileName);
throw new FileNotFoundException(message, fixedFileName);
Expand Down Expand Up @@ -5002,7 +5002,7 @@ internal PSModuleInfo IsModuleImportUnnecessaryBecauseModuleIsAlreadyLoaded(stri
else // reimport the module + return alreadyLoadedModule (alreadyLoadedModule = no need to proceed with regular import)
{
// If the module has already been loaded, then while loading it the second time, we should load it with the DefaultCommandPrefix specified in the module manifest. (If there is no Prefix from command line)
if (string.IsNullOrEmpty(prefix) && Utils.NativeFileExists(alreadyLoadedModule.Path))
if (string.IsNullOrEmpty(prefix) && Utils.FileExists(alreadyLoadedModule.Path))
{
string defaultPrefix = GetDefaultPrefix(alreadyLoadedModule);
if (!string.IsNullOrEmpty(defaultPrefix))
Expand Down Expand Up @@ -5178,7 +5178,7 @@ internal PSModuleInfo LoadUsingExtensions(PSModuleInfo parentModule,
found = true;
return module;
}
else if (Utils.NativeFileExists(fileName))
else if (Utils.FileExists(fileName))
{
moduleFileFound = true;
// Win8: 325243 - Added the version check so that we do not unload modules with the same name but different version
Expand Down Expand Up @@ -5328,7 +5328,7 @@ internal PSModuleInfo LoadModule(PSModuleInfo parentModule, string fileName, str
{
Dbg.Assert(fileName != null, "Filename argument to LoadModule() shouldn't be null");

if (!Utils.NativeFileExists(fileName))
if (!Utils.FileExists(fileName))
{
found = false;
moduleFileFound = false;
Expand Down
4 changes: 2 additions & 2 deletions src/System.Management.Automation/engine/Modules/ModuleUtils.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ internal static List<string> GetModuleVersionsFromAbsolutePath(string directory)
{
string moduleFile = Path.Combine(directory, fileName) + ext;

if (!Utils.NativeFileExists(moduleFile))
if (!Utils.FileExists(moduleFile))
{
continue;
}
Expand Down Expand Up @@ -233,7 +233,7 @@ internal static IEnumerable<string> GetDefaultAvailableModuleFiles(string topDir
foreach (string ext in ModuleIntrinsics.PSModuleExtensions)
{
string moduleFile = Path.Combine(directoryToCheck, proposedModuleName) + ext;
if (!Utils.NativeFileExists(moduleFile))
if (!File.Exists(moduleFile))
Copy link
Member

@daxian-dbw daxian-dbw Jun 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.
Actually, I see Utils.NativeFileExists and File.Exists mixed in this file (possibly in that files too). Do we really care about the exception on failure in those cases?

Copy link
Collaborator Author

@iSazonov iSazonov Jun 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Utils.FileExists behavior is the same (no exception is thrown). So it is better to use the standard method.

Yes, we should review all these cases but there are dozens of them to do in this PR or in a new?

Copy link
Member

@daxian-dbw daxian-dbw Jun 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. let's do it in a separate PR. For this PR, just do the simple rename replacement: Utils.NativeFileExists -> Utils.FileExists.

{
continue;
}
Expand Down
4 changes: 2 additions & 2 deletions src/System.Management.Automation/engine/SessionStateDriveAPIs.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -965,9 +965,9 @@ private bool IsAStaleVhdMountedDrive(PSDriveInfo drive)
{
try
{
// Checking for the presence of mounted drive locally using Utils.NativeDirectoryExists API as
// Checking for the presence of mounted drive locally using Utils.DirectoryExists API as
// the calls to this API is faster than normal Directory.Exist API.
bool validDrive = Utils.NativeDirectoryExists(drive.Root);
bool validDrive = Utils.DirectoryExists(drive.Root);
if (!validDrive)
{
result = true;
Expand Down
104 changes: 24 additions & 80 deletions src/System.Management.Automation/engine/Utils.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -505,7 +505,7 @@ internal static bool IsValidPSEditionValue(string editionValue)
T policy = null;
#if !UNIX
// On Windows, group policy settings from registry take precedence.
// If the requested policy is not defined in registry, we query the configuration file.
// If the requested policy is not defined in registry, we query the configuration file.
policy = GetPolicySettingFromGPO<T>(preferenceOrder);
if (policy != null) { return policy; }
#endif
Expand Down Expand Up @@ -895,113 +895,57 @@ internal static bool IsAdministrator()
#endif
}

internal static bool NativeItemExists(string path)
internal static bool ItemExists(string path)
{
bool unusedIsDirectory;
Exception unusedException;
try
{
return ItemExists(path, out bool _);
}
catch
{
}

return NativeItemExists(path, out unusedIsDirectory, out unusedException);
return false;
}

// This is done through P/Invoke since File.Exists and Directory.Exists pay 13% performance degradation
// through the CAS checks, and are terribly slow for network paths.
internal static bool NativeItemExists(string path, out bool isDirectory, out Exception exception)
internal static bool ItemExists(string path, out bool isDirectory)
Copy link
Member

@daxian-dbw daxian-dbw Jun 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, maybe changing this to CheckItemExistsAndThrowOnError.

Copy link
Collaborator Author

@iSazonov iSazonov Jun 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll rename if we will not migrate to File.Exists/Directory.Exists in the PR.

{
exception = null;
isDirectory = false;

if (String.IsNullOrEmpty(path))
{
isDirectory = false;
return false;
}
#if UNIX
isDirectory = Platform.NonWindowsIsDirectory(path);
return Platform.NonWindowsIsFile(path);
#else

if (IsReservedDeviceName(path))
{
isDirectory = false;
return false;
}

int result = NativeMethods.GetFileAttributes(path);
if (result == -1)
try
{
int errorCode = Marshal.GetLastWin32Error();
if (errorCode == 5)
{
// Handle "Access denied" specifically.
Win32Exception win32Exception = new Win32Exception(errorCode);
exception = new UnauthorizedAccessException(win32Exception.Message, win32Exception);
}
else if (errorCode == 32)
{
Copy link
Member

@daxian-dbw daxian-dbw Jun 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

File.GetAttributes doesn't spend this extra effort in case the errorCode is 32 (ERROR_SHARING_VIOLATION). So, this is potentially a breaking change.

Copy link
Member

@daxian-dbw daxian-dbw Jun 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same happens to WinSafeGetFileAttributes which is removed by #6909.

Copy link
Collaborator Author

@iSazonov iSazonov Jun 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see the PR description (Fix is ready in CoreFX, we get it in 2.1.1.)

Copy link
Collaborator Author

@iSazonov iSazonov Jun 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see the PR description - we'll get the fix in .Net Core 2.1.1.

Copy link
Member

@daxian-dbw daxian-dbw Jun 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, so this will only affect c:\pagefile.sys? If so, then it's fine to leave it as is.

Copy link
Collaborator Author

@iSazonov iSazonov Jun 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Three files - pagefile.sys, hiberfil.sys, swapfile.sys. In the CoreFX PR a comment says that they can not emulate the behavior and so use the real file c:\pagefile.sys in test. I think users are unlikely to find something like this

// Errorcode 32 is 'ERROR_SHARING_VIOLATION' i.e.
// The process cannot access the file because it is being used by another process.
// GetFileAttributes may return INVALID_FILE_ATTRIBUTES for a system file or directory because of this error.
// GetFileAttributes function tries to open the file with FILE_READ_ATTRIBUTES access right but it fails if the
// sharing flag for the file is set to 0x00000000.This flag prevents it from opening a file for delete, read, or
// write access. For example: C:\pagefile.sys is always opened by OS with sharing flag 0x00000000.
// But FindFirstFile is still able to get attributes as this api retrieves the required information using a find
// handle generated with FILE_LIST_DIRECTORY access.
// Fall back to FindFirstFile to check if the file actually exists.
IntPtr INVALID_HANDLE_VALUE = new IntPtr(-1);
NativeMethods.WIN32_FIND_DATA findData;
IntPtr findHandle = NativeMethods.FindFirstFile(path, out findData);
if (findHandle != INVALID_HANDLE_VALUE)
{
isDirectory = (findData.dwFileAttributes & NativeMethods.FileAttributes.Directory) != 0;
NativeMethods.FindClose(findHandle);
return true;
}
}
else if (errorCode == 53)
{
// ERROR_BAD_NETPATH - The network path was not found.
Win32Exception win32Exception = new Win32Exception(errorCode);
exception = new IOException(win32Exception.Message, win32Exception);
}
// Use 'File.GetAttributes()' because we want to get access exceptions.
FileAttributes attributes = File.GetAttributes(path);
isDirectory = attributes.HasFlag(FileAttributes.Directory);

isDirectory = false;
return (int)attributes != -1;
}
catch (IOException)
{
return false;
}

isDirectory = (result & ((int)NativeMethods.FileAttributes.Directory)) ==
((int)NativeMethods.FileAttributes.Directory);

return true;
#endif
}

// This is done through P/Invoke since we pay 13% performance degradation
// through the CAS checks required by File.Exists and Directory.Exists
internal static bool NativeFileExists(string path)
internal static bool FileExists(string path)
Copy link
Member

@daxian-dbw daxian-dbw Jun 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think all uses of this method expects an exception to be thrown. Some uses might just believe that it's faster than File.Exists but expect it to behave like File.Exists().

I think you can inspect the uses of Utils.FileExists and Utils.DirectoryExists and change to File.Exists and Directory.Exists as appropriate.

{
bool isDirectory;
Exception ioException;

bool itemExists = NativeItemExists(path, out isDirectory, out ioException);
if (ioException != null)
{
throw ioException;
}
bool itemExists = ItemExists(path, out bool isDirectory);

return (itemExists && (!isDirectory));
}

// This is done through P/Invoke since we pay 13% performance degradation
// through the CAS checks required by File.Exists and Directory.Exists
internal static bool NativeDirectoryExists(string path)
internal static bool DirectoryExists(string path)
Copy link
Member

@daxian-dbw daxian-dbw Jun 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should change the name of this one and FileExists to something that indicate it might throw exception, so it's easy to tell whether you should use them instead of File.Exists and Directory.Exists when writing code.

I suggest rename them to CheckFileExistsAndThrowOnError and CheckDirectoryExistsAndThrowOnError.

Copy link
Collaborator Author

@iSazonov iSazonov Jun 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll rename if we will not migrate to File.Exists/Directory.Exists in the PR.

Copy link
Member

@daxian-dbw daxian-dbw Jun 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, let's use a separate PR to do the migration changes. Go ahead rename this method.

{
bool isDirectory;
Exception ioException;

bool itemExists = NativeItemExists(path, out isDirectory, out ioException);
if (ioException != null)
{
throw ioException;
}
bool itemExists = ItemExists(path, out bool isDirectory);

return (itemExists && isDirectory);
}
Expand Down
4 changes: 2 additions & 2 deletions src/System.Management.Automation/help/UpdatableHelpSystem.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1636,7 +1636,7 @@ internal static string GetFilePath(string path)
string fileName = item.Name;

// Prerequisite: The directory in the given path must exist and it is case sensitive.
if (Utils.NativeDirectoryExists(directoryPath))
if (Utils.DirectoryExists(directoryPath))
{
// Get the list of files in the directory.
string[] fileList = Directory.GetFiles(directoryPath);
Expand All @@ -1649,7 +1649,7 @@ internal static string GetFilePath(string path)
}
}
#else
if (Utils.NativeFileExists(path))
if (Utils.FileExists(path))
{
return path;
}
Expand Down
Loading