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

Excel file(xlsx) save failed in a drive mounted by default mirror sample #611

Closed
Jaozhang opened this issue Oct 24, 2017 · 19 comments
Closed

Comments

@Jaozhang
Copy link

Environment

  • Windows version: Win 10 16299(Only)
  • Processor architecture: 64
  • Dokany version: 1.0.5
  • Office version: office 2016
  • Library type (Dokany/FUSE): Dokany
  • Tested program: default mirror sample in install path

Description

When we edit&save an excel file(xlsx) in a mounted drive, the file will be saved failed.

Reproduce Step

  1. Download & Install Dokany 1.0.5
  2. Mount a drive M(mirror.exe /r C:\Users /l m)
  3. Access drive M and copy A.xlsx to M
  4. Open & Edit A.xlsx in M
  5. Save A.xlsx(CTRL + S)
  6. Excel alert says"Because of sharing violation, Microsfte Excel can not saved to file"M:\F5D42000")

Addition

  1. I can reproduce this issue always
  2. I can save xlsx file on other drive(c:\ or d:) successfully.
  3. "M:\F5D42000" is a temp file created when Excel saving.
  4. Any docx file could not be saved too(similar phenomenon)
  5. Win 10 16299 is a build released by Microsoft recently, seems it changed a lot.
  6. I can only reproduce this issue on win 10 16299.
  7. On win 10 16299, Dokany 2.0.0 beta can't mount a drive M or can't access drive M after mounted, so i can't test more.
@pthurau
Copy link

pthurau commented Oct 31, 2017

I've exactly the same problem, using Windows 10 1709 build 16299 aka Fall Creators Update and Dokan 1.0.5.1000

@Jaozhang
Copy link
Author

Jaozhang commented Nov 2, 2017

I debugged and tested, this issue caused by FileRenameInformationEx(This value is available starting with Windows 10, version 1709).dokany does not handle FileRenameInformationEx
https://msdn.microsoft.com/en-us/library/windows/hardware/ff728840(v=vs.85).aspx

And you can also check FILE_RENAME_INFORMATION, it's different with FILE_LINK_INFORMATION and DOKAN_RENAME_INFORMATION, but the code assumes they are same.
#if (_WIN32_WINNT >= _WIN32_WINNT_WIN10_RS1)
union {
BOOLEAN ReplaceIfExists; // FileRenameInformation
ULONG Flags; // FileRenameInformationEx
} DUMMYUNIONNAME;
#else
BOOLEAN ReplaceIfExists;
#endif
HANDLE RootDirectory;
ULONG FileNameLength;
WCHAR FileName[1];
} FILE_RENAME_INFORMATION, *PFILE_RENAME_INFORMATION;

@Liryna
Copy link
Member

Liryna commented Nov 9, 2017

Hi @Jaozhang ,

Thank you for the report !
So you really see in the log the call of FileRenameInformationEx ? There is no fallback in FileRenameInformation just after we return STATUS_NOT_IMPLEMENTED for FileRenameInformationEx ?
Would be amazing that they added a feature mandatory 😢

@Jaozhang
Copy link
Author

Jaozhang commented Nov 10, 2017

Hi @Liryna
Thank you for the reply !

Background:

  1. I debugged in dokany(1.0.5) + myTestApp.exe(had the same issue with default sample before)
  2. I installed WDK&SDK 1709(16299)
  3. I set Target Platform Version: 10.0.16299.0 in sys Property Pages(vs2017)
  4. I add FileRenameInformationEx Processing logic in DokanDispatchSetInformation and
    DokanCompleteSetInformation
  5. I built sys and tested in myTestApp.exe(it works fine now)

Answer your question:

