-
Notifications
You must be signed in to change notification settings - Fork 256
Fix: Enable Windows to determine File or Folder in events #722
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.
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
base: main
Are you sure you want to change the base?
Conversation
| }; | ||
| } | ||
| match cur_entry.Action { | ||
| FILE_ACTION_RENAMED_OLD_NAME => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also rolled the FILE_ACTION_RENAMED_OLD_NAME into the match... Because I can't see any reason it shouldn't be in it already. Was there a good reason that I'm unaware of? If so, I can revert this part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also don't see any reason and I'm ok with the current :)
…ich the FileAttributes can tell if a file or folder is being created/modified/deleted. This raises the minimum required Windows version to Windows 10, as this is where ReadDirectoryChangesExW was introduced.
54df206 to
572dfbf
Compare
|
As per @JohnTitor in #261, I made it backwards compatible for older versions of Windows too, with a one time runtime check (the results of which are passed around). Did so by effectively duplicating handle_event (into handle_extended_event). I'm sure there's portions of the code that can be made more DRY, but then again, it might be less error prone to have to keep up to date two copies, and maybe eventually drop the older one. |
…le (Windows versions earlier than 10). Checked dynamically once at server creation and passed along to the ReadData.
ad4f803 to
bdba4e0
Compare
| if ret == 0 { | ||
| // error reading. retransmute request memory to allow drop. | ||
| // Because of the error, ownership of the `overlapped` alloc was not passed | ||
| // over to `ReadDirectoryChangesExW`. | ||
| // So we can claim ownership back. | ||
| let _overlapped = Box::from_raw(overlapped); | ||
| let request = Box::from_raw(request); | ||
| ReleaseSemaphore(request.data.complete_sem, 1, ptr::null_mut()); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it could fail silently, right? If so, the watched paths are silently ignored and it may cause some confusion. Some form of logging would be nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there some backend independent logging I could hook into? I don't think it's wise to add one only for windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use the log crate, we've already used it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind... I saw log::error!()... I'll use that.
…eTypeOfEvent # Conflicts: # CHANGELOG.md # notify/Cargo.toml
526a614 to
1f4cc65
Compare
| _ => { | ||
| // Some unidentified error occurred, log and unwatch the directory, then return. | ||
| log::error!( | ||
| "unknown error in ReadDirectoryChangesExW for directory {}: {}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this line handles errors already... No need to log the error twice.
Replace ReadDirectoryChangesW with ReadDirectoryChangesExW, out of which the FileAttributes can tell if a file or folder is being created/modified/deleted.
This raises the minimum required Windows version to Windows 10, as this is where ReadDirectoryChangesExW was introduced.
Fixes #261