<filesystem> Treat ERROR_BAD_NETPATH as file not found. #616

BillyONeal · 2020-03-17T19:21:21Z

Extract _Is_file_not_found to <xfilesystem_abi.h> as __std_is_file_not_found because we also need that in filesystem.cpp.
Add ERROR_BAD_NETPATH to __std_is_file_not_found.
Map ERROR_BAD_NETPATH to errc::no_such_file_or_directory.
Change filesystem tests that look for file not exists behavior to also test bad network paths.

Checklist

Be sure you've read README.md and understand the scope of this repo.

If you're unsure about a box, leave it unchecked. A maintainer will help you.

Identifiers in product code changes are properly _Ugly as per
https://eel.is/c++draft/lex.name#3.1 or there are no product code changes.
The STL builds successfully and all tests have passed (must be manually
verified by an STL maintainer before automated testing is enabled on GitHub,
leave this unchecked for initial submission).
These changes introduce no known ABI breaks (adding members, renaming
members, adding virtual functions, changing whether a type is an aggregate
or trivially copyable, etc.).
These changes were written from scratch using only this repository,
the C++ Working Draft (including any cited standards), other WG21 papers
(excluding reference implementations outside of proposed standard wording),
and LWG issues as reference material. If they were derived from a project
that's already listed in NOTICE.txt, that's fine, but please mention it.
If they were derived from any other project (including Boost and libc++,
which are not yet listed in NOTICE.txt), you must mention it here,
so we can determine whether the license is compatible and what else needs
to be done.

Resolves microsoftGH-615 / DevCom-950424. * Extract _Is_file_not_found to <xfilesystem_abi.h> as __std_is_file_not_found because we also need that in filesystem.cpp. * Add ERROR_BAD_NETPATH to __std_is_file_not_found. * Map ERROR_BAD_NETPATH to errc::no_such_file_or_directory. * Change filesystem tests that look for file not exists behavior to also test bad network paths.

tests/std/tests/P0218R1_filesystem/test.cpp

StephanTLavavej · 2020-03-17T20:19:06Z

stl/inc/xfilesystem_abi.h

+    case __std_win_error::_File_not_found:
+    case __std_win_error::_Path_not_found:
+    case __std_win_error::_Invalid_name:
+    case __std_win_error::_Error_bad_netpath:


These are neither lexicographically sorted nor enumerator sorted.

StephanTLavavej · 2020-03-17T20:22:44Z

stl/src/syserror.cpp

