Upgrade to Dokan 2.0.4+fix

[KoltesDigital]

Proposal to upgrade, please review and give feedback but do not accept it yet, it relies on a fix in the dokany repository which hasn't been released yet. It may lead to dokany 2.0.5 or 2.1.0.

Now DokanInit and DokanShutdown must be called accordingly. I've tried to that with a RAII pattern, i.e. an empty struct with call the former in the constructor and the latter in Drop impl, but I found it too cumbersome: the struct had to be Send etc. Besides users can instantiate it twice: to prevent that, an extra runtime cost would be required. I finally went simply for two functions which have to be called when users know best.

I've added a config file to ensure formatting rules are the same for all contributors. I've done that in separated commits to keep the upgrade apart.

[Liryna]

I am going to release 2.0.5, @KoltesDigital would you be interested to maintain the rust wrapper ? It should not require much time unless there is an API breakage which rarely happened in the past.

[KoltesDigital]

@Liryna Hi sorry for the delay. Agreed to be maintainer, hopefully you'll be right 😄

BTW, I'd like your opinion on the way this binding "tags" the upstream Dokan version.

Currently, semver's build metadata is used to inform: version = "0.2.0+dokan150". My understanding of build metadata is that they are inserted by CICD processes for test purposes, but is not meant to be exposed.

I've read that some crates use an old lib version by default, and opt in new versions through features. This makes sense for backward-compatible changes, but it won't help upgrading Dokan from 1.5 to 2.0. Besides, these considerations apply for Linux where it's common to have lib*.so globally available, while this project targets Windows and it's common for each app to embed their dependency DLLs. So maybe it should be made clear that every app should embed its Dokan DLL.

This may be specific to Rust, but it seems no other Rust maintainers are available, and maybe you know enough of it. But I'm also having this question with other frameworks (e.g. creating a Chocolatey package for an app), and it seems the correct way does depend on the framework (e.g. Chocolatey packages should use a forth segment in the version id). Well if you've ever thought about it, I'd be glad to hear.

[Liryna]

Thanks @KoltesDigital for accepting the role! 🥳 Welcome to the family.
I sent you an invitation to join dokan-rust group.

Regarding the versioning I would propose to have the native dokan library version used (so that users can directly understand for which dokan library it works with) + a version number that gives the wrapper the capability to apply fixes.
This is exactly what your chocolatey is recommending. Whether it should be a sequential number or a date at the end like 1.2.0.20120627, I do not have strong opinion. I have experienced that a date would limit you to one release per day 😄 which can be annoying if you face it.

[KoltesDigital]

Joined! Thanks!

The problem with the forth segment is it's not in the semver specs, that's an addition of NuGet. Pretty sure most language package managers won't accept it. What I ended up doing is using the third segment, as it should be fine to update to any patch release. But in this case where there is a Rusty wrapper on top of the binding, the wrapper may change in a non-backward compatible way with the same DLL.

This comment suggests semver can't handle usage complexity by design. So after all, maybe the best way is to log a message like Your app must link to Dokan's DLL version ^2.0.5. during the build process. That would ensure that developers embeds the proper DLL in their apps. Again this may be Rust specific, not sure if other languages can print things during compilation. And such a message can be annoying...

Well, that was future-oriented. For now I'll just push this PR 😉

[Liryna]

Sounds good to me! Thanks again for helping the community!

[chyyran]

Any updates on this since July?

[KoltesDigital]

Ha yes sorry I'll push tomorrow. BTW while you're here, as it seems you're interested in that feature, feel free to review the code ☺️

[KoltesDigital]

I'm still on it, I overlooked unit tests. Sorry for the couple of force-pushes, and the still broken main branch. Some changes are needed in the high-level wrapper lib, and I think it's okay to do them now as there also will be changes due to moving from Dokan 1 to 2. I'll do that in a further commit, put the reasons and move instructions in a changelog. Briefly, the main reason is that the current high-level lib creates the FS with the options being on the stack, and depending whether the function is inlined, the process may segfault. This issue is already present, but the stars were so aligned that it was never caught by the tests or example apps.

