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

How to return error to userland close? #1165

Open
5 tasks
ylgybbz opened this issue Aug 5, 2023 · 7 comments
Open
5 tasks

How to return error to userland close? #1165

ylgybbz opened this issue Aug 5, 2023 · 7 comments

Comments

@ylgybbz
Copy link

ylgybbz commented Aug 5, 2023

Feature request can skip this form. Bug report must complete it. Check List must be 100% match or it will be automatically closed without further discussion. Please remove this line.

Environment

  • Windows version:
  • Processor architecture:
  • Dokany version:
  • Library type (Dokany/FUSE):

Check List

  • I checked my issue doesn't exist yet
  • My issue is valid with mirror default sample and not specific to my user-mode driver implementation
  • I can always reproduce the issue with the provided description below.
  • I have updated Dokany to the latest version and have reboot my computer after.
  • I tested one of the last snapshot from appveyor CI

Description

we want to flush the buffer inside our filesystem driver when user called close, and return the result to userland. But Dokan CloseFile/CleanUp seems not provide any results to userland. I find the same question: #1157 (comment). Could you please provide more detailed information? Or any suggestion to solve this problem?

Logs

Please attach in separate files: mirror output, library logs and kernel logs.
In case of BSOD, please attach minidump or dump analyze output.

@Liryna
Copy link
Member

Liryna commented Aug 6, 2023

Hi @ylgybbz ,

Indeed there is currently no way to return a status for close but cleanup status is returned to the system.

Microsoft design is to release all resources attached to the handle in cleanup. Close is just here as a notification.

https://learn.microsoft.com/en-us/windows-hardware/drivers/kernel/irp-mj-close

I have rarely seen an application calling closehandle, checking the result and retrying it on failure. I would suggest to flush your buffer on cleanup if possible.

@ylgybbz
Copy link
Author

ylgybbz commented Aug 7, 2023

Hi @ylgybbz ,

Indeed there is currently no way to return a status for close but cleanup status is returned to the system.

Microsoft design is to release all resources attached to the handle in cleanup. Close is just here as a notification.

https://learn.microsoft.com/en-us/windows-hardware/drivers/kernel/irp-mj-close

I have rarely seen an application calling closehandle, checking the result and retrying it on failure. I would suggest to flush your buffer on cleanup if possible.

Thanks, but "Cleanup" have no return value too, "void(DOKAN_CALLBACK *Cleanup)(LPCWSTR FileName, PDOKAN_FILE_INFO DokanFileInfo)" . How can we return error when we flush buffer failed?

@Liryna
Copy link
Member

Liryna commented Aug 7, 2023

My bad, you are correct. It also does not allow returning a status.

We could make it possible to return a status but as said before, how will you handle the case if the user doesn't retry the CloseHandle to make your Cleanup/Close called again ?

Couldn't your implementation retry until it successfully flushed?

@ylgybbz
Copy link
Author

ylgybbz commented Aug 7, 2023

My bad, you are correct. It also does not allow returning a status.

We could make it possible to return a status but as said before, how will you handle the case if the user doesn't retry the CloseHandle to make your Cleanup/Close called again ?

Couldn't your implementation retry until it successfully flushed?
Yes,We are considering retrying the flush in cleanup, but in extreme scenarios, the flush failure may still occur. Retrying all the time may not be a good option. It would be better to be able to return an error

@Liryna
Copy link
Member

Liryna commented Aug 7, 2023

If you return an error, it does not mean cleanup/close will be called again which I imagine is worst.
You could cache the data pending to be flushed and do the flush when the situation is resolved.

@ylgybbz
Copy link
Author

ylgybbz commented Aug 7, 2023

If you return an error, it does not mean cleanup/close will be called again which I imagine is worst. You could cache the data pending to be flushed and do the flush when the situation is resolved.

If we return an error, the user may indicate that there is an error in the close operation, but if we do not return, the user will not even have the chance to know.
Returning to “clean up” first and then flushing also has certain problems, because the filesystem may core before flushing

@Liryna
Copy link
Member

Liryna commented Aug 7, 2023

Like I said before, I have very rarely seen code that retry closehandle on failure. They would just log the error at best. Your implementation should not count on third applications to safely save your data.

That said, I am not against that we allow returning a status for those callbacks. I would be glad to review pull requests for this feature 💪

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

No branches or pull requests

2 participants