@@ -25,6 +25,7 @@ static constexpr _Win_errtab_t _Win_errtab[] = {
    {ERROR_ACCESS_DENIED, errc::permission_denied},
    {ERROR_ALREADY_EXISTS, errc::file_exists},
    {ERROR_BAD_UNIT, errc::no_such_device},
+    {ERROR_BAD_NETPATH, errc::no_such_file_or_directory},


This should be lexicographically sorted.

StephanTLavavej · 2020-03-17T20:24:26Z

stl/src/filesystem.cpp

-        switch (_Err) {
-        case __std_win_error::_File_not_found:
-        case __std_win_error::_Path_not_found:
+        if (__std_is_file_not_found(_Err)) {


Now this is additionally translating __std_win_error::_Invalid_name. That seems desirable, but is it possible to construct a test case for this?

I don't know of one. Every bogus path I was able to form is coming back with the netpath one :(. I observe the _Invalid_name one was added in https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_git/msvc/commit/4110cc61e2e22dc826c2ba517a237dc76134ab63 -- it might have just been emailed to me or something that someone saw it somewhere.

Even without test cases I don't think there's good reason for variation on what we consider not found.

StephanTLavavej · 2020-03-17T20:27:51Z

tests/std/tests/P0218R1_filesystem/test.cpp

-constexpr wstring_view badPath         = L"// ?? ?? ///// ?? ?? ? ////"sv;
-constexpr wstring_view nonexistentPath = L"C:/This/Path/Should/Not/Exist"sv;
+constexpr wstring_view badPath = L"// ?? ?? ///// ?? ?? ? ////"sv;
+path nonexistentPaths[]        = {


Why is this global array modifiable? Could it instead be constexpr wstring_view[] with path constructed later on demand?

It could be but then it's awkward to use / debug through because you step through path / wstring constructors constantly. I could make it const but it isn't used for anything constexpr here.

StephanTLavavej · 2020-03-17T20:30:45Z

tests/std/tests/P0218R1_filesystem/test.cpp

+        EXPECT(status(nonexistent).type() == file_type::not_found); // should not throw
+        EXPECT(status(nonexistent, ec).type() == file_type::not_found);
+        EXPECT(ec.category() == system_category());
+        EXPECT(ec.value() == 2 || ec.value() == 3 || ec.value() == 53);


Pre-existing, but bigger now: should there be a comment as to what these magic values mean?

StephanTLavavej · 2020-03-17T20:32:34Z

tests/std/tests/P0218R1_filesystem/test.cpp

    }

    // test VSO-654638 where create_directory(p, existing_p) was doing copy_symlink behavior
    {
        error_code ec;
-        create_directory_symlink(nonexistentPath, p, ec);
+        create_directory_symlink(nonexistentPaths[0], p, ec);


Is it critical that nonexistentPaths[0] be a local nonexistent path? If so, should that be commented above?

Nope, it just needed to be any nonexistent path. Do you want a separate variable like "anyGivenNonexistentPath"? (I want to avoid reusing nonexistentPath when there's already a variable nonexistentPaths)

No, I think it's fine.

BillyONeal requested a review from a team as a code owner March 17, 2020 19:21

BillyONeal self-assigned this Mar 17, 2020

clang-format

602fd41

CaseyCarter approved these changes Mar 17, 2020

View reviewed changes

tests/std/tests/P0218R1_filesystem/test.cpp Outdated Show resolved Hide resolved

tests/std/tests/P0218R1_filesystem/test.cpp Outdated Show resolved Hide resolved

tests/std/tests/P0218R1_filesystem/test.cpp Outdated Show resolved Hide resolved

StephanTLavavej added the bug Something isn't working label Mar 17, 2020

StephanTLavavej requested changes Mar 17, 2020

View reviewed changes

Sorting, const and comments.

d0f8b7e

BillyONeal requested a review from StephanTLavavej March 17, 2020 21:51

StephanTLavavej approved these changes Mar 17, 2020

View reviewed changes

BillyONeal merged commit bf944d2 into microsoft:master Mar 18, 2020

BillyONeal deleted the filenotfound branch March 18, 2020 01:29

StephanTLavavej mentioned this pull request May 6, 2020

Fix filesystem's handling of nonexistent network paths emitting ERROR_INVALID_NAME #800

Merged

StephanTLavavej mentioned this pull request Jul 1, 2022

syserror.cpp: Assert that _Win_errtab is sorted and unique #2837

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

<filesystem> Treat ERROR_BAD_NETPATH as file not found. #616

<filesystem> Treat ERROR_BAD_NETPATH as file not found. #616

BillyONeal commented Mar 17, 2020

StephanTLavavej Mar 17, 2020

StephanTLavavej Mar 17, 2020

StephanTLavavej Mar 17, 2020

BillyONeal Mar 17, 2020

StephanTLavavej Mar 17, 2020

BillyONeal Mar 17, 2020

StephanTLavavej Mar 17, 2020

StephanTLavavej Mar 17, 2020

BillyONeal Mar 17, 2020

StephanTLavavej Mar 17, 2020

<filesystem> Treat ERROR_BAD_NETPATH as file not found. #616

<filesystem> Treat ERROR_BAD_NETPATH as file not found. #616

Conversation

BillyONeal commented Mar 17, 2020

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment