[PATCH 0/1] MR7322: programs/cmd: Implement ability to abort lengthy directory operations via Ctrl-C.
Allow the user to press Ctrl-C to abort lengthy DIR (or DIR /p, etc.) operations. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7322
From: Joe Souza <jsouza(a)yahoo.com> --- programs/cmd/directory.c | 13 ++++++++----- programs/cmd/wcmd.h | 2 ++ programs/cmd/wcmdmain.c | 20 ++++++++++++++++++++ 3 files changed, 30 insertions(+), 5 deletions(-) diff --git a/programs/cmd/directory.c b/programs/cmd/directory.c index 3a2a5955d12..6c3538484f6 100644 --- a/programs/cmd/directory.c +++ b/programs/cmd/directory.c @@ -256,6 +256,7 @@ static DIRECTORY_STACK *WCMD_list_directory (DIRECTORY_STACK *inputparms, int le DIRECTORY_STACK *parms; int concurrentDirs = 0; BOOL done_header = FALSE; + BOOL operation_aborted = FALSE; dir_count = 0; file_count = 0; @@ -333,7 +334,7 @@ static DIRECTORY_STACK *WCMD_list_directory (DIRECTORY_STACK *inputparms, int le } WINE_TRACE("cols=%d, rows=%d\n", numCols, numRows); - for (rows=0; rows<numRows; rows++) { + for (rows=0; rows<numRows && !operation_aborted; rows++) { BOOL addNewLine = TRUE; for (cols=0; cols<numCols; cols++) { WCHAR username[24]; @@ -428,9 +429,11 @@ static DIRECTORY_STACK *WCMD_list_directory (DIRECTORY_STACK *inputparms, int le } if (addNewLine) WCMD_output_asis(L"\r\n"); cur_width = 0; + + operation_aborted = WCMD_is_command_aborted(); } - if (!bare) { + if (!bare && !operation_aborted) { if (file_count == 1) { WCMD_output (L" 1 file %1!25s! bytes\n", WCMD_filesize64 (byte_count.QuadPart)); } @@ -442,7 +445,7 @@ static DIRECTORY_STACK *WCMD_list_directory (DIRECTORY_STACK *inputparms, int le file_total = file_total + file_count; dir_total = dir_total + dir_count; - if (!bare && !recurse) { + if (!bare && !recurse && !operation_aborted) { if (dir_count == 1) { WCMD_output (L"%1!8d! directory ", 1); } else { @@ -453,7 +456,7 @@ static DIRECTORY_STACK *WCMD_list_directory (DIRECTORY_STACK *inputparms, int le free(fd); /* When recursing, look in all subdirectories for matches */ - if (recurse) { + if (recurse && !operation_aborted) { DIRECTORY_STACK *dirStack = NULL; DIRECTORY_STACK *lastEntry = NULL; WIN32_FIND_DATAW finddata; @@ -535,7 +538,7 @@ static void WCMD_dir_trailer(const WCHAR *path) { WINE_TRACE("Writing trailer for '%s' gave %d(%ld)\n", wine_dbgstr_w(path), status, GetLastError()); - if (errorlevel == NO_ERROR && !bare) { + if (errorlevel == NO_ERROR && !bare && !WCMD_is_command_aborted()) { if (recurse) { WCMD_output (L"\n Total files listed:\n%1!8d! files%2!25s! bytes\n", file_total, WCMD_filesize64 (byte_total)); WCMD_output (L"%1!8d! directories %2!18s! bytes free\n\n", dir_total, WCMD_filesize64 (freebytes.QuadPart)); diff --git a/programs/cmd/wcmd.h b/programs/cmd/wcmd.h index caacd44995d..2e6d1eeffdc 100644 --- a/programs/cmd/wcmd.h +++ b/programs/cmd/wcmd.h @@ -224,6 +224,8 @@ RETURN_CODE WCMD_run_builtin_command(int cmd_index, WCHAR *cmd); BOOL WCMD_find_label(HANDLE h, const WCHAR*, LARGE_INTEGER *pos); void WCMD_set_label_end(WCHAR *string); +BOOL WCMD_is_command_aborted(void); + void *xrealloc(void *, size_t) __WINE_ALLOC_SIZE(2) __WINE_DEALLOC(free); static inline void *xalloc(size_t sz) __WINE_MALLOC; diff --git a/programs/cmd/wcmdmain.c b/programs/cmd/wcmdmain.c index 223bb55d2c6..fe8e3ddcff0 100644 --- a/programs/cmd/wcmdmain.c +++ b/programs/cmd/wcmdmain.c @@ -57,6 +57,8 @@ static int max_height; static int max_width; static int numChars; +static HANDLE control_c_event = NULL; + #define MAX_WRITECONSOLE_SIZE 65535 /* @@ -246,6 +248,8 @@ static void WCMD_output_asis_handle (DWORD std_handle, const WCHAR *message) { const WCHAR* ptr; WCHAR string[1024]; HANDLE handle = GetStdHandle(std_handle); + //FIXME: (see note below) + //HANDLE wait_handles[] = {GetStdHandle(STD_INPUT_HANDLE), control_c_event}; if (paged_mode) { do { @@ -260,6 +264,10 @@ static void WCMD_output_asis_handle (DWORD std_handle, const WCHAR *message) { if (++line_count >= max_height - 1) { line_count = 0; WCMD_output_asis_len(pagedMessage, lstrlenW(pagedMessage), handle); + //FIXME: This WaitForMultipleObjects call should be used instead of the WCMD_ReadFile call, + // but apparently the standard input handle is always signalled in Wine. + //WaitForMultipleObjects(ARRAY_SIZE(wait_handles), wait_handles, FALSE, INFINITE); + //WCMD_output_asis(L"\r\n"); WCMD_ReadFile(GetStdHandle(STD_INPUT_HANDLE), string, ARRAY_SIZE(string), &count); } } while (((message = ptr) != NULL) && (*ptr)); @@ -3848,9 +3856,18 @@ RETURN_CODE node_execute(CMD_NODE *node) return return_code; } + +BOOL WCMD_is_command_aborted(void) +{ + return (WAIT_OBJECT_0 == WaitForSingleObject(control_c_event, 0)); +} + static BOOL WINAPI my_event_handler(DWORD ctrl) { WCMD_output(L"\n"); + if (ctrl == CTRL_C_EVENT) { + SetEvent(control_c_event); + } return ctrl == CTRL_C_EVENT; } @@ -4107,6 +4124,7 @@ int __cdecl wmain (int argc, WCHAR *argvW[]) } else { + control_c_event = CreateEventW(NULL, TRUE, FALSE, NULL); SetConsoleCtrlHandler(my_event_handler, TRUE); } @@ -4234,10 +4252,12 @@ int __cdecl wmain (int argc, WCHAR *argvW[]) { if (rpl_status == RPL_SUCCESS && toExecute) { + ResetEvent(control_c_event); node_execute(toExecute); node_dispose_tree(toExecute); if (echo_mode) WCMD_output_asis(L"\r\n"); } } + CloseHandle(control_c_event); return 0; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7322
eric pouech (@epo) commented about programs/cmd/wcmdmain.c:
static int max_width; static int numChars;
+static HANDLE control_c_event = NULL; the NULL initialization isn't needed
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7322#note_94437
eric pouech (@epo) commented about programs/cmd/wcmdmain.c:
const WCHAR* ptr; WCHAR string[1024]; HANDLE handle = GetStdHandle(std_handle); + //FIXME: (see note below) we don't allow C++ comments in Wine and this comment looks like left over from iteration of creating the patch
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7322#note_94438
eric pouech (@epo) commented about programs/cmd/wcmdmain.c:
return return_code; }
+ +BOOL WCMD_is_command_aborted(void) +{ + return (WAIT_OBJECT_0 == WaitForSingleObject(control_c_event, 0)); +} + static BOOL WINAPI my_event_handler(DWORD ctrl) { WCMD_output(L"\n"); + if (ctrl == CTRL_C_EVENT) {
please keep the surrounding style (open curly bracket should be on next line) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7322#note_94439
Hi, a couple of remarks (this isn't a full review yet) the ctrl-handler + event look fine to me (I don't think native does process the ctrl-c by reading the input stream as 0x03) but as the event is a one-time operation, it should be up to the caller to handle and propagate the ctrl-c information I don't like the variable added to the DIR helpers to propagate it as I think this should be integrated as another value in the return code management instead a couple of elements to reach that conclusion: * a small test using `(dir /s \*.* &&echo SUCCESS !errorlevel!||echo FAILURE !errorlevel!)` and breaking it with ctrl-c before it ends shows a `FAILURE 1` indicating that the event is propagated (at some point) through the return code which in turn sets the `ERRORLEVEL` * there are traces on Internet that the ctrl-c event is (at some point) propagated as what looks like an internal return code \[1\]; `-1073741510 = 0xC000013A = STATUS_CONTROL_C_EXIT` * and lastly, it could be (didn't test it though) that if an error occurs while using `WCMD_output `(eg disk full) that error should be propagated as well * this would also fit well into the various handling of ctrl-c while calling into other bat/cmd files (interruping the subcommand when in a CALL) so this rather calls for: * `RETURN_CODE WCMD_ctrlc_status(void)` : return `NO_ERROR` if event isn't set, `STATUS_CONTROL_C_EXIT` if set * then rework the DIR loops as being controlled by a `RETURN_CODE` (and keep looping while it's `NO_ERROR`) \-Eric \[1\] https://www.dostips.com/forum/viewtopic.php?t=5859 -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7322#note_94443
On Thu Feb 13 10:34:11 2025 +0000, eric pouech wrote:
the NULL initialization isn't needed In practice I *always* initialize global variables. I can remove it.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7322#note_94466
On Thu Feb 13 10:35:10 2025 +0000, eric pouech wrote:
we don't allow C++ comments in Wine and this comment looks like left over from iteration of creating the patch I left it there purposely with the intent that the code would be restored when/if the signalling of the standard input handle is fixed. But I'll remove it.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7322#note_94467
On Thu Feb 13 10:36:05 2025 +0000, eric pouech wrote:
please keep the surrounding style (open curly bracket should be on next line) Will do.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7322#note_94468
On Thu Feb 13 11:09:41 2025 +0000, eric pouech wrote:
Hi, a couple of remarks (this isn't a full review yet) the ctrl-handler + event look fine to me (I don't think native does process the ctrl-c by reading the input stream as 0x03) but as the event is a one-time operation, it should be up to the caller to handle and propagate the ctrl-c information I don't like the variable added to the DIR helpers to propagate it as I think this should be integrated as another value in the return code management instead a couple of elements to reach that conclusion: * a small test using `(dir /s \*.* &&echo SUCCESS !errorlevel!||echo FAILURE !errorlevel!)` and breaking it with ctrl-c before it ends shows a `FAILURE 1` indicating that the event is propagated (at some point) through the return code which in turn sets the `ERRORLEVEL` * there are traces on Internet that the ctrl-c event is (at some point) propagated as what looks like an internal return code \[1\]; `-1073741510 = 0xC000013A = STATUS_CONTROL_C_EXIT` * and lastly, it could be (didn't test it though) that if an error occurs while using `WCMD_output `(eg disk full) that error should be propagated as well * this would also fit well into the various handling of ctrl-c while calling into other bat/cmd files (interruping the subcommand when in a CALL) so this rather calls for: * `RETURN_CODE WCMD_ctrlc_status(void)` : return `NO_ERROR` if event isn't set, `STATUS_CONTROL_C_EXIT` if set * then rework the DIR loops as being controlled by a `RETURN_CODE` (and keep looping while it's `NO_ERROR`) \-Eric \[1\] https://www.dostips.com/forum/viewtopic.php?t=5859 I don't understand much of this. The variable added to propagate the event is a local variable in a single function only, as an optimization to prevent calling WCMD_is_command_aborted() multiple times.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7322#note_94469
On Thu Feb 13 16:12:32 2025 +0000, Joe Souza wrote:
I don't understand much of this. The variable added to propagate the event is a local variable in a single function only, as an optimization to prevent calling WCMD_is_command_aborted() multiple times. Thinking more about this, I think what you're saying here is to remove the WCMD_is_command_aborted function and replace it with a WCMD_ctrl_c_status function, and to also propagate the proper error code up the stack. OK, will look into that.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7322#note_94470
On Thu Feb 13 16:09:05 2025 +0000, Joe Souza wrote:
In practice I *always* initialize global variables. I can remove it. I think that technically it is needed because the command processing loop calls ResetEvent regardless of whether the event was initialized earlier when/if the Control-C handler was installed. I can rework the code such that this event is always initialized, near the top of the function, instead of conditionally when the Control-C handler is installed.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7322#note_94471
On Thu Feb 13 16:38:42 2025 +0000, Joe Souza wrote:
I think that technically it is needed because the command processing loop calls ResetEvent regardless of whether the event was initialized earlier when/if the Control-C handler was installed. I can rework the code such that this event is always initialized, near the top of the function, instead of conditionally when the Control-C handler is installed. static variables are initialized to 0 by default
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7322#note_94472
On Thu Feb 13 16:30:23 2025 +0000, Joe Souza wrote:
Thinking more about this, I think what you're saying here is to remove the WCMD_is_command_aborted function and replace it with a WCMD_ctrl_c_status function, and to also propagate the proper error code up the stack. OK, will look into that. yes, that what I meant... you spelled it way clearer <g>
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7322#note_94473
participants (3)
-
eric pouech (＠epo) -
Joe Souza
-
Joe Souza (＠JoeS209)