Fix/fuse deletedirectory

[matepek]

Fix: Deleting directory doesn't working when using Dokan FUSE library.

[Liryna]

Hi @matepek ,

In the Dokan/Windows logic, the delete should only happen in the cleanup. delete_directory should only check if the directory can be removed.
https://github.com/tresorit/dokany/blob/fix/fuse-deletedirectory/dokan_fuse/src/fusemain.cpp#L404

This is never call in your case ?

@toksaitov since you made the changes with delete directory, does is changes are needed ?

[toksaitov]

@Liryna
I did have issues with files. I am not sure about directories as my Fuse FS does not support them (I really should get some time to port the Mirror example to get some grounds for testing).

The directory code was added by analogy following these instructions:
https://github.com/dokan-dev/dokany/blob/master/dokan/dokan.h#L193
We can even see that the Dokan's version of Mirror follows that logic:
https://github.com/dokan-dev/dokany/blob/master/dokan_mirror/mirror.c#L748
The same approach for files (not directories) did fix the problem in my case.

Nevertheless, If there is a file system for which the following fix works for @matepek, I am OK with the PR.

[matepek]

Hi @Liryna and @toksaitov!

I needed some debugging to understand your answer and the intended directory-delete workflow.

I start from the beggining:
Using Dokany Fuse, I can't delete directories. As I can see know the cause is in here: https://github.com/dokan-dev/dokany/blob/master/dokan_fuse/src/fusemain.cpp#L400
The "dokan_file_info->Context" is false. It seems to me that, this is a normal/intended value for that variable in directory cases, and that line is unneccesary.
If I am correct about this, then a possible solution should be something like this:

  if (dokan_file_info->DeleteOnClose) {
    close_file(file_name, dokan_file_info);
    if (dokan_file_info->IsDirectory) {
      do_delete_directory(file_name, dokan_file_info);
    } else {
      if (dokan_file_info->Context) {
        do_delete_file(file_name, dokan_file_info);
      }
    }
  }

(Because I'm not sure about what is the requested value in file cases.)

What do you thing about it?
If you agree, I will update the pull request.

Regards,
MP

[Liryna]

Hi @matepek ,

This could be the case indeed !
But it seems that it could be a context in the case that a opendir exist
https://github.com/dokan-dev/dokany/blob/master/dokan_fuse/src/fusemain.cpp#L120
https://github.com/dokan-dev/dokany/blob/master/dokan_fuse/src/fusemain.cpp#L367

Do you have an opendir in your test ?

[matepek]

Hi @Liryna ,

You are right.
The "opendir" hadn't been hooked. :)
It is easy to fix it now to work for me.
Only one question left:
"Is it necessary for directory-deleting that, the opendir is hooked?"

Anyhow, thank You for Your help. ^^

[Liryna]

From what I see and if we want to keep the logic, we should do something like this:

  if (dokan_file_info->Context
    || (dokan_file_info->IsDirectory && !ops_.opendir)) { // No context when ops_.opendir is not set
    if (dokan_file_info->DeleteOnClose) {
      close_file(file_name, dokan_file_info);
      if (dokan_file_info->IsDirectory) {
        do_delete_directory(file_name, dokan_file_info);
      } else {
        do_delete_file(file_name, dokan_file_info);
      }
    }
  }

Do you agree with it ?

Otherwise: About fuse wrapper, what is the benefit to not implement opendir ?

[matepek]

Hi @Liryna ,

We are using Fuse on Linux and OS X. I'm porting it to Windows.
It seems we didn't needed to use opendir for libfuse and OS X Fuse, readdir was sufficient, and it's working.

On the case of a files it couldn't be the same as I can see in fusemain.cpp:
https://github.com/dokan-dev/dokany/blob/master/dokan_fuse/src/fusemain.cpp#L124
versus
https://github.com/dokan-dev/dokany/blob/master/dokan_fuse/src/fusemain.cpp#L132
So, yes, I think your snippet is Ok.

Would you like to do it or should I update the pull request? :)

Regards,
MP

[Liryna]

You can update your pull request :)
(with clean history)

[Liryna]

Thank you @matepek for your contribution and your time !

[g3gg0]

partial OT:
when moving a file with the windows explorer, the file will first get opened, then moved and released after.
if i just "tunnel through" standard open/rename/etc calls, the rename operation will fail because the file is already opened.

is this behavior subject to be changed too?

[Liryna]

@g3gg0 Hi g3gg0,

This behavior that you describe is made by the windows kernel.
We have face the same issue as you during moving file with the dokan fuse wrapper yesterday (with this fix).

The wrapper need to be debug to understand what is happening exactly. Workflow of the kernel calls on the file... Etc

[g3gg0]

hi Liryna,

yeah, faced that too and am surprised that they even do read calls on that file handle before renaming.
perhaps it would just be good enough to close() that file, rename() and then re-open() it again in the wrapper with the original r/w flags.

[Liryna]

It is not use that choose when to open/read/close files 😃 It is the kernel that ask us to do it.

To know what is happening wrong in the wrapper. We need to see what is the difference in the calls order and result between the C mirror and with the dokanfuse.

I think there is something that the dokanfuse handle wrong (for example return error where it should succeed or something like).

[g3gg0]

i wish you good luck in tracking that down.
in a world where existing files are opened using "FileCreate" i wouldnt be surprised if you also have to create a file before renaming it :D

thanks for looking into it :)

[Liryna]

I don't really have time for it right now 😄 so it gonna take times

I created an issue related to it: #178