Return STATUS_INVALID_PARAMETER where appropriate

[mrnerdhair]

Checklist

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the existing documentation
• My changes generate no new warnings
• I have updated the change log (Add/Change/Fix)
• I have cleaned up the commit history (use rebase and squash)

Changes proposed in this pull request:

• Returns STATUS_INVALID_PARAMETER rather than STATUS_NOT_IMPLEMENTED to report the use of an unimplemented information class value with an otherwise implemented IRP type. (Fixes Can't list directories in WSL2 #932.)
• Minor typo edits.
• Minor cleanup in DokanSetAllocationInformation to normalize style of code.

[Liryna]

Thanks @reidrankin for the PR and the extra changes 🚀 !!!

Changing the default return value in dokan/volume.c is fine but it would be better to directly return this status from the kernel to avoid sending the request to the userland for cases

dokany/dokan/directory.c

Lines 542 to 548 in 91e8782

	if (fileInfoClass != FileDirectoryInformation &&
	fileInfoClass != FileIdFullDirectoryInformation &&
	fileInfoClass != FileFullDirectoryInformation &&
	fileInfoClass != FileNamesInformation &&
	fileInfoClass != FileIdBothDirectoryInformation &&
	fileInfoClass != FileBothDirectoryInformation &&
	fileInfoClass != FileIdExtdBothDirectoryInformation) {

that we know that are not supported.
I know that not the case yet but some projects do not directly use the library code so fixing in the driver would also be fixing the issue for them.
Could you add all the previous list of supported infoclass in the kernel here and return the status in the default behaviour ?

dokany/sys/directory.c

Lines 122 to 145 in f7161d7

	switch (irpSp->Parameters.QueryDirectory.FileInformationClass) {
	case FileDirectoryInformation:
	DDbgPrint(" FileDirectoryInformation\n");
	break;
	case FileIdFullDirectoryInformation:
	DDbgPrint(" FileIdFullDirectoryInformation\n");
	break;
	case FileFullDirectoryInformation:
	DDbgPrint(" FileFullDirectoryInformation\n");
	break;
	case FileNamesInformation:
	DDbgPrint(" FileNamesInformation\n");
	break;
	case FileBothDirectoryInformation:
	DDbgPrint(" FileBothDirectoryInformation\n");
	break;
	case FileIdBothDirectoryInformation:
	DDbgPrint(" FileIdBothDirectoryInformation\n");
	break;
	default:
	DDbgPrint(" unknown FileInfoClass %d\n",
	irpSp->Parameters.QueryDirectory.FileInformationClass);
	break;
	}

If you could this for the other cases it would be great 👍 I know that might be too much to ask but it would definitely solve the issue.

Rename other commit to:
Library - typo fixes
Library - minor cleanup

[mrnerdhair]

I don't think it's necessarily a good idea to limit the range of info classes that the user-mode code can choose to handle. See for example #835, where a new class was implemented without touching the kernel code. So instead I went on a journey trying to figure out how I could intercept STATUS_NOT_IMPLEMENTED responses once the user-mode code had had a chance to look at the request and decided it couldn't. I thought about putting it in DokanCompleteDirectoryControl, but I don't think this is an IRP_MJ_DIRECTORY_CONTROL-specific issue; I ended up putting code in DokanCompleteIrpRequest to replace STATUS_NOT_IMPLEMENTED with STATUS_NOT_SUPPORTED instead.

My reasoning is that Dokan clients use STATUS_NOT_IMPLEMENTED to signal that they have not provided an implementation, and that a default behavior should be used instead. No matter what the requested operation or where the implementation might be provided, though, it's certainly got to happen before calling DokanCompleteIrpRequest. Therefore, we can confidently state that any function not implemented by this point is not supported by the device at all.

I used STATUS_NOT_SUPPORTED instead of STATUS_INVALID_PARAMETER in DokanCompleteIrpRequest because the only thing that can really decide whether the request failed because a parameter value was bad is the user-mode code. Once the driver hears about it, it can't really be sure if a STATUS_NOT_IMPLEMENTED result was caused because of an unsupported info class parameter or something different entirely. For this reason, I've used STATUS_NOT_SUPPORTED, because it seems like a more accurate description of what we can be sure of inside DokanCompleteIrpRequest, and because it's one of the fallback-triggering codes recognized by WSL2's Plan 9 server.

I've left the library changes that return STATUS_INVALID_PARAMETER in response to an unsupported info class in place to provide a more-specific description of these cases (which also works with WSL2).

Finally, I've added support for FileIdExtdDirectoryInformation -- I realized we had code for FileIdExtdBothDirectoryInformation, and it seemed quite odd to skip the simpler one. This, of course, is the info class that WSL2 first requests, so this will fix the problem in the first instance.

In fact, because the WSL2 Plan 9 server actually comes from the Windows Containers infrastructure, I would bet that issue #834 is caused by a similar setup that would have fallen back to other info classes if it had gotten a slightly different error code. I know that the virtual DVD drive driver doesn't support FileIdExtdDirectoryInformation, and I doubt it handles FileIdExtdBothDirectoryInformation either. I'm pretty sure those work in containers, though, which I'd expect to be because of a similar fallback mechanism to the one at issue in #932.

(Now the only question is if any of this will break anything. I'm not afraid to say that I'm not experienced enough with the way Dokan is used in the field to understand the compatibility implications here, even if I do think that the approach is theoretically sound.)

[mrnerdhair]

OK, so now that the stupid thing works, let's talk about whether this approach is a good idea or not.

(And I love the Appveyor setup's automagic Driver Verifier test suite, though I love the hour-and-a-half CI pipeline somewhat less.)

[Liryna]

Glad you like the CI :)! I also agree that it is pretty slow... Making the test running side by side would be a good start but thats a premium feature :(

Awesome that you added the support for this infoclass! Have you been able to build and test it against WSL 2?

You made a good point of not restricting the infoclass sent by the kernel and for the same reason I would prefer to not override the status sent by the library in the kernel. If other libraries want to get this fix, they will need to import it themselves and that's fine.
Therefore would you be able to just remove the last commit of your PR?

[mrnerdhair]

I've tested the AppVeyor build for 9c62a45, and the new infoclass looks like it works as intended with WSL. I'll reset and force-push this branch back to 4a5087e.

[Liryna]

Really by thanks to you @reidrankin for taking your valuable time on this! 🏆
I might have been long to deal with this PR but if later you will have other changes to propose, I will ensure that I review them more quickly 👍

... failed with the merging. Github as squashed them all by default 😢