[PATCH v2 0/2] MR8920: cmd: Fix some TYPE command behavior (one more try).
Giving this one final shot. If this is not deemed acceptable, I will abandon the effort and just move on to other things, no hard feelings. This is IMO a cleaner, more conservative/less invasive change. What is fixed: - Bug #56381, "TYPE c:\windows\winhelp.exe >foo", i.e. binary mode operation. I would probably consider this the main reason for this change. I'm trying to get the compiler mentioned in the bug report working. - Ctrl-Z termination of TYPE output to the console. - "TYPE con >foo", with Ctrl-Z handling, functionally equivalent to "COPY con foo". -- v2: cmd/tests: Add tests to check for TYPE truncation in binary mode. cmd: Fix some TYPE command behavior. https://gitlab.winehq.org/wine/wine/-/merge_requests/8920
From: Joe Souza <jsouza(a)yahoo.com> --- programs/cmd/builtins.c | 102 +++++++++++++++++++++++----------------- 1 file changed, 59 insertions(+), 43 deletions(-) diff --git a/programs/cmd/builtins.c b/programs/cmd/builtins.c index dd9808aa9f2..ff835386560 100644 --- a/programs/cmd/builtins.c +++ b/programs/cmd/builtins.c @@ -540,6 +540,49 @@ end: return ret; } +/**************************************************************************** + * WCMD_copy_loop + * + * Copies from a file + * optionally reading only until EOF (ascii copy) + * Returns TRUE on success + */ +static BOOL WCMD_copy_loop(HANDLE in, HANDLE out, BOOL ascii) +{ + BOOL ok; + DWORD bytesread, byteswritten; + char *eof = NULL; + + /* Loop copying data from source to destination until EOF read */ + do + { + char buffer[MAXSTRING]; + + ok = ReadFile(in, buffer, MAXSTRING, &bytesread, NULL); + if (ok) { + + /* Stop at first EOF */ + if (ascii) { + eof = (char *)memchr((void *)buffer, '\x1a', bytesread); + if (eof) bytesread = (eof - buffer); + } + + if (bytesread) { + ok = WriteFile(out, buffer, bytesread, &byteswritten, NULL); + if (!ok || byteswritten != bytesread) { + WINE_ERR("Unexpected failure writing, rc=%ld\n", + GetLastError()); + } + } + } else { + WINE_ERR("Unexpected failure reading, rc=%ld\n", + GetLastError()); + } + } while (ok && bytesread > 0 && !eof); + + return ok; +} + /**************************************************************************** * WCMD_ManualCopy * @@ -552,7 +595,6 @@ static BOOL WCMD_ManualCopy(WCHAR *srcname, WCHAR *dstname, BOOL ascii, BOOL app { HANDLE in,out; BOOL ok; - DWORD bytesread, byteswritten; WINE_TRACE("Manual Copying %s to %s (ascii: %u) (append: %u)\n", wine_dbgstr_w(srcname), wine_dbgstr_w(dstname), ascii, append); @@ -578,32 +620,7 @@ static BOOL WCMD_ManualCopy(WCHAR *srcname, WCHAR *dstname, BOOL ascii, BOOL app SetFilePointer(out, 0, NULL, FILE_END); } - /* Loop copying data from source to destination until EOF read */ - do - { - char buffer[MAXSTRING]; - - ok = ReadFile(in, buffer, MAXSTRING, &bytesread, NULL); - if (ok) { - - /* Stop at first EOF */ - if (ascii) { - char *ptr = (char *)memchr((void *)buffer, '\x1a', bytesread); - if (ptr) bytesread = (ptr - buffer); - } - - if (bytesread) { - ok = WriteFile(out, buffer, bytesread, &byteswritten, NULL); - if (!ok || byteswritten != bytesread) { - WINE_ERR("Unexpected failure writing to %s, rc=%ld\n", - wine_dbgstr_w(dstname), GetLastError()); - } - } - } else { - WINE_ERR("Unexpected failure reading from %s, rc=%ld\n", - wine_dbgstr_w(srcname), GetLastError()); - } - } while (ok && bytesread > 0); + ok = WCMD_copy_loop(in, out, ascii); CloseHandle(out); CloseHandle(in); @@ -3416,31 +3433,30 @@ RETURN_CODE WCMD_type(WCHAR *args) while (argN) { WCHAR *thisArg = WCMD_parameter (args, argno++, &argN, FALSE, FALSE); - HANDLE h; - WCHAR buffer[512]; - DWORD count; + HANDLE hIn, hOut; + DWORD console_mode; if (!argN) break; WINE_TRACE("type: Processing arg '%s'\n", wine_dbgstr_w(thisArg)); - h = CreateFileW(thisArg, GENERIC_READ, FILE_SHARE_READ|FILE_SHARE_WRITE|FILE_SHARE_DELETE, NULL, OPEN_EXISTING, - FILE_ATTRIBUTE_NORMAL, NULL); - if (h == INVALID_HANDLE_VALUE) { + hIn = CreateFileW(thisArg, GENERIC_READ, FILE_SHARE_READ|FILE_SHARE_WRITE|FILE_SHARE_DELETE, NULL, OPEN_EXISTING, + FILE_ATTRIBUTE_NORMAL, NULL); + if (hIn == INVALID_HANDLE_VALUE) { WCMD_print_error (); WCMD_output_stderr(WCMD_LoadMessage(WCMD_READFAIL), thisArg); return errorlevel = ERROR_INVALID_FUNCTION; - } else { - if (writeHeaders) { - WCMD_output_stderr(L"\n%1\n\n\n", thisArg); - } - while (WCMD_ReadFile(h, buffer, ARRAY_SIZE(buffer) - 1, &count)) { - if (count == 0) break; /* ReadFile reports success on EOF! */ - buffer[count] = 0; - WCMD_output_asis (buffer); - } - CloseHandle (h); } + hOut = GetStdHandle(STD_OUTPUT_HANDLE); + + if (writeHeaders) { + WCMD_output_stderr(L"\n%1\n\n\n", thisArg); + } + + WCMD_copy_loop(hIn, hOut, GetConsoleMode(hIn, &console_mode) || GetConsoleMode(hOut, &console_mode)); + + CloseHandle (hIn); } + return errorlevel = return_code; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8920
From: Joe Souza <jsouza(a)yahoo.com> --- programs/cmd/tests/batch.c | 24 ++++++++++++++++++++++++ programs/cmd/tests/test_builtins.cmd | 12 ++++++++++++ programs/cmd/tests/test_builtins.cmd.exp | 4 ++++ 3 files changed, 40 insertions(+) diff --git a/programs/cmd/tests/batch.c b/programs/cmd/tests/batch.c index 03a05f34215..8f3d4daf22b 100644 --- a/programs/cmd/tests/batch.c +++ b/programs/cmd/tests/batch.c @@ -531,6 +531,28 @@ void create_nul_test_file(void) CloseHandle(file); } +void create_binary_test_file(void) +{ + HANDLE file; + DWORD size; + BOOL bres; + unsigned char contents[256]; + + for (int x=0; x<sizeof(contents); x++) { + contents[x] = x; + } + + file = CreateFileA("binary_test_file", GENERIC_WRITE, 0, NULL, CREATE_ALWAYS, + FILE_ATTRIBUTE_NORMAL, NULL); + ok(file != INVALID_HANDLE_VALUE, "CreateFile failed\n"); + if(file == INVALID_HANDLE_VALUE) + return; + + bres = WriteFile(file, contents, ARRAYSIZE(contents), &size, NULL); + ok(bres, "Could not write to file: %lu\n", GetLastError()); + CloseHandle(file); +} + START_TEST(batch) { int argc; @@ -556,6 +578,7 @@ START_TEST(batch) shortpath_len = GetShortPathNameA(path, shortpath, ARRAY_SIZE(shortpath)); create_nul_test_file(); + create_binary_test_file(); argc = winetest_get_mainargs(&argv); if(argc > 2) @@ -564,4 +587,5 @@ START_TEST(batch) EnumResourceNamesA(NULL, "TESTCMD", test_enum_proc, 0); DeleteFileA("nul_test_file"); + DeleteFileA("binary_test_file"); } diff --git a/programs/cmd/tests/test_builtins.cmd b/programs/cmd/tests/test_builtins.cmd index 9f144bb4e0a..fd9f6bd6152 100644 --- a/programs/cmd/tests/test_builtins.cmd +++ b/programs/cmd/tests/test_builtins.cmd @@ -1405,6 +1405,14 @@ echo ---6 type foobaw echo ---7 del foobaz foobay foobax foobaw +echo ---8 +type c:\windows\winhelp.exe >foo +call :CompareFileSizes c:\windows\winhelp.exe foo +del foo +echo ---9 +type binary_test_file >foo +call :CompareFileSizes binary_test_file foo +del foo echo ------------ Testing NUL ------------ md foobar & cd foobar @@ -3513,6 +3521,10 @@ shift if not "%1"=="" goto :CheckFileSize goto :eof +:CompareFileSizes +if "%~z1"=="%~z2" (echo passed) else (echo failed) +goto :eof + :testcopy rem ----------------------- diff --git a/programs/cmd/tests/test_builtins.cmd.exp b/programs/cmd/tests/test_builtins.cmd.exp index 6ec30eba3a7..f9bec6813d0 100644 --- a/programs/cmd/tests/test_builtins.cmd.exp +++ b/programs/cmd/tests/test_builtins.cmd.exp @@ -994,6 +994,10 @@ foobay ---7 +---8 +passed +---9 +passed ------------ Testing NUL ------------ bar bar -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8920
On Mon Sep 8 16:37:48 2025 +0000, eric pouech wrote:
it seems to me that this will continue to read after the ctrl-z (it just drops the bytes in buffer after ctrl-z, but doesn't stop reading next buffer) this looks like a bug in existing code, but worth fixing (potentially in a separate patch) Fixed.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8920#note_115335
On Mon Sep 8 16:35:03 2025 +0000, Joe Souza wrote:
changed this line in [version 2 of the diff](/wine/wine/-/merge_requests/8920/diffs?diff_id=207116&start_sha=87ce16e884e2e29304d7621a6032ac3ba289766d#5da6ff77f01131f15f0584e3da4bfbcdb6790735_3464_3455) Done.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8920#note_115336
On Mon Sep 8 16:35:03 2025 +0000, Joe Souza wrote:
changed this line in [version 2 of the diff](/wine/wine/-/merge_requests/8920/diffs?diff_id=207116&start_sha=87ce16e884e2e29304d7621a6032ac3ba289766d#5da6ff77f01131f15f0584e3da4bfbcdb6790735_3462_3455) Done.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8920#note_115337
On Mon Sep 8 16:35:03 2025 +0000, Joe Souza wrote:
changed this line in [version 2 of the diff](/wine/wine/-/merge_requests/8920/diffs?diff_id=207116&start_sha=87ce16e884e2e29304d7621a6032ac3ba289766d#5da6ff77f01131f15f0584e3da4bfbcdb6790735_3451_3450) Done.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8920#note_115338
sorry looks I forgot to post following bits alongside previous comments --- (general note: you'd better update the previous MR rather than closing/creating a new one as we lose some context) for the tests: * IMO adding a test dependency to winhelp.exe (even from system dir) could cause trouble on some configurations (and it takes for granted that it always contains a ctrl-z character) * it's already a pain to swing back and forth between the two files for input / expected output, so I'd rather not add another test file creation into batch.c * unfortunately, trying to create the test file inside test_builtin.cmd using the @\x1b@ sequence doesn't work (it looks like native just erases ctrl-z from the input in that case) * so I'd ended up with [test-type.patch](/uploads/bc7b0de8295a24a1242c043c36043387/test-type.patch) instead * (and we should also get rid at some point of create_test_nul_file() in batch.c and move test file creation in test_builtins.cmd) * and adding tests should be first patch in MR (so it's clearer in subsequent patches what has been fixed by removing the todo_wine) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8920#note_115429
participants (3)
-
eric pouech (＠epo) -
Joe Souza
-
Joe Souza (＠JoeS209)