[PATCH v4 0/1] MR1492: Load and save application-specific console settings from the registry
On Windows, application-specific settings are stored in the registry under HKCU\\Console\\<path_to_app>. Our implementation of conhost.exe does not know which process is connected to it, so we need to get the full process image name before loading any application-specific console settings. To do this, we extend the server protocol to pass the process Id of the connected console process to conhost.exe. Please run tools/make_requests before merging. -- v4: conhost: Load/Save console settings using the process path or window title https://gitlab.winehq.org/wine/wine/-/merge_requests/1492
From: Hugh McMaster <hugh.mcmaster(a)outlook.com> On Windows, application-specific console settings are stored in the registry under HKCU\\Console\\<identifier>, where <identifier> is either the process path or the window title. Wine currently uses the path to wineconsole.exe when loading or saving application-specific console settings, which is incorrect. To ensure we load the correct console settings, we wait until the console process has initialized before converting the process path or window title into a registry key name. --- programs/conhost/conhost.c | 4 ++- programs/conhost/conhost.h | 2 ++ programs/conhost/window.c | 51 ++++++++++++++++++++++++-------------- 3 files changed, 37 insertions(+), 20 deletions(-) diff --git a/programs/conhost/conhost.c b/programs/conhost/conhost.c index c3a106c9321..a331216f62b 100644 --- a/programs/conhost/conhost.c +++ b/programs/conhost/conhost.c @@ -2753,6 +2753,9 @@ static NTSTATUS process_console_ioctls( struct console *console ) return status; } + if (!console->init && console->win) + init_window_config( console ); + if (code == IOCTL_CONDRV_INIT_OUTPUT) { TRACE( "initializing output %x\n", output ); @@ -2947,7 +2950,6 @@ int __cdecl wmain(int argc, WCHAR *argv[]) if (!init_window( &console )) return 1; GetStartupInfoW( &si ); set_console_title( &console, si.lpTitle, wcslen( si.lpTitle ) * sizeof(WCHAR) ); - ShowWindow( console.win, (si.dwFlags & STARTF_USESHOWWINDOW) ? si.wShowWindow : SW_SHOW ); } return main_loop( &console, signal ); diff --git a/programs/conhost/conhost.h b/programs/conhost/conhost.h index 4464f51032f..e03d6239ce8 100644 --- a/programs/conhost/conhost.h +++ b/programs/conhost/conhost.h @@ -75,6 +75,7 @@ struct edit_line struct console { HANDLE server; /* console server handle */ + BOOL init; /* TRUE if console has initialized */ unsigned int mode; /* input mode */ struct screen_buffer *active; /* active screen buffer */ int is_unix; /* UNIX terminal mode */ @@ -145,6 +146,7 @@ NTSTATUS change_screen_buffer_size( struct screen_buffer *screen_buffer, int new void update_console_font( struct console *console, const WCHAR *face_name, size_t face_name_size, unsigned int height, unsigned int weight ); BOOL init_window( struct console *console ); +void init_window_config( struct console *console ); void init_message_window( struct console *console ); void update_window_region( struct console *console, const RECT *update ); void update_window_config( struct console *console, BOOL delay ); diff --git a/programs/conhost/window.c b/programs/conhost/window.c index 3db4b159696..cd02d60f40a 100644 --- a/programs/conhost/window.c +++ b/programs/conhost/window.c @@ -2392,33 +2392,25 @@ void update_window_region( struct console *console, const RECT *update ) update_window_config( console, TRUE ); } -BOOL init_window( struct console *console ) +void init_window_config( struct console *console ) { + size_t len, i; struct console_config config; - WNDCLASSW wndclass; STARTUPINFOW si; - CHARSETINFO ci; - - static struct console_window console_window; - console->window = &console_window; - if (!TranslateCharsetInfo( (DWORD *)(INT_PTR)GetACP(), &ci, TCI_SRCCODEPAGE )) - return FALSE; + console->init = TRUE; + if (!console->window || console->no_window) return; - console->window->ui_charset = ci.ciCharset; + len = console->title ? wcslen( console->title ) : 0; + console->window->config_key = malloc( (len + 1) * sizeof(WCHAR) ); - GetStartupInfoW(&si); - if (si.lpTitle) - { - size_t i, title_len = wcslen( si.lpTitle ); - if (!(console->window->config_key = malloc( (title_len + 1) * sizeof(WCHAR) ))) - return FALSE; - for (i = 0; i < title_len; i++) - console->window->config_key[i] = si.lpTitle[i] == '\\' ? '_' : si.lpTitle[i]; - console->window->config_key[title_len] = 0; - } + for (i = 0; i < len; i++) + console->window->config_key[i] = console->title[i] == '\\' ? '_' : console->title[i]; + console->window->config_key[len] = 0; load_config( console->window->config_key, &config ); + + GetStartupInfoW( &si ); if (si.dwFlags & STARTF_USECOUNTCHARS) { config.sb_width = si.dwXCountChars; @@ -2427,6 +2419,25 @@ BOOL init_window( struct console *console ) if (si.dwFlags & STARTF_USEFILLATTRIBUTE) config.attr = si.dwFillAttribute; + apply_config( console, &config ); + + ShowWindow( console->win, (si.dwFlags & STARTF_USESHOWWINDOW) ? si.wShowWindow : SW_SHOW ); +} + +BOOL init_window( struct console *console ) +{ + struct console_config config; + WNDCLASSW wndclass; + CHARSETINFO ci; + + static struct console_window console_window; + + console->window = &console_window; + if (!TranslateCharsetInfo( (DWORD *)(INT_PTR)GetACP(), &ci, TCI_SRCCODEPAGE )) + return FALSE; + + console->window->ui_charset = ci.ciCharset; + wndclass.style = CS_DBLCLKS; wndclass.lpfnWndProc = window_proc; wndclass.cbClsExtra = 0; @@ -2445,6 +2456,8 @@ BOOL init_window( struct console *console ) 0, 0, 0, 0, wndclass.hInstance, console )) return FALSE; + load_config( NULL, &config ); + if (!config.face_name[0]) set_first_font( console, &config ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1492
On Wed Nov 23 14:38:50 2022 +0000, eric pouech wrote:
for the title bits, just a wild guess: - if the console title is set via CreateProcess STARTUPINFO.lpTitle then the configuration is bound to the title - otherwise the configuration is bound to the first process path (AFAICS loading/storing configuration based on title is already present in window.c) you're right in Wine conhost / created process relationship can be tricky (to say the least): ``` $ wine cmd ``` both cmd.exe and conhost.exe are children of start.exe (and we'd expect to use cmd.exe and not start.exe for the configuration path, as they should be both attached to console) ``` $ wine programs/cmd/cmd.exe ``` conhost.exe is a child of cmd.exe (things are slightly different when allocating console at first with wineconsole) so picking the 'right' program may not be an easy task (note GetConsoleProcessList may help) I've taken a different approach that supports process path and custom titles.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1492#note_18022
Hi @epo, @jacek - I've reworked this patch, so it no longer requires server modifications. With the patch applied, we can load console settings using process path or custom title, depending on how the console process was launched. I've also fixed division-by-zero bug affecting the kernel32/console tests. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1492#note_18024
I'm puzzled: it's strange that delaying stuff will let STARTUP_INFO.lpTitle change. Looking a bit into it: it seems we don't get the expected values for lpTitle. Are you testing on Wine with wineconsole or do you use other ways? If by wineconsole, maybe the patch below could help. If not, then there are likely other places to be fixed. Extending the tests for CreateProcess regarding lpTitle could be starting point. ``` diff --git a/programs/wineconsole/wineconsole.c b/programs/wineconsole/wineconsole.c index 1924bf791e6..c029693bfcb 100644 --- a/programs/wineconsole/wineconsole.c +++ b/programs/wineconsole/wineconsole.c @@ -42,22 +42,10 @@ int WINAPI wWinMain( HINSTANCE inst, HINSTANCE prev, WCHAR *cmdline, INT show ) static WCHAR default_cmd[] = L"cmd"; FreeConsole(); /* make sure we're not connected to inherited console */ - if (!AllocConsole()) - { - ERR( "failed to allocate console: %lu\n", GetLastError() ); - return 1; - } if (!*cmd) cmd = default_cmd; - startup.dwFlags = STARTF_USESTDHANDLES; - startup.hStdInput = CreateFileW( L"CONIN$", GENERIC_READ | GENERIC_WRITE, 0, NULL, - OPEN_EXISTING, 0, 0 ); - startup.hStdOutput = CreateFileW( L"CONOUT$", GENERIC_READ | GENERIC_WRITE, 0, NULL, - OPEN_EXISTING, 0, 0 ); - startup.hStdError = startup.hStdOutput; - - if (!CreateProcessW( NULL, cmd, NULL, NULL, FALSE, 0, NULL, NULL, &startup, &info )) + if (!CreateProcessW( NULL, cmd, NULL, NULL, FALSE, CREATE_NEW_CONSOLE, NULL, NULL, &startup, &info )) { WCHAR format[256], *buf; INPUT_RECORD ir; ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1492#note_18067
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=127014 Your paranoid android. === debian11 (32 bit report) === wmvcore: wmvcore.c:1579: Test failed: Stream 0: Got hr 0xc00d0041.
On Wed Nov 30 16:50:58 2022 +0000, **** wrote:
Marvin replied on the mailing list: ``` 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=127014 Your paranoid android. === debian11 (32 bit report) === wmvcore: wmvcore.c:1579: Test failed: Stream 0: Got hr 0xc00d0041. ``` Hi Eric, I'm testing with wineconsole to get a plain windowed console with cmd.exe. I then run `start "abc" cmd.exe` to create a new console to test the title aspect.
As you've seen, wineconsole starts with a title of "C:\\windows\\system32\\wineconsole.exe" then updates to "C:\\windows\\system32\\cmd.exe". I tried your patch with good results. Trace output showed the windowed console started with "C:\\windows\\system32\\cmd.exe". Unless there's a compelling reason not to make this change, can you please send a patch? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1492#note_18563
On Sat Dec 3 11:51:28 2022 +0000, Hugh McMaster wrote:
Hi Eric, I'm testing with wineconsole to get a plain windowed console with cmd.exe. I then run `start "abc" cmd.exe` to create a new console to test the title aspect. As you've seen, wineconsole starts with a title of `C:\\windows\\system32\\wineconsole.exe` then updates to `C:\\windows\\system32\\cmd.exe`. I tried your patch with good results. Trace output showed the windowed console started with `C:\\windows\\system32\\cmd.exe`. Unless there's a compelling reason not to make this change, can you please send a patch? thanks Hugh. I-ll post the MR.
My interrogation was whether you've seen some other ways than just 'wine wineconsole xxx" to trigger the bad title -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1492#note_18566
This merge request was closed by Hugh McMaster. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1492
participants (4)
-
eric pouech (＠epo) -
Hugh McMaster
-
Hugh McMaster (＠hmc)
-
Marvin