Skip to content
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

Library - Allow FindFilesWithPattern to return STATUS_NOT_IMPLEMENTED #1096

Merged
merged 1 commit into from
Jun 2, 2022

Conversation

KoltesDigital
Copy link
Contributor

@KoltesDigital KoltesDigital commented Jun 1, 2022

Reverts #9eb4d15bc4c533b73dd718b3114c0be758f874d9

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the existing documentation
  • My changes generate no new warnings
  • I have updated the change log (Add/Change/Fix)
  • I have cleaned up the commit history (use rebase and squash)

Changes proposed in this pull request:

I'm upgrading the Rust bindings to use ^2. The high-level API always defines FindFilesWithPattern, with a default implementation returning STATUS_NOT_IMPLEMENTED. But the MemFS example was not working well. I discovered that a few months ago, this function was not allowed to return STATUS_NOT_IMPLEMENTED anymore. I find that's too bad.

After I tried for a long time to allow Rust to choose whether FindFiles and FindFilesWithPattern are defined, I finally moved to changing the C library's behavior. It gives back the ability to all functions to return STATUS_NOT_IMPLEMENTED. I believe this will be beneficial for other bindings as well.

dokan/directory.c Show resolved Hide resolved
@Liryna
Copy link
Member

Liryna commented Jun 2, 2022

Thank you @KoltesDigital for the pull request! I agree it is way easier to change the library than adapting all the wrappers. This should have been done at first but we needed a 💪 contributor like you to make it happen!
Please see the comment I added.

@KoltesDigital
Copy link
Contributor Author

Haha don't overstate it 😄

@KoltesDigital
Copy link
Contributor Author

While you're here: is there a reason why PFillFindData returns 0 on success and PFillFindStreamData returns 0 on error?

@Liryna
Copy link
Member

Liryna commented Jun 2, 2022

Thanks for the quick change! 🏆

Regarding PFillFindData & PFillFindStreamData , some context: The difference is that directory listing is produced by a IRP_MJ_DIRECTORY_CONTROL which supports Flags like SL_INDEX_SPECIFIED which request to resume the listing (previously stopped by the destination buffer being too small) from where we stopped. That's why DokanFillFileData cache all the items and never return an error right now.

Stream listing is produced by IRP_MJ_QUERY_INFORMATION which does not have this logic. When the buffer is full, we just return buffer overflow so that the kernel reallocate a larger one until it fit the data. That's why DokanFillFindStreamData return a failure when the buffer is full to ask the user FS to abort the listing since no new entry can be added.

So now answering your question why 0 is a success for PFillFindData and FALSE is a failure for PFillFindStreamData: Legacy logic 😢 There is no reason. Maybe it should have been changed when we moved to 2.x

@Liryna Liryna merged commit da3078d into dokan-dev:master Jun 2, 2022
@KoltesDigital KoltesDigital deleted the patch-2 branch June 2, 2022 16:45
@KoltesDigital
Copy link
Contributor Author

Thanks for the explanations, the merge, and the project overall!

@KoltesDigital
Copy link
Contributor Author

KoltesDigital commented Jun 2, 2022

I just noticed that it's written at the bottom of https://github.com/dokan-dev/dokany/wiki/Update-Dokan-1.1.0-application-to-Dokany-2.0.0 . I'm not allowed to change it, so please consider editing it!

@Liryna
Copy link
Member

Liryna commented Jun 3, 2022

@KoltesDigital
Copy link
Contributor Author

Thanks! Last annoyance 😄 I think you've made a typo, you wrote 2.5.0 but it should be 2.0.5.

BTW are you maintainer on the Rust repo by any chance?

@Liryna
Copy link
Member

Liryna commented Jun 7, 2022

Indeed 😄 I updated the version! Thanks
I am not a rust maintainer but I can make you one if you are interested. Let's just wait a few more days to give a chance to @DDoSolitary to review it. Otherwise I will see what I can do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants