[PATCH 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. I have attempted to leave much of the existing code in place and mainly refactor and make smaller changes. As such, some issues are fixed/improved, but not everything. 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. However, "TYPE c:\windows\winhelp.exe" (no redirected output) is still broken (output truncated), exactly as before. . Ctrl-Z termination of TYPE output to the console. . "TYPE con >foo", with Ctrl-Z handling, functionally equivalent to "COPY con foo". All other behavior should remain the same/unchanged. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8920
From: Joe Souza <jsouza(a)yahoo.com> --- programs/cmd/builtins.c | 113 +++++++++++++++++++++++++++------------- 1 file changed, 76 insertions(+), 37 deletions(-) diff --git a/programs/cmd/builtins.c b/programs/cmd/builtins.c index dd9808aa9f2..84cef7614da 100644 --- a/programs/cmd/builtins.c +++ b/programs/cmd/builtins.c @@ -540,6 +540,48 @@ 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; + + /* 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, rc=%ld\n", + GetLastError()); + } + } + } else { + WINE_ERR("Unexpected failure reading, rc=%ld\n", + GetLastError()); + } + } while (ok && bytesread > 0); + + return ok; +} + /**************************************************************************** * WCMD_ManualCopy * @@ -552,7 +594,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 +619,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,30 +3432,53 @@ RETURN_CODE WCMD_type(WCHAR *args) while (argN) { WCHAR *thisArg = WCMD_parameter (args, argno++, &argN, FALSE, FALSE); - HANDLE h; + HANDLE hIn, hOut; WCHAR buffer[512]; DWORD count; + WCHAR *eofchar; 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)) { + } + hOut = GetStdHandle(STD_OUTPUT_HANDLE); + if (hOut == INVALID_HANDLE_VALUE) { + WCMD_print_error (); + WCMD_output_stderr(WCMD_LoadMessage(WCMD_READFAIL), thisArg); + CloseHandle(hIn); + return errorlevel = ERROR_INVALID_FUNCTION; + } + + if (writeHeaders) { + WCMD_output_stderr(L"\n%1\n\n\n", thisArg); + } + + if (GetFileType(hIn) == FILE_TYPE_CHAR || GetFileType(hOut) == FILE_TYPE_CHAR) { + /* text mode */ + while (WCMD_ReadFile(hIn, buffer, ARRAY_SIZE(buffer) - 1, &count)) { if (count == 0) break; /* ReadFile reports success on EOF! */ buffer[count] = 0; + eofchar = wcschr(buffer, L'\x1A'); + if (eofchar) { + *eofchar = L'\0'; + } WCMD_output_asis (buffer); + if (eofchar) { + break; + } } - CloseHandle (h); + } else { + /* binary mode */ + WCMD_copy_loop(hIn, hOut, FALSE); } + + 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
eric pouech (@epo) commented about programs/cmd/builtins.c:
+ } + hOut = GetStdHandle(STD_OUTPUT_HANDLE); + if (hOut == INVALID_HANDLE_VALUE) { + WCMD_print_error (); + WCMD_output_stderr(WCMD_LoadMessage(WCMD_READFAIL), thisArg); + CloseHandle(hIn); + return errorlevel = ERROR_INVALID_FUNCTION; + } + + if (writeHeaders) { + WCMD_output_stderr(L"\n%1\n\n\n", thisArg); + } + + if (GetFileType(hIn) == FILE_TYPE_CHAR || GetFileType(hOut) == FILE_TYPE_CHAR) { + /* text mode */ + while (WCMD_ReadFile(hIn, buffer, ARRAY_SIZE(buffer) - 1, &count)) { shouldn't we just do a ManualCopy(..., TRUE) here instead?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8920#note_115184
eric pouech (@epo) commented about programs/cmd/builtins.c:
+ } + + 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); + + return ok; 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)
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8920#note_115183
eric pouech (@epo) commented about programs/cmd/builtins.c:
- 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)) { + } + hOut = GetStdHandle(STD_OUTPUT_HANDLE); + if (hOut == INVALID_HANDLE_VALUE) { that block about hOut isn't needed (validation of output file when redirecting has already been done)
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8920#note_115186
eric pouech (@epo) commented about programs/cmd/builtins.c:
- } - while (WCMD_ReadFile(h, buffer, ARRAY_SIZE(buffer) - 1, &count)) { + } + hOut = GetStdHandle(STD_OUTPUT_HANDLE); + if (hOut == INVALID_HANDLE_VALUE) { + WCMD_print_error (); + WCMD_output_stderr(WCMD_LoadMessage(WCMD_READFAIL), thisArg); + CloseHandle(hIn); + return errorlevel = ERROR_INVALID_FUNCTION; + } + + if (writeHeaders) { + WCMD_output_stderr(L"\n%1\n\n\n", thisArg); + } + + if (GetFileType(hIn) == FILE_TYPE_CHAR || GetFileType(hOut) == FILE_TYPE_CHAR) { not sure about that test: you want ascii copy is either input or output is a console (we can leave other devices like COM or PRN out for now) otherwise binary so to me, this boils down to something like: `copy_loop(hin, hout, GetConsoleMode(hin, &dummy) || GetConsoleMode(hout, &dummy))`
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8920#note_115185
participants (3)
-
eric pouech (＠epo) -
Joe Souza
-
Joe Souza (＠JoeS209)