[PATCH v6 0/4] MR6904: ntdll.dll: Update NtQueryDirectoryFile to align with current Windows behaviour
This PR updates the behaviour of `NtQueryDirectoryFile`, bringing it in line with current Windows behaviour. The need for this update was discovered when attempting to build the Unreal Engine with MSVC under Wine. In certain cases conditional include statements do not behave as expected, due to MSVC depending on undocumented behaviour of `NtQueryDirectoryFile`. We ran tests on multiple versions of Windows, and discovered that the behaviour has changed since the original Wine implementation, but the documentation has not. The source code for our test tool, and a set of results can be found [here](https://github.com/TensorWorks/NtQueryDirectoryFile-Test). As of Windows 8, calling `NtQueryDirectoryFile` with a re-used handle, a new mask, and setting the `RestartScan` flag to True, causes the cached results to be erased and a new scan to be performed with the updated mask. Currently, Wine performs as did earlier versions of Windows, where the changed mask is ignored, and the cache is reused. This can cause `NtQueryDirectoryFile` under Wine to falsely report that files exist, when they do not. This PR corrects this behaviour, invalidating the cache when required. Implementing this exposed further undocumented behaviour of `NtQueryDirectoryFile`, where a search for a non-existent file will return either `STATUS_NO_MORE_FILES` or `STATUS_NO_SUCH_FILE`, depending on whether or not the handle had been previously used regardless of the value of `RestartScan`. This was reflected in a `winetest` which allowed for the response to be either `STATUS_SUCCESS` or `STATUS_NO_MORE_FILES`. This test has been updated to only allow current Windows behaviour, and `NtQueryDirectoryFile` will return either `STATUS_NO_MORE_FILES` or `STATUS_NO_SUCH_FILE` as appropriate. This patch also adds unit tests for the new behaviour of `NtQueryDirectoryFile`. These tests pass when running `winetest` under Windows, and under Wine with these changes in place, but they will fail under older versions of Wine. -- v6: ntdll: Simplify check that unexpected files were not found https://gitlab.winehq.org/wine/wine/-/merge_requests/6904
From: Eugene McArdle <eugene(a)tensorworks.com.au> --- dlls/ntdll/unix/file.c | 55 ++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 53 insertions(+), 2 deletions(-) diff --git a/dlls/ntdll/unix/file.c b/dlls/ntdll/unix/file.c index 13dc36b42ff..3baa4aa611d 100644 --- a/dlls/ntdll/unix/file.c +++ b/dlls/ntdll/unix/file.c @@ -225,6 +225,7 @@ struct dir_data struct file_identity id; /* directory file identity */ struct dir_data_names *names; /* directory file names */ struct dir_data_buffer *buffer; /* head of data buffers list */ + UNICODE_STRING mask; /* the mask used when creating the cache entry */ }; static const unsigned int dir_data_buffer_initial_size = 4096; @@ -565,6 +566,7 @@ static void free_dir_data( struct dir_data *data ) free( buffer ); } free( data->names ); + if (data->mask.Buffer) free( data->mask.Buffer ); free( data ); } @@ -2606,6 +2608,14 @@ static NTSTATUS init_cached_dir_data( struct dir_data **data_ret, int fd, const return status; } + /* if a mask was specified then copy it into the cache entry */ + if (mask && mask->Length) + { + data->mask.Length = data->mask.MaximumLength = mask->Length; + if (!(data->mask.Buffer = calloc( 1, mask->Length ))) return STATUS_NO_MEMORY; + memcpy(data->mask.Buffer, mask->Buffer, mask->Length); + } + /* sort filenames, but not "." and ".." */ i = 0; if (i < data->count && !strcmp( data->names[i].unix_name, "." )) i++; @@ -2628,17 +2638,35 @@ static NTSTATUS init_cached_dir_data( struct dir_data **data_ret, int fd, const } +/*********************************************************************** + * ustring_equal + * + * Simplified version of RtlEqualUnicodeString that performs only case-sensitive comparisons. + */ +static BOOLEAN ustring_equal( const UNICODE_STRING *a, const UNICODE_STRING *b ) +{ + USHORT length_a = (a ? a->Length : 0); + USHORT length_b = (b ? b->Length : 0); + if (length_a != length_b) return FALSE; + + if (length_a == 0 && length_b == 0) return TRUE; + return (!memcmp(a->Buffer, b->Buffer, a->Length)); +} + + /*********************************************************************** * get_cached_dir_data * * Retrieve the cached directory data, or initialize it if necessary. */ static unsigned int get_cached_dir_data( HANDLE handle, struct dir_data **data_ret, int fd, - const UNICODE_STRING *mask ) + const UNICODE_STRING *mask, BOOLEAN restart_scan, + BOOLEAN *fresh_handle ) { unsigned int i; int entry = -1, free_entries[16]; unsigned int status; + *fresh_handle = TRUE; SERVER_START_REQ( get_directory_cache_entry ) { @@ -2675,6 +2703,21 @@ static unsigned int get_cached_dir_data( HANDLE handle, struct dir_data **data_r dir_data_cache_size = size; } + /* If we have an existing cache entry then set the flag to indicate that the handle is not fresh */ + if (dir_data_cache[entry]) *fresh_handle = FALSE; + + /* If we have an existing cache entry, but restart_scan is true and a new non-empty mask + was specified then we need to invalidate the existing cache entry and create a new one + */ + if (dir_data_cache[entry] && restart_scan && mask && + !ustring_equal(&dir_data_cache[entry]->mask, mask)) + { + TRACE( "invalidating existing cache entry for handle %p, old mask: \"%s\", new mask: \"%s\"\n", + handle, debugstr_us(&(dir_data_cache[entry]->mask)), debugstr_us(mask)); + free_dir_data( dir_data_cache[entry] ); + dir_data_cache[entry] = NULL; + } + if (!dir_data_cache[entry]) status = init_cached_dir_data( &dir_data_cache[entry], fd, mask ); *data_ret = dir_data_cache[entry]; @@ -2694,6 +2737,7 @@ NTSTATUS WINAPI NtQueryDirectoryFile( HANDLE handle, HANDLE event, PIO_APC_ROUTI enum server_fd_type type; struct dir_data *data; unsigned int status; + BOOLEAN fresh_handle = TRUE; TRACE("(%p %p %p %p %p %p 0x%08x 0x%08x 0x%08x %s 0x%08x\n", handle, event, apc_routine, apc_context, io, buffer, @@ -2746,7 +2790,7 @@ NTSTATUS WINAPI NtQueryDirectoryFile( HANDLE handle, HANDLE event, PIO_APC_ROUTI cwd = open( ".", O_RDONLY ); if (fchdir( fd ) != -1) { - if (!(status = get_cached_dir_data( handle, &data, fd, mask ))) + if (!(status = get_cached_dir_data( handle, &data, fd, mask, restart_scan, &fresh_handle ))) { union file_directory_info *last_info = NULL; @@ -2768,6 +2812,13 @@ NTSTATUS WINAPI NtQueryDirectoryFile( HANDLE handle, HANDLE event, PIO_APC_ROUTI } else status = errno_to_status( errno ); + /* Return STATUS_NO_MORE_FILES if a mask did not match a file and the handle had previously been used */ + if (status == STATUS_NO_SUCH_FILE && !fresh_handle) + { + status = STATUS_NO_MORE_FILES; + io->Status = status; + } + mutex_unlock( &dir_mutex ); if (needs_close) close( fd ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6904
From: Eugene McArdle <eugene(a)tensorworks.com.au> --- dlls/ntdll/tests/directory.c | 164 +++++++++++++++++++++++++++++++++++ 1 file changed, 164 insertions(+) diff --git a/dlls/ntdll/tests/directory.c b/dlls/ntdll/tests/directory.c index 52ff9936560..27027d41d9d 100644 --- a/dlls/ntdll/tests/directory.c +++ b/dlls/ntdll/tests/directory.c @@ -964,6 +964,169 @@ done: pRtlFreeUnicodeString(&ntdirname); } +static void set_up_mask_test(const char *testdir) +{ + BOOL ret; + char buf[MAX_PATH + 5]; + HANDLE h; + + ret = CreateDirectoryA(testdir, NULL); + ok( ret, "couldn't create dir '%s', error %ld\n", testdir, GetLastError() ); + + sprintf(buf, "%s\\%s", testdir, "a-file.h"); + h = CreateFileA(buf, GENERIC_READ|GENERIC_WRITE, 0, NULL, CREATE_ALWAYS, + FILE_ATTRIBUTE_NORMAL, 0); + ok( h != INVALID_HANDLE_VALUE, "failed to create temp file '%s'\n", buf ); + CloseHandle(h); + + sprintf(buf, "%s\\%s", testdir, "another-file.h"); + h = CreateFileA(buf, GENERIC_READ|GENERIC_WRITE, 0, NULL, CREATE_ALWAYS, + FILE_ATTRIBUTE_NORMAL, 0); + ok( h != INVALID_HANDLE_VALUE, "failed to create temp file '%s'\n", buf ); + CloseHandle(h); +} + +static void tear_down_mask_test(const char *testdir) +{ + int ret; + char buf[MAX_PATH]; + + sprintf(buf, "%s\\%s", testdir, "a-file.h"); + ret = DeleteFileA(buf); + ok( ret || (GetLastError() == ERROR_FILE_NOT_FOUND) || (GetLastError() == ERROR_PATH_NOT_FOUND), + "Failed to rm %s, error %ld\n", buf, GetLastError() ); + + sprintf(buf, "%s\\%s", testdir, "another-file.h"); + ret = DeleteFileA(buf); + ok( ret || (GetLastError() == ERROR_FILE_NOT_FOUND) || (GetLastError() == ERROR_PATH_NOT_FOUND), + "Failed to rm %s, error %ld\n", buf, GetLastError() ); + + RemoveDirectoryA(testdir); +} + + +/* Performs a query with NtQueryDirectoryFile() */ +static BOOL test_NtQueryDirectoryFile_mask(HANDLE handle, BOOL restart_scan, UNICODE_STRING mask, + BOOL expect_success, BOOL validate_only) +{ + NTSTATUS status; + IO_STATUS_BLOCK io; + UINT data_size = sizeof(FILE_DIRECTORY_INFORMATION) + (MAX_PATH * sizeof(wchar_t)); + BYTE data[8192]; + FILE_DIRECTORY_INFORMATION *dir_info; + WCHAR *name; + ULONG name_len; + ULONG mask_len = mask.Length / sizeof(WCHAR); + + /* Perform the query */ + status = pNtQueryDirectoryFile( handle, NULL, NULL, NULL, &io, data, data_size, + FileDirectoryInformation, TRUE, &mask, restart_scan ); + if (expect_success) + { + if (validate_only && status != STATUS_SUCCESS) return FALSE; + + ok( status == STATUS_SUCCESS, "could not find file mask: '%s', restart: %d, error %ld\n", + wine_dbgstr_wn(mask.Buffer, mask_len), restart_scan, GetLastError() ); + /* Print the query parameters and the result */ + if (status == 0) { + dir_info = (FILE_DIRECTORY_INFORMATION *)data; + name = dir_info->FileName; + name_len = dir_info->FileNameLength / sizeof(WCHAR); + + ok( name_len == mask_len, "unexpected filename length %lu, expected %lu\n", name_len, mask_len ); + ok( !memcmp(name, mask.Buffer, mask_len), "unexpected filename %s, expected %s\n", + wine_dbgstr_wn(name, name_len), wine_dbgstr_wn(mask.Buffer, mask_len) ); + } + } + else + { + if (validate_only && status == STATUS_SUCCESS) return FALSE; + /* Print the query parameters and the result */ + if (status == 0) { + dir_info = (FILE_DIRECTORY_INFORMATION *)data; + name = dir_info->FileName; + name_len = dir_info->FileNameLength / sizeof(WCHAR); + + ok ( status != STATUS_SUCCESS, "unexpectedly found file. mask: '%s', found %s\n", + wine_dbgstr_wn(mask.Buffer, mask_len), wine_dbgstr_wn(name, name_len) ); + } + } + return TRUE; +} + +static void test_NtQueryDirectoryFile_change_mask(void) +{ + NTSTATUS status; + HANDLE dirh; + char testdir[MAX_PATH]; + OBJECT_ATTRIBUTES attr; + IO_STATUS_BLOCK io; + UNICODE_STRING ntdirname; + WCHAR testdir_w[MAX_PATH]; + + UNICODE_STRING atestfile; + UNICODE_STRING anothertestfile; + UNICODE_STRING notatestfile; + + BOOL run_updated_tests = TRUE; + + pRtlInitUnicodeString(&atestfile, L"a-file.h"); + pRtlInitUnicodeString(&anothertestfile, L"another-file.h"); + pRtlInitUnicodeString(¬atestfile, L"not-a-file.h"); + + /* Clean up from prior aborted run, if any, then set up test files */ + ok( GetTempPathA(MAX_PATH, testdir), "couldn't get temp dir\n" ); + strcat(testdir, "mask.tmp"); + tear_down_mask_test(testdir); + set_up_mask_test(testdir); + + pRtlMultiByteToUnicodeN(testdir_w, sizeof(testdir_w), NULL, testdir, strlen(testdir) + 1); + if (!pRtlDosPathNameToNtPathName_U(testdir_w, &ntdirname, NULL, NULL)) + { + ok( 0, "RtlDosPathNametoNtPathName_U failed\n" ); + goto done; + } + InitializeObjectAttributes(&attr, &ntdirname, OBJ_CASE_INSENSITIVE, 0, NULL); + + /* Open a handle for our test directory */ + status = pNtOpenFile(&dirh, SYNCHRONIZE | FILE_LIST_DIRECTORY, &attr, &io, FILE_SHARE_READ, + FILE_SYNCHRONOUS_IO_NONALERT | FILE_OPEN_FOR_BACKUP_INTENT | FILE_DIRECTORY_FILE); + ok( status == STATUS_SUCCESS, "failed to open dir '%s', ret 0x%lx, error %ld\n", testdir, status, GetLastError() ); + if (status != STATUS_SUCCESS) + { + skip("can't test if we can't open the directory\n"); + goto done; + } + + /* Verify that updated windows 8 and higher behaviour is supported */ + if (!winetest_platform_is_wine && !test_NtQueryDirectoryFile_mask(dirh, TRUE, atestfile, TRUE, TRUE)) + run_updated_tests = FALSE; + if (!winetest_platform_is_wine && !test_NtQueryDirectoryFile_mask(dirh, TRUE, notatestfile, FALSE, TRUE)) + run_updated_tests = FALSE; + + if (!run_updated_tests) + { + skip("Win8+ NtQueryDirectoryMask behaviour not supported"); + goto done; + } + + /* Run tests - all searches for `notatestfile` are expected to fail */ + test_NtQueryDirectoryFile_mask(dirh, TRUE, atestfile, TRUE, FALSE); + test_NtQueryDirectoryFile_mask(dirh, TRUE, anothertestfile, TRUE, FALSE); + test_NtQueryDirectoryFile_mask(dirh, TRUE, notatestfile, FALSE, FALSE); + + test_NtQueryDirectoryFile_mask(dirh, TRUE, atestfile, TRUE, FALSE); + test_NtQueryDirectoryFile_mask(dirh, FALSE, notatestfile, FALSE, FALSE); + + test_NtQueryDirectoryFile_mask(dirh, TRUE, atestfile, TRUE, FALSE); + /* Since we are not resetting the scan, and are using an incompatible mask, this should also fail */ + test_NtQueryDirectoryFile_mask(dirh, FALSE, anothertestfile, FALSE, FALSE); + + /* Cleanup */ +done: + tear_down_mask_test(testdir); +} + static NTSTATUS get_file_id( FILE_INTERNAL_INFORMATION *info, const WCHAR *root, const WCHAR *name ) { OBJECT_ATTRIBUTES attr; @@ -1157,5 +1320,6 @@ START_TEST(directory) test_directory_sort( sysdir ); test_NtQueryDirectoryFile(); test_NtQueryDirectoryFile_case(); + test_NtQueryDirectoryFile_change_mask(); test_redirection(); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6904
From: Eugene McArdle <eugene(a)tensorworks.com.au> --- dlls/ntdll/tests/directory.c | 41 +++++++++++++++++++++++++----------- 1 file changed, 29 insertions(+), 12 deletions(-) diff --git a/dlls/ntdll/tests/directory.c b/dlls/ntdll/tests/directory.c index 27027d41d9d..b07654ee1ff 100644 --- a/dlls/ntdll/tests/directory.c +++ b/dlls/ntdll/tests/directory.c @@ -1025,8 +1025,8 @@ static BOOL test_NtQueryDirectoryFile_mask(HANDLE handle, BOOL restart_scan, UNI { if (validate_only && status != STATUS_SUCCESS) return FALSE; - ok( status == STATUS_SUCCESS, "could not find file mask: '%s', restart: %d, error %ld\n", - wine_dbgstr_wn(mask.Buffer, mask_len), restart_scan, GetLastError() ); + ok( status == STATUS_SUCCESS, "could not find file mask: '%s', restart: %d, status 0x%lx\n", + wine_dbgstr_wn(mask.Buffer, mask_len), restart_scan, status ); /* Print the query parameters and the result */ if (status == 0) { dir_info = (FILE_DIRECTORY_INFORMATION *)data; @@ -1058,6 +1058,7 @@ static void test_NtQueryDirectoryFile_change_mask(void) { NTSTATUS status; HANDLE dirh; + HANDLE dirh_alt; char testdir[MAX_PATH]; OBJECT_ATTRIBUTES attr; IO_STATUS_BLOCK io; @@ -1068,8 +1069,6 @@ static void test_NtQueryDirectoryFile_change_mask(void) UNICODE_STRING anothertestfile; UNICODE_STRING notatestfile; - BOOL run_updated_tests = TRUE; - pRtlInitUnicodeString(&atestfile, L"a-file.h"); pRtlInitUnicodeString(&anothertestfile, L"another-file.h"); pRtlInitUnicodeString(¬atestfile, L"not-a-file.h"); @@ -1098,19 +1097,34 @@ static void test_NtQueryDirectoryFile_change_mask(void) goto done; } + /* Open a second handle for our test directory */ + status = pNtOpenFile(&dirh_alt, SYNCHRONIZE | FILE_LIST_DIRECTORY, &attr, &io, FILE_SHARE_READ, + FILE_SYNCHRONOUS_IO_NONALERT | FILE_OPEN_FOR_BACKUP_INTENT | FILE_DIRECTORY_FILE); + ok( status == STATUS_SUCCESS, "failed to open second handle to dir '%s', ret 0x%lx, error %ld\n", testdir, status, GetLastError() ); + if (status != STATUS_SUCCESS) + { + skip("can't test if we can't open a second handle to the directory\n"); + goto done; + } + /* Verify that updated windows 8 and higher behaviour is supported */ - if (!winetest_platform_is_wine && !test_NtQueryDirectoryFile_mask(dirh, TRUE, atestfile, TRUE, TRUE)) - run_updated_tests = FALSE; + test_NtQueryDirectoryFile_mask(dirh, TRUE, atestfile, TRUE, TRUE); if (!winetest_platform_is_wine && !test_NtQueryDirectoryFile_mask(dirh, TRUE, notatestfile, FALSE, TRUE)) - run_updated_tests = FALSE; - - if (!run_updated_tests) { - skip("Win8+ NtQueryDirectoryMask behaviour not supported"); + skip("Win8+ NtQueryDirectoryMask behaviour not supported\n"); goto done; } - /* Run tests - all searches for `notatestfile` are expected to fail */ + /* Test that caches for two handles to a single directory do not interfere with each other*/ + /* Use dirh to search for a non-existent file, putting it into a state with an empty cache */ + test_NtQueryDirectoryFile_mask(dirh, TRUE, notatestfile, FALSE, FALSE); + /* Confirm that dirh is in a state where it fails to find atestfile */ + test_NtQueryDirectoryFile_mask(dirh, FALSE, atestfile, FALSE, FALSE); + /* Confirm that dirh_alt is able to find atestfile */ + test_NtQueryDirectoryFile_mask(dirh_alt, FALSE, atestfile, TRUE, FALSE); + + /* All searches for `notatestfile` are expected to fail */ + /* Tests should also fail if the scan is not reset, and the mask changes to an incompatible one */ test_NtQueryDirectoryFile_mask(dirh, TRUE, atestfile, TRUE, FALSE); test_NtQueryDirectoryFile_mask(dirh, TRUE, anothertestfile, TRUE, FALSE); test_NtQueryDirectoryFile_mask(dirh, TRUE, notatestfile, FALSE, FALSE); @@ -1119,12 +1133,15 @@ static void test_NtQueryDirectoryFile_change_mask(void) test_NtQueryDirectoryFile_mask(dirh, FALSE, notatestfile, FALSE, FALSE); test_NtQueryDirectoryFile_mask(dirh, TRUE, atestfile, TRUE, FALSE); - /* Since we are not resetting the scan, and are using an incompatible mask, this should also fail */ test_NtQueryDirectoryFile_mask(dirh, FALSE, anothertestfile, FALSE, FALSE); /* Cleanup */ done: tear_down_mask_test(testdir); + pRtlFreeUnicodeString(&ntdirname); + pRtlFreeUnicodeString(&atestfile); + pRtlFreeUnicodeString(&anothertestfile); + pRtlFreeUnicodeString(¬atestfile); } static NTSTATUS get_file_id( FILE_INTERNAL_INFORMATION *info, const WCHAR *root, const WCHAR *name ) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6904
From: Eugene McArdle <eugene(a)tensorworks.com.au> --- dlls/ntdll/tests/directory.c | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/dlls/ntdll/tests/directory.c b/dlls/ntdll/tests/directory.c index b07654ee1ff..b98d399d7e2 100644 --- a/dlls/ntdll/tests/directory.c +++ b/dlls/ntdll/tests/directory.c @@ -1041,15 +1041,8 @@ static BOOL test_NtQueryDirectoryFile_mask(HANDLE handle, BOOL restart_scan, UNI else { if (validate_only && status == STATUS_SUCCESS) return FALSE; - /* Print the query parameters and the result */ - if (status == 0) { - dir_info = (FILE_DIRECTORY_INFORMATION *)data; - name = dir_info->FileName; - name_len = dir_info->FileNameLength / sizeof(WCHAR); - - ok ( status != STATUS_SUCCESS, "unexpectedly found file. mask: '%s', found %s\n", - wine_dbgstr_wn(mask.Buffer, mask_len), wine_dbgstr_wn(name, name_len) ); - } + + ok ( status == STATUS_NO_MORE_FILES, "unexpectedly found file\n" ); } return TRUE; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6904
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=150302 Your paranoid android. === w7pro64 (64 bit report) === Report validation errors: ntdll:directory crashed (c0000374) === w864 (64 bit report) === Report validation errors: ntdll:directory crashed (c0000374) === w1064v1507 (64 bit report) === Report validation errors: ntdll:directory crashed (c0000374) === w1064v1809 (64 bit report) === Report validation errors: ntdll:directory crashed (c0000374) === w1064_2qxl (64 bit report) === Report validation errors: ntdll:directory crashed (c0000374) === w1064_adm (64 bit report) === Report validation errors: ntdll:directory crashed (c0000374) === w1064_tsign (64 bit report) === Report validation errors: ntdll:directory crashed (c0000374) === w10pro64 (64 bit report) === Report validation errors: ntdll:directory crashed (c0000374) === w10pro64_ar (64 bit report) === Report validation errors: ntdll:directory crashed (c0000374) === w10pro64_ja (64 bit report) === Report validation errors: ntdll:directory crashed (c0000374) === w10pro64_zh_CN (64 bit report) === Report validation errors: ntdll:directory crashed (c0000374) === w11pro64_amd (64 bit report) === Report validation errors: ntdll:directory crashed (c0000374) === debian11b (64 bit WoW report) === Report validation errors: mshtml:script crashed (c0000005)
On Sun Dec 8 23:29:10 2024 +0000, Eugene McArdle wrote:
changed this line in [version 6 of the diff](/wine/wine/-/merge_requests/6904/diffs?diff_id=147819&start_sha=0f3b34247974c7dc9c885b9a33cf9302d4c8d1e9#7fa7ee56f28b04717b114bfb52e12634ff21b2e2_1045_1044) @epo are there any outstanding changes you'd like to this code? I'm happy to squash the last couple of commits together so we only have one for the behaviour changes and one for the tests, just let me know :thumbsup:
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6904#note_90168
On Wed Dec 11 18:35:45 2024 +0000, Eugene McArdle wrote: > @epo are there any outstanding changes you'd like to this code? I'm > happy to squash the last couple of commits together so we only have one > for the behaviour changes and one for the tests, just let me know :thumbsup: a couple of comments: - we tend to have the tests first (and in your case a single commit would do, so please squash the 3 commits into one), marking the failing test with todo_wine - then the fix in a second patch (removing the todo_wine for the tests that now succeed) I'm still not convinced we grasp the whole situation: - basically, your current tests show that in some cases the current scan is aborted, and its mask (and position) are changed - all the failures end up with NO_MORE_FILES, which I can't tell if this mirrors all the Windows behavior or if it's a short coming to tests themselves (eg current code returns NO_MORE_FILES when all files have been read, while NO_SUCH_FILE is returned for an empty new scan; so perhaps we should discriminate between the two using a mask with wildcard and one partial (ie not returning all the entries at once)). - also, testing a NULL mask, an empty mask could be interesting for the patch to unix/file.c itself, I think you should write it by keeping all the changes within get_dir_cached_data() (just passing the extraneous restart_scan variable) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6904#note_90288
On Wed Dec 11 18:35:45 2024 +0000, eric pouech wrote:
a couple of comments: - we tend to have the tests first (and in your case a single commit would do, so please squash the 3 commits into one), marking the failing test with todo_wine - then the fix in a second patch (removing the todo_wine for the tests that now succeed) I'm still not convinced we grasp the whole situation: - basically, your current tests show that in some cases the current scan is aborted, and its mask (and position) are changed - all the failures end up with NO_MORE_FILES, which I can't tell if this mirrors all the Windows behavior or if it's a short coming to tests themselves (eg current code returns NO_MORE_FILES when all files have been read, while NO_SUCH_FILE is returned for an empty new scan; so perhaps we should discriminate between the two using a mask with wildcard and one partial (ie not returning all the entries at once)). - also, testing a NULL mask, an empty mask could be interesting for the patch to unix/file.c itself, I think you should write it by keeping all the changes within get_dir_cached_data() (just passing the extraneous restart_scan variable) I'll add some more tests so that we can be confident that we are fully testing the behaviour, and can be confident in what current Windows behaviour is, and that these changes align with that.
Once we're happy with the test coverage I'll squash the commits together and do some git magic to apply them in the requested order. I do have some concerns in terms of keeping all changes within `get_cached_dir_data()`. We need to track the mask in the `dir_data` struct, so adding the field there is necessary, as are the changes to `free_dir_data()` and `init_cached_dir_data()`. Those changes need to stay where they are, otherwise parts of the struct will not be initialised or freed correctly. The only code that could be inlined into the `get_cached_dir_data()` function is the `ustring_equal()` function. We created this helper function specifically to keep the `get_cached_dir_data()` function more readable, are you sure that you want this moved inline instead? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6904#note_90562
participants (4)
-
eric pouech (＠epo) -
Eugene McArdle
-
Eugene McArdle (＠eugenemcardle)
-
Marvin