Open mriehm opened 5 years ago
Thanks for spotting.
Work here is mainly to test it (need CD or floppy )
Triage: Dan's change was the correct one, we just need to test this manually with a card reader, floppy, cd, etc.
We need to make sure the driver is loaded by accessing media on the device, then remove the media, and run a program that enumerates the files. A prompt should not show up with the fix (but will with the current code).
I spotted this too today, almost 4 years later. https://github.com/dotnet/runtime/blob/5d1b7e77e054f74de05d6cd34de11c55ffbd125f/src/libraries/System.Private.CoreLib/src/System/IO/Enumeration/FileSystemEnumerator.Windows.cs#L46
It should probably be
using (DisableMediaInsertionPrompt.Create())
default
/ new
does not call the wrapped Interop.Kernel32.SetThreadErrorMode
@phizch any interest in offering a PR? And Dispose can assert that it was properly created.
@danmoseley, I'm on it. I haven't contributed to dotnet/runtime
before, so I hope I'll do it right.
I assume you meant changing DisableMediaInsertionPrompt.Dispose
to
public void Dispose()
{
Debug.Assert( _disableSuccess );
if ( _disableSuccess )
Interop.Kernel32.SetThreadErrorMode( _oldMode, out _ );
}
Would it be a good idea to check the return value of SetThreadErrorMode in the Dispose method too? e.g.
public void Dispose()
{
Debug.Assert( _disableSuccess );
if ( _disableSuccess )
{
var res = Interop.Kernel32.SetThreadErrorMode( _oldMode, out _ );
Debug.Assert( res );
}
}
I'm not sure if there's any test cases that covers this, but I've implemented the changes in a project of my own, and it didn't brick my PC. I'm not sure how to use my updated repository as a runtime/sdk for a test project. I did a test with a USB SD-card reader without a card, but I got the same result with both the old code and the new:
System.IO.IOException: 'The device is not ready. : 'K:\''
I expected to get a Windows message like this with the old code: (This is by trying to open the drive in Explorer)
I assume you meant changing
DisableMediaInsertionPrompt.Dispose
to
Most likely @danmoseley meant adding a debug asser that ensures that we are not dealing with a default DisableMediaInsertionPrompt
. Since DisableMediaInsertionPrompt
does not implement IEquatable<DisableMediaInsertionPrompt>
you can just verify its all field values:
Debug.Assert(_disableSuccess != default && _oldMode != default, "Use DisableMediaInsertionPrompt.Create");
I'm not sure how to use my updated repository as a runtime/sdk for a test project.
The easiest way would be to add a temporary test to one of our tests projects and just run it:
.\build.cmd -rc Release -subset clr+libs+libs.tests
.\dotnet.cmd build /t:Test .\src\libraries\System.IO.FileSystem\tests\System.IO.FileSystem.Tests.csproj /p:Configuration=Debug
@phizch please let me know if you still want to work on the PR
Unless I'm very much confused, it appears that the FileSystemEnumerator constructor uses DisableMediaInsertionPrompt incorrectly such that it will do nothing because it calls 'new DisableMediaInsertionPrompt()' instead of DisableMediaInsertionPrompt.Create().
I just happened to notice this while scrolling through the code. I don't know exactly in which situations the undesirable media prompt would occur and don't have a particular need for this to be fixed.