@Liryna I saw that g_DebugMode and g_UseStdErr are assigned in DokanCreateFileSystem, in addition to DokanDebugMode and DokanUseStdErr. It means that if a user writes DokanDebugMode(TRUE); DokanCreateFileSystem(... debug disabled ...); debug will be disabled, or with DokanCreateFileSystem(... FS #1, debug enabled ...); DokanCreateFileSystem(... FS #2, debug disabled ...); debug will be disabled for both FS. I think this is misleading. Is there a reason behind that, or is it just historical?

The high-level lib also states that DokanUseStdErr(FALSE) set the debug output to stdout, but looking at the C lib, it never uses stdout but rather calls OutputDebugStringA, which itself never uses stdout as well but forwards the debugger / DbgView. Can you please confirm?

[Liryna]

@KoltesDigital This is really only historical and yes you are correct, the free functions and the create file system functions will affect the same global variables. DokanDebugMode and DokanUseStdErr should probably be removed to avoid this confusion. They should have been kept private for internal usage by dokanctl and other dokan binaries.

it never uses stdout but rather calls OutputDebugStringA

It does stdout! Here: https://github.com/dokan-dev/dokany/blob/2352816c996537e90a5786b84219b708f6cfeb02/dokan/dokanc.h#L64

FYI, there is dokan-dev/dokany#851 to Allow custom logger set during DokanInit and would be a good opportunity to remove the global variables.
People have expressed this to be improved but not strong enough to make it happen or contribute.

[KoltesDigital]

Also, DokanGetMountPointList returns NULL if there is an error, or if the list is empty, which doesn't feel like an error to me. Has that never bothered anyone? Say one wants to make some manager widget, and there's no mount point yet, would the GUI report an error?

If ever you consider fixing that while staying backward compatible as much as possible, I'm thinking about a nasty hack: return a non-null value and set *nbRead to 0. This would mean no errors occurred, but there are no elements in the list neither. Thus the non-null value shall never be read. I can open a PR if you're okay with that.

[KoltesDigital]

I'm doing a lot of NodeJS stuff and most libraries allow to pass a custom function to receive log messages, which I find a good thing, so I'm rather for the idea of function pointer. I can try a PR too. However, this would mean the two bitflags would be removed -> breaking change -> major release. Or they can be marked as deprecated to avoid the breaking change.

The link you provided indicates stderr, not stdout ;) Anyway, not important given the above discussion.

[Liryna]

I'm thinking about a nasty hack: return a non-null value and set *nbRead to 0

Good idea! Created dokan-dev/dokany#1109 and will take of it.

the two bitflags would be removed -> breaking change -> major release

For the library this is fine. It would just be a 2.1.x. Happy to review a pull request if you have time!

[Liryna]

The link you provided indicates stderr, not stdout ;) Anyway, not important given the above discussion.

Sorry https://github.com/dokan-dev/dokany/blob/2352816c996537e90a5786b84219b708f6cfeb02/dokan/dokanc.h#L66 OutputDebugStringA is not the stderr indeed but the debug output that is "often" used on Windows which can be read with DebugView

[KoltesDigital]

Great!

All tests are passing now, except two, and I'd like to have your opinion on them.

The first one concerns FindFiles. As you can see here, the expected file list with the mocked FS is ., .., and a constant. However, the actual list is in the reverse order. I guess the test expectation should be updated, I haven't found any doc that states that . and .. must be yielded first. Can you confirm that you somehow flipped the order between versions 1.5 and 2.0?

