Skip to content

Mac: Check the return value of stat in PrjFS_DeleteFile#1018

Merged
wilbaker merged 1 commit into
microsoft:masterfrom
wilbaker:prjfs_delete_file_errors
Apr 9, 2019
Merged

Mac: Check the return value of stat in PrjFS_DeleteFile#1018
wilbaker merged 1 commit into
microsoft:masterfrom
wilbaker:prjfs_delete_file_errors

Conversation

@wilbaker
Copy link
Copy Markdown
Member

@wilbaker wilbaker commented Apr 9, 2019

Fixes #1019

PrjFS_DeleteFile is incorrectly returning PrjFS_Result_EVirtualizationInvalidOperation when it's called for files/directories that do not exist.

This results in a lot of misleading messages being recorded in the GVFS mount process log files.

Example log message

[2019-04-08 13:18:54 -07:00] RemoveFolderPlaceholderIfEmpty_DeleteFileFailure {"Area":"GitIndexProjection","Folder Path":"Test_EPF_GitCommandsTestOnlyFileFolder","result.Result":"VirtualizationInvalidOperation","result.RawResult":536871936,"UpdateFailureCause":"DirtyData"}

Prior to this PR the functional tests recorded RemoveFolderPlaceholderIfEmpty_DeleteFileFailure 129 times. With this change the message is not recorded at all during the functional test runs.

@wilbaker wilbaker added this to the M151 milestone Apr 9, 2019
@wilbaker
Copy link
Copy Markdown
Member Author

wilbaker commented Apr 9, 2019

@kewillford I noticed this issue when looking at the logs for your #1014 PR.

I noticed this was getting logged a lot in your test run (as well as successful test runs) and the noise of this getting logged all of the time made it difficult to understand if there was a problem.

switch(errno)
{
case ENOENT: // A component of fullPath does not exist
case ENOTDIR: // A component of fullPath is not a directory
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do you know when it would hit this case? How does that stat know that you are looking for a directory? What if this was called for a file? I don't think in VFSForGit we ever do try to delete a file though.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Do you know when it would hit this case?

I would expect the ENOTDIR case to be hit if you called stat on a path like /a/b/c/d.txt and a/b/c is a file.

How does that stat know that you are looking for a directory?
What if this was called for a file?

stat doesn't know what you're looking for, these errors indicate that the file that you're looking for is not on disk (either because part of the path does not exist, or part of the path is a file).

I don't think in VFSForGit we ever do try to delete a file though.

In VFS4G we delete files when changing to a projection where some of the file placeholders no longer exist.

@wilbaker wilbaker merged commit 4e9aad3 into microsoft:master Apr 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants