Skip to content

Recursively expand directories before delete and enable folder rename functional tests#272

Merged
wilbaker merged 5 commits intomicrosoft:masterfrom
wilbaker:mac_expand_b4_delete
Sep 28, 2018
Merged

Recursively expand directories before delete and enable folder rename functional tests#272
wilbaker merged 5 commits intomicrosoft:masterfrom
wilbaker:mac_expand_b4_delete

Conversation

@wilbaker
Copy link
Member

Fixes #202

Summary of changes:

  • Added a new kernel->user message MessageType_KtoU_RecursivelyEnumerateDirectory
  • The new message is sent from the kext to user mode whenever a directory delete is about to occur
  • Handling of the message is done by the user mode library. The lib walks the directory and asks the provider to expands any empty directories it finds
  • Enabled partial folder rename functional tests

Copy link
Contributor

@sanoursa sanoursa left a comment

Choose a reason for hiding this comment

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

The code looks good but it does potentially introduce more complicated error modes that I'd like to understand better.

@wilbaker
Copy link
Member Author

@sanoursa @nickgra this is ready for another look, let me know what you think.

Copy link
Member

@nickgra nickgra left a comment

Choose a reason for hiding this comment

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

Left some nits, but LGTM.

{
if (!TrySendRequestAndWaitForResponse(
root,
VDIR == vnodeType ? MessageType_KtoU_NotifyDirectoryPreDelete : MessageType_KtoU_NotifyFilePreDelete,
Copy link
Member

Choose a reason for hiding this comment

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

This TrySendRequestAndWaitForResponse and the one below structure their ternary statements differently. I have a preference for how I think it should look, but you can ignore that so long as they're consistent with each other :)

Can we pull the comparison up into a separate variable here too? Additionally, this line is getting long, I'd rather we break it up into 3 separate lines like we do in the below addition.

Copy link
Member Author

Choose a reason for hiding this comment

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

This TrySendRequestAndWaitForResponse and the one below structure their ternary statements differently. I have a preference for how I think it should look, but you can ignore that so long as they're consistent with each other

I can make them consistent, but could you provide more details on how you see them as inconsistent?

Can we pull the comparison up into a separate variable here too? Additionally, this line is getting long, I'd rather we break it up into 3 separate lines like we do in the below addition.

Yes to both of these, I'll make the change.

Copy link
Member Author

Choose a reason for hiding this comment

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

@nickgra I think I've addressed this feedback with my latest commit, but let me know if there's still changes you'd like to see.

@wilbaker wilbaker merged commit c9923a7 into microsoft:master Sep 28, 2018
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.

4 participants