The second failing test concerns FindStreams. The mocked FS calls DokanFillFindStreamData with some constant (whose StreamSize is 42) , but it seems this function returns 0. I think the build process compiles the C project in Release even if Rust is in Debug, because the debugger doesn't display all variables. Thankfully, at least resultBufferSize's value is shown: 32. However, if I compute correctly, sizeof(FILE_STREAM_INFORMATION) seems to be 42. resultBufferSize is lower, and the function exits with FALSE as expected. I haven't much info there. I don't know if StreamSize must always be set to 42 to match the sizeof, or if it's just a random number for the test. And is it possible to make the buffer larger?

[Liryna]

Can you confirm that you somehow flipped the order between versions 1.5 and 2.0?

Indeed the memory container type was changed and the order of how they are inserted with. It should not have any impact so I would suggest to just adapt the tests.

For FindStreams, I am not sure to follow, DokanFillFindStreamData has to be called with the WIN32_FIND_STREAM_DATA as first param and the findstream context that you get as FindStreams param.
Could you point at some code ? See https://github.com/dokan-dev/dokany/blob/a04b410e692c08a1143c901e8265751c89a70819/samples/dokan_memfs/memfs_operations.cpp#L761 if it helps

[KoltesDigital]

Ok, one test left!

The test is here, which triggers this handler. fill_find_stream_data is an adaptation of fill_find_stream_data with arguments WIN32_FIND_STREAM_DATA , find_stream_context through this wrapper, whereby fill_data is actually DokanFillFindStreamData. This call should therefore return TRUE, but actually returns FALSE as explained. Stepping into the debugger, this condition seems to be true. At this point, resultBufferSize seems to be 32, is it expected?

[Liryna]

Oh yes this is expected. It is the system that sends us the buffer with this limited size. In that case the test should return STATUS_BUFFER_OVERFLOW to inform the system we require a larger buffer https://github.com/dokan-dev/dokany/blob/a04b410e692c08a1143c901e8265751c89a70819/samples/dokan_memfs/memfs_operations.cpp#L762 The system will then send a new request with a larger buffer.

[KoltesDigital]

Oh I see. Well this wasn't implemented, although the tests did pass... Anyway, they all pass again! I'll clean and push very soon. Thanks for your help!

[KoltesDigital]

One more question related to the C lib: is there a reason why you hold a whole VolumeSecurityDescriptor and not just a pointer to it? It looks cumbersome: you also have to manage its length, memcpy_s and so on.

[Liryna]

That was introduced a while ago. I don't recall if there were any specific reason it is like that. Maybe it is one way to limit the size of the descriptor which will be copied to the struct sent to the driver during mount. Same way as the MountPoint.

[KoltesDigital]

Yay the CICD is fine now. Upgraded to the latest 2.0.6 as well!

Although some runs failed, apparently because of transient errors. The debug messages are output to find out if they occur again. I force-pushed a couple of times, more or less purposefully, but it hasn't happened yet.

I've added changelogs with the same formatting as the main repo. Since this is a monorepo of two crates, I'll also start to tag releases with the crate as prefix: dokan@v*.*.* and dokan-sys@v*.*.*.

@Liryna sadly the publishing of crates is not part of the CICD. Are you owner of the crates on crates.io, and if so, would you rather publish them yourself or invite me as publisher?

[Liryna]

I have no access unfortunately :( @DDoSolitary could it be possible to give access to myself and @KoltesDigital ?

[DDoSolitary]

@KoltesDigital added
@Liryna crates.io says user not found, probably you need to sign up first

[Liryna]

@DDoSolitary Indeed I had not account, it should work now. Thanks!

[KoltesDigital]

@DDoSolitary thanks! While you're here, please consider reviewing the code before I publish it. Because you use the lib for your own projects, you'll be impacted.

[DDoSolitary]

@KoltesDigital Please just go ahead. Currently I don't even have time for maintaining the project that uses yasfw.

[DDoSolitary]

@Liryna Owner invitation sent.

[Liryna]

Got it! Thanks!

[Liryna]

Thanks @KoltesDigital for the contribution!

[KoltesDigital]

Finally it's there! Sorry for the delay. I'll soon begin an app, therefore I'll double-check the changes in the API.