Future changes (TODO) about dokany HOT 9 CLOSED

Hi @marinkobabic,
I am currently changing NTSTATUS as plained. (https://github.com/dokan-dev/dokany/compare/NTSTATUS)
I am facing a issue with CreateFile that it seems you faced too 😃

###Create 0000
   CreateDisposition 0x00000001
CreateFile : C:\Users\autorun.inf
  AccountName: Adrien, DomainName: WIN-9RM07VT4S97
        OPEN_EXISTING
        ShareMode = 0x7
        FILE_SHARE_READ
        FILE_SHARE_WRITE
        FILE_SHARE_DELETE
        AccessMode = 0x80
        FILE_READ_ATTRIBUTES
        FlagsAndAttributes = 0x0
        error code = 2

CreateFile status = 2
error openInfo is NULL
###GetFileInfo -001
GetFileInfo : C:\Users\autorun.inf
        invalid handle
**MIRROR CRASH**

This happened since in mirror I return the error from getLastError.
https://github.com/dokan-dev/dokany/blob/NTSTATUS/dokan_mirror/mirror.c#L220

error openInfo is NULL happen here
https://github.com/dokan-dev/dokany/blob/NTSTATUS/dokan/dokan.c#L487
because openInfo is free here
https://github.com/dokan-dev/dokany/blob/NTSTATUS/dokan/create.c#L212

By removing the free like you, I was surprised to have no crash.
marinkobabic/dokanx@eeb692a#diff-d7735925422d2f0b4a057d4f1ec7a0d5R261

It seems that dokanx have chosen to fix it in a different way...
https://github.com/BenjaminKim/dokanx/blob/develop/mirrorfs/mirrorfs.cpp#L227

By the way, the comment say that Close is never called...but I get a Cleanup and close after GetFileInfo.

Do you have a reason for removing this lines ? Are they important ?

Otherwise mirror is correctly running without problem. I will run dokan-net.Test before merging it.

from dokany.

@Liryna
The comment says "Needs to free openInfo because Close is never called." The developer expected that the close will not be executed because of the status pEventInfo->Status != STATUS_SUCCESS

The fact is, that even in this case the cleanup is executed. Therefore the pOpenInfo should not be freed during create. Cleanup requires the pOpenInfo.

Here you have the mapping table Win32 to NTSTATUS https://support.microsoft.com/en-us/kb/113996

GetLastError code should not be passed directly to the driver. GetLastError is the Win32 error and not NTSTATUS.

from dokany.

Thank you for the clear answer 👍

Do you think we should let the user return Win32 error and we convert it in NTSTATUS (dokan.dll)?

from dokany.

No. Then you will have always a discussion of mapping. Which win32 errors do you map to which code. Please add this code as well and so on...

from dokany.

Ok ! I agree 😃
I remove the free and continue the changes.

from dokany.

@marinkobabic, I finished the NTSTATUS changes. I tested it with the mirror and it seems to work good / without crash.

I would like your review before merging it 😃 and know if it fit your request ! (and If you have the time ofc)
https://github.com/dokan-dev/dokany/compare/NTSTATUS

from dokany.

@Liryna
Wow, a lot of changes. You had also to remove a lot of code 😄 Great job! 👍

• Please check your code for return 0 and replace it using the STATUS_SUCCESS.
• Also check where result == 0 is used and change the code if possible.
• In generally is the question if status > 0 should be used. Inside of the driver NT_SUCCESS and NT_ERROR is used http://rogerkar.blogspot.ch/2009/06/nterror-ntsuccess-identifier-not-found.html. Here the definition https://msdn.microsoft.com/en-us/library/windows/hardware/ff565436(v=vs.85).aspx

from dokany.

Thank @marinkobabic for your feedback and informations !
I made the changes as you suggested !

I will merge it in master when I finish to change dokan-net with NtStatus.

from dokany.

The todo has been split to different tickets.

from dokany.