dokan-dev / dokany

User mode file system library for windows with FUSE Wrapper
http://dokan-dev.github.io
5.15k stars 657 forks source link

How to return error to userland close? #1165

Open ylgybbz opened 11 months ago

ylgybbz commented 11 months ago

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

Check List

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: https://github.com/dokan-dev/dokany/issues/1157#issuecomment-1606750737. 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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 💪