Q : So you really see in the log the call of FileRenameInformationEx ?
A : Yes, after modified sys, I see call of FileRenameInformationEx in mayApp.exe(I'm not familiar with debugging dokany), of course we can be sure that it also called in dokany.

Q : There is no fallback in FileRenameInformation just after we return STATUS_NOT_IMPLEMENTED for FileRenameInformationEx ?
A : There is always no fallback in FileRenameInformation.

@Liryna
Copy link
Member

Liryna commented Nov 10, 2017

Oh that's awesome ! Thanks for the informations

Does the implementation also solved the excel file save issue ?

@Jaozhang
Copy link
Author

@Liryna
Yes, the implementation solved the excel(also word, MS Photo) file save issue.

@Liryna
Copy link
Member

Liryna commented Nov 10, 2017

@Jaozhang Would you like to contribute to dokan with a pull request with your changes ?

@freeplant
Copy link

@Liryna We have exactly the same issue (https://github.com/haiwen/seadrive-gui/issues/37). It would be nice if a fixed version available in this month.

@Liryna
Copy link
Member

Liryna commented Nov 12, 2017

I started to migrate the solution / sdk / wdk / installer to VS 2017 10.0.16299.0 to be able to fix this issue.

Currently keeping the migration in my repository: Liryna@41c4468

@Liryna Liryna mentioned this issue Nov 12, 2017
4 tasks
@Jaozhang
Copy link
Author

@Liryna
Sorry for the late reply.

I am glad to contribute to dokan, but i couldn't.
As a beginner, i can't even understand the details of dokan code,
there may be a lot of side effect in my test code.
sorry.😢

By the way, It seems that you have already working on it, thank you!

@Liryna
Copy link
Member

Liryna commented Nov 13, 2017

@Jaozhang even not a PR to review what you did 😢 ? Would be helpful and I am really looking to have contributors especially when they come with the issue and fix.
I would take a look and fix if needed on the top.

@Jaozhang
Copy link
Author

@Liryna
I openned a PR, i hope it would be helpful.
sorry for the late PR.

@webaxys
Copy link

webaxys commented Nov 16, 2017

hello,
is there a workaround ? a specific Microsoft KB to remove to get it working back for example?
thanks a lot.

@Liryna
Copy link
Member

Liryna commented Nov 18, 2017

Hello @webaxys h

Unfortunately the workaround would be to remove the windows creators update... 😢

I have tested and merged the implementation of the new FileRenameInformationEx coming with Windows 10 update RS1 (v1709) of @Jaozhang 7add59e
They simply replaced the FileRenameInformation ReplaceIfExists bool by a Flags that can hold different options:

#if (_WIN32_WINNT >= _WIN32_WINNT_WIN10_RS1)
#define FILE_RENAME_REPLACE_IF_EXISTS                  0x00000001
#define FILE_RENAME_POSIX_SEMANTICS                    0x00000002
#endif

#if (_WIN32_WINNT >= _WIN32_WINNT_WIN10_RS3)
#define FILE_RENAME_SUPPRESS_PIN_STATE_INHERITANCE     0x00000004
#endif

Some reading about it here

The dokan keep using ReplaceIfExists bool for now so only compatible with FILE_RENAME_REPLACE_IF_EXISTS, I will open another issue to see how we can implement the other options (change the dokan API) later.

We will need to release this fix under dokan version 1.1.0. The fix needs to be released in the Kernel and Library. Just updating the Kernel or the Library would keep the issue to exist. The struct size does not change so the kernel <-> library communication is not broken (no need to update to 2.0.0).

@Rondom We do not need to move to VS 2017 and new WDK for now but I think we can make the move when appveyor will be ready appveyor/ci#1554

I start to look to implement the pending TODO for 1.1.0

@Liryna Liryna added this to the 1.1.0 milestone Nov 18, 2017
@Liryna
Copy link
Member

Liryna commented Nov 19, 2017

Appveyor is read, we can do the move to VS 2017 for 1.1.0 #622

@Liryna
Copy link
Member

Liryna commented Nov 28, 2017

1.1.0 has been release https://github.com/dokan-dev/dokany/releases/tag/v1.1.0 🏆

If someone can confirme the fix is working 👍

@0x4a616e
Copy link

Seems to be working perfectly. Great job, thx!

@freeplant
Copy link

The fix is working. Thanks.

@Liryna
Copy link
Member

Liryna commented Dec 1, 2017

Great news !
Thanks a lot all of you for reporting the issue and your feedback !
Also a big thanks to @Jaozhang for finding the reason and providing the fix ! 🏆

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

6 participants