Mac: Add PreDelete Notification #181

wilbaker · 2018-08-16T22:26:18Z

Part of the work required for #152.

Changes for PreDelete

Added MessageType_KtoU_NotifyFilePreDelete and MessageType_KtoU_NotifyDirectoryPreDelete (kernel->user) notifications
Updated VFSForGit's user mode code to handle the PreDelete notification
Added new Categories.Mac.M2TODO functional test categories for tests that are required for M2 but not yet passing
Enabled additional functional tests for file\folder deletes
Added PreDelete handling to MirrorProvider

Unrelated Fixes\Changes

Updated Message_Init to properly set the pid and procname in the MessageHeader
Updated ParseMessageMemory to set the path to empty string rather than null (when the message from the kernel has a null path)
Made log formatting more consistent in MirrorProvider
Updated Kext project to treat warnings as errors

Mac functional test run with these changes

Test Run Summary
  Overall result: Warning
  Test Count: 156, Passed: 154, Failed: 0, Warnings: 0, Inconclusive: 0, Skipped: 2
    Skipped Tests - Ignored: 2, Explicit: 0, Other: 0
  Start time: 2018-08-17 15:55:50Z
    End time: 2018-08-17 15:59:53Z
    Duration: 242.238 seconds

wilbaker · 2018-08-16T22:31:29Z

ProjFS.Mac/PrjFSKext/PrjFSKext.xcodeproj/project.pbxproj

@@ -196,6 +196,7 @@
 				TargetAttributes = {
 					C6C780AF207FC67200E7E054 = {
 						CreatedOnToolsVersion = 9.3;
+						ProvisioningStyle = Automatic;


@sanoursa @nickgra any ideas what this setting does? At some point xcode decided to add it to the project.

I was going to ask you the same thing :-)

Did you update your XCode version lately? Apple is reworking some things with signing and provisioning profiles, so this is likely related. We should just take the change now (like we do when VS insists on changing a project metadata file), no point in fighting with the IDE.

sanoursa

The code changes are looking good. I'll try to give this a test on my machine as well, though you don't need to block on that if I don't get to it in time.

sanoursa · 2018-08-16T22:28:29Z

GVFS/GVFS.FunctionalTests/FileSystemRunners/BashRunner.cs

@@ -233,7 +239,14 @@ public override void DeleteFile_FileShouldNotBeFound(string path)

        public override void DeleteFile_AccessShouldBeDenied(string path)
        {
-            this.DeleteFile(path).ShouldContain(permissionDeniedMessage);
+            if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))


If we start getting more of these, we could create a WindowsBashRunner and a MacBashRunner. This seems fine for now though.

sanoursa · 2018-08-16T22:29:05Z

GVFS/GVFS.FunctionalTests/FileSystemRunners/BashRunner.cs

-            this.DeleteFile(path).ShouldContain(permissionDeniedMessage);
+            if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
+            {
+                this.DeleteFile(path).ShouldContain(permissionDeniedMessage);


Alternatively, don't bother checking the platform, and just make this ShouldContainOneOf(both messages)

I'd rather not relax the check when we're running the tests on Windows. I like the idea of separate Windows\Mac BashRunners, but I also agree at this point we probably don't need both.

sanoursa · 2018-08-16T22:30:31Z

GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/GitFilesTests.cs

@@ -51,11 +54,12 @@ public void CreateFileInFolderTest()

            this.Enlistment.WaitForBackgroundOperations().ShouldEqual(true, "Background operations failed to complete.");

-            GVFSHelpers.ModifiedPathsShouldContain(this.fileSystem, this.Enlistment.DotGVFSRoot, folderName + "/" + Environment.NewLine);


Consider renaming ModifiedPathsShouldContain to ModifiedPathsShouldContainLine, and move the newline inside the method. That way every caller doesn't have to know this detaill

I like that idea, that will also allow for making ModifiedPathsNewLine private (as mentioned in a separate comment)

I will move the newline inside the method but leave the method named ModifiedPathsShouldContain.

I found ModifiedPathsShouldContainLine looked a bit off as the caller is not passing in lines, it's passing in the paths that ``ModifiedPaths` should (or should not) have.

sanoursa · 2018-08-16T22:31:24Z

GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/GitFilesTests.cs

@@ -153,11 +158,13 @@ public void ReadingFileDoesNotUpdateIndexOrSparseCheckout()
            afterUpdateResult.Output.StartsWith("S ").ShouldEqual(true);
            afterUpdateResult.Output.ShouldContain("ctime: 0:0", "mtime: 0:0", "size: 0\t");

-            GVFSHelpers.ModifiedPathsShouldNotContain(this.fileSystem, this.Enlistment.DotGVFSRoot, gitFileToCheck + Environment.NewLine);
+            GVFSHelpers.ModifiedPathsShouldNotContain(this.fileSystem, this.Enlistment.DotGVFSRoot, gitFileToCheck + GVFSHelpers.ModifiedPathsNewLine);


Same for ShouldNotContainLine

sanoursa · 2018-08-16T22:32:12Z

GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/GitMoveRenameTests.cs

@@ -139,7 +145,7 @@ public void GitStatusAfterUnstage()
        public void GitStatusAfterFileDelete()
        {
            string existingFilename = "test.cs";
-            this.Enlistment.GetVirtualPathTo(existingFilename).ShouldBeAFile(this.fileSystem);
+            this.EnsureTestFileExists(existingFilename);


Why did you need to change the behavior here?

Unfortunately all of the tests in this class were written to be completely dependent on all of the tests running in order.

GitStatusAfterFileDelete was assuming that all prior tests ran, and one of those tests creates "test.cs". Because we can't run all of the tests just yet there is currently no "test.cs" at the point this test runs.

With this change you can run this individual test without having to run the tests listed above it.

sanoursa · 2018-08-16T22:35:54Z

ProjFS.Mac/PrjFSKext/PrjFSKext/KauthHandler.cpp

+
+            // This delete could be part of a rename, and so we must hydrate the file now to
+            // ensure it's hydrated post-rename
+            if (kauthResult == KAUTH_RESULT_DEFER &&


How could the result currently be anything but defer?

With VFSForGit it will always be defer, but if a provider decided to block the delete of an empty placeholder it could be DENY.

(The idea here was to only hydrate if the delete is going to be allowed, let me know what you think)

See related comment, I'll refactor this code to avoid duplication (and to hydrate first)

sanoursa · 2018-08-16T22:36:59Z

ProjFS.Mac/PrjFSKext/PrjFSKext/KauthHandler.cpp

+            if (kauthResult == KAUTH_RESULT_DEFER &&
+                FileFlagsBitIsSet(currentVnodeFileFlags, FileFlags_IsEmpty))
+            {
+                if (!TrySendRequestAndWaitForResponse(


Can we avoid duplicating this logic for hydrating a file? It seems like KAUTH_VNODE_DELETE should just be another action that can trigger a hydration in the existing code path. Then after that, we could check for KAUTH_VNODE_DELETE again and trigger the pre-delete notification.

I think we could move the PreDelete notification code down and avoid duplicating the logic. It's a trade-off between avoiding duplicating the logic and preventing an unnecessary hydration if a provider blocks the delete.

However, blocking the delete of an unhydrated would be quite rare (and it's just a perf tradeoff), and so I'll go with your suggestion as I'd rather keep the kext simpler at this point.

sanoursa · 2018-08-16T22:37:22Z

ProjFS.Mac/PrjFSKext/PrjFSKext/KauthHandler.cpp

-        if (ActionBitIsSet(
+        if (ActionBitIsSet(action, KAUTH_VNODE_DELETE))
+        {
+            if (!TrySendRequestAndWaitForResponse(


Though I'm curious, why are you triggering pre-delete before hydration? Does the ordering make any difference?

Does the ordering make any difference?

If the provider decides to block the delete it avoids an unnecessary hydration (I don't think VFSForGit would ever do that though ...)

Assuming the provider allows the delete, I don't think the ordering would make a difference. You could argue that the provider wouldn't get the PreDelete notification if the hydration were to fail, but I'm not sure that's enough of a reason to re-order these.

See related comment, I'll refactor this code to avoid duplication (and to hydrate first)

Update: For now I've removed all of the "hydrate on delete" code. Interestingly, it seems like we still get some kauth request that hydrates the file before moving. I'll test more in this area when adding support for rename notifications.

sanoursa · 2018-08-16T22:38:01Z

ProjFS.Mac/PrjFSKext/PrjFSKext/KauthHandler.cpp

+        // TODO(Mac): Determine the overhead of making this check for all actions and
+        // vnodes.  If it's too high, this check can be further reduced to the scenarios
+        // that VFSForGit care about.
+        if (!HasParentInVirtualizationRoot(vnode, context))


Why did you need this change in behaivor?

Without this change VFSForGit would not get PreDelete notifications for the index file as it does not have the FileFlags_IsInVirtualizationRoot flag set.

In general the FileFlags_IsInVirtualizationRoot will only be set for files inside the root that originated as placeholders. It will not be set for newly created files or files that were renamed into the root.

(And I decided to walk up the tree looking for the flag as we still want to get out the way as fast as we can for files we don't care about, and so I didn't want to add the overhead of looking up the VirtualizationRoot* here)

In the prototype, we detected that a new file was created, and set the FileFlags_IsInVirtualizationRoot bit on it. The point of that flag is to give you one spot to check for this optimization. Though I think @pmj might have some plans to get rid of that flag and just do a vnode lookup instead.

In the prototype, we detected that a new file was created, and set the FileFlags_IsInVirtualizationRoot bit on it. The point of that flag is to give you one spot to check for this optimization.

Got it that makes sense.

It sounds like we will need this change for handling when new files are created, and at that time we can add the FileFlags_IsInVirtualizationRoot flag and eliminate this check for other notifications\events (like PreDelete).

The other complication is that the index file is created before the virtualization root is active. If we want to continue to block deletes of the index we'll either need to be OK with only having this protection for index files created after the initial index, or we'll need a way for the clone verb to stick this flag on the index after it's created.

My plan is to pick up new file notifications next. Let me know if you'd like me to combine those changes with these changes, or if you're OK with me merging these changes in first and then making the changes discussed above in the next PR.

If doing that work avoids the need for workarounds that we're going to immediately throw away, it may be worth combining them.

sanoursa · 2018-08-16T22:38:39Z

ProjFS.Mac/PrjFSKext/PrjFSKext/KauthHandler.cpp

+        //
+        // TODO(Mac): Double check that rules are properly enforced when filedFlaggedAsInVirtualizationRoot
+        // is false.  These checks are currently skipped when filedFlaggedAsInVirtualizationRoot is false to
+        // allow providers to create new files inside of their root before starting the virtualization


Why do we want to allow that?

The problem we have right now is that gvfs mount will always attempt to copy in the hooks.

This code didn't cause a problem before because prior to my change above (to proceed even when FileFlags_IsInVirtualizationRoot is not set on the file), the kext would bail out before here because neither the .git folder nor the hooks file have the flag set.

I think that once we have the all of the FileOp\Vnode event handling wired up we'll need to revisit how to handle these offline scenarios.

I've filed a follow up issue (#182) for this

See related comment, this might not be an issue after new file handling is added (and FileFlags_IsInVirtualizationRootis stamped on new files)

sanoursa · 2018-08-16T22:45:44Z

ProjFS.Mac/PrjFSKext/PrjFSKext/KauthHandler.cpp

@@ -284,10 +296,56 @@ static int HandleVnodeOperation(
                }
            }
        }
+        else if (ActionBitIsSet(action, KAUTH_VNODE_DELETE))


One more question - do we need to trigger a recursive hydration of this directory? What's the current behavior if you move a virtual directory? What if you move a directory that is non-empty but has virtual items nested deep within it?

I'll leave this investigation to the PR that adds support for the rename notification.

nickgra · 2018-08-16T23:30:15Z

ProjFS.Mac/PrjFSKext/PrjFSKext.xcodeproj/project.pbxproj

@@ -196,6 +196,7 @@
 				TargetAttributes = {
 					C6C780AF207FC67200E7E054 = {
 						CreatedOnToolsVersion = 9.3;
+						ProvisioningStyle = Automatic;


Did you update your XCode version lately? Apple is reworking some things with signing and provisioning profiles, so this is likely related. We should just take the change now (like we do when VS insists on changing a project metadata file), no point in fighting with the IDE.

wilbaker · 2018-08-16T23:43:05Z

@nickgra @pmj, after seeing the feedback from @sanoursa (and how things can get cleaned up with handling new file notifications) I've decided to make the changes for new file notifications in this PR as well.

Feel free to wait until those changes are ready before reviewing.

Thanks!

nickgra

Waiting on your proposed changes.

wilbaker · 2018-08-17T15:42:53Z

I've decided to make the changes for new file notifications in this PR as well.

@sanoursa @nickgra @pmj, after looking into what's needed for new file notifications, I've come to the conclusion that it would be cleaner to leave those changes out of this PR. Rather than adding new file notifications, I'll update this by removing all of the code that's not strictly needed to get basic PreDelete notifications working (e.g. hydrating on delete, delete notifications for renamed files, etc.)

In the next round of PRs (when adding new file and rename notifications), I can add back support for the more complicated scenarios without the need for the work around currently in this PR.

I'll post another message to this PR when it's ready for another round of review.

Thanks!

…vider

wilbaker · 2018-08-17T17:38:24Z

after looking into what's needed for new file notifications, I've come to the conclusion that it would be cleaner to leave those changes out of this PR. Rather than adding new file notifications, I'll update this by removing all of the code that's not strictly needed to get basic PreDelete notifications working (e.g. hydrating on delete, delete notifications for renamed files, etc.)

@sanoursa @nickgra @pmj, this cleanup as been completed and the PR is ready for another look.

sanoursa · 2018-08-17T18:38:07Z

ProjFS.Mac/PrjFSKext/PrjFSKext/KauthHandler.cpp

@@ -284,10 +296,38 @@ static int HandleVnodeOperation(
                }
            }
        }
+        else if (ActionBitIsSet(action, KAUTH_VNODE_DELETE))


Will the DELETE bit always be exclusive of all the bits we checked for above? You may need to drop this else to avoid missing events.

Good point, I'll remove the elses

sanoursa · 2018-08-17T18:38:38Z

ProjFS.Mac/PrjFSKext/PrjFSKext/KauthHandler.cpp

+                goto CleanupAndReturn;
+            }
+        }
+        else if (ActionBitIsSet(


Same here, probably want to drop the else

nickgra

LGTM

Mac: Add PreDelete Notification

81afe7f

wilbaker added domain: kext domain: user-mode labels Aug 16, 2018

wilbaker self-assigned this Aug 16, 2018

wilbaker commented Aug 16, 2018

View reviewed changes

wilbaker requested review from sanoursa and nickgra August 16, 2018 22:37

sanoursa reviewed Aug 16, 2018

View reviewed changes

nickgra approved these changes Aug 16, 2018

View reviewed changes

wilbaker changed the title ~~Mac: Add PreDelete Notification~~ [WIP] Mac: Add PreDelete and new file notifications Aug 16, 2018

wilbaker requested review from nickgra and removed request for nickgra August 16, 2018 23:49

nickgra suggested changes Aug 16, 2018

View reviewed changes

wilbaker changed the title ~~[WIP] Mac: Add PreDelete and new file notifications~~ Mac: Add PreDelete Notification Aug 17, 2018

wilbaker added 4 commits August 17, 2018 09:32

Reduce scope of changes to be specific to PreDelete, update MirrorPro…

25fc24a

…vider

Fix notification registration in MirrorProvider

e9a616c

Move newline into ModifiedPathsShouldNotContain

ac386e5

Add logging to KauthHandler_HandleKernelMessageResponse

fa42007

wilbaker added 2 commits August 17, 2018 10:44

Merge branch 'master' into users/wilbaker/pre_delete_notification

7266b9f

Fix failing functional tests

52f6522

sanoursa approved these changes Aug 17, 2018

View reviewed changes

wilbaker added 2 commits August 17, 2018 12:25

Remove elses when checking for delete in HandleVnodeOperation

0d29e46

Merge branch 'master' into users/wilbaker/pre_delete_notification

522e4e5

nickgra approved these changes Aug 17, 2018

View reviewed changes

wilbaker merged commit 6fa5dfe into microsoft:master Aug 17, 2018

jrbriggs mentioned this pull request Oct 12, 2018

Merge milestones/M142 to releases/shipped #381

Merged

		@@ -51,11 +54,12 @@ public void CreateFileInFolderTest()

		this.Enlistment.WaitForBackgroundOperations().ShouldEqual(true, "Background operations failed to complete.");

		GVFSHelpers.ModifiedPathsShouldContain(this.fileSystem, this.Enlistment.DotGVFSRoot, folderName + "/" + Environment.NewLine);

Mac: Add PreDelete Notification #181

Mac: Add PreDelete Notification #181

Conversation

wilbaker commented Aug 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanoursa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanoursa Aug 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilbaker commented Aug 16, 2018

nickgra left a comment

Choose a reason for hiding this comment

wilbaker commented Aug 17, 2018

wilbaker commented Aug 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickgra left a comment

Choose a reason for hiding this comment

wilbaker commented Aug 16, 2018 •

edited

Loading

sanoursa Aug 16, 2018 •

edited

Loading