* [edk2-devel] [PATCH v1] EmulatorPkg: Fix Terminal Issues @ 2023-09-26 18:36 Nate DeSimone 2023-09-26 20:03 ` Michael D Kinney 0 siblings, 1 reply; 7+ messages in thread From: Nate DeSimone @ 2023-09-26 18:36 UTC (permalink / raw) To: devel; +Cc: Andrew Fish, Ray Ni, Michael D Kinney, Chasel Chiu After running EmulatorPkg, one will notice that their terminal acts strangely. This is caused by the EmulatorPkg Host changing the terminal mode and not restoring the original mode, which is now fixed. Cc: Andrew Fish <afish@apple.com> Cc: Ray Ni <ray.ni@intel.com> Cc: Michael D Kinney <michael.d.kinney@intel.com> Cc: Chasel Chiu <chasel.chiu@intel.com> Signed-off-by: Nate DeSimone <nathaniel.l.desimone@intel.com> --- EmulatorPkg/Unix/Host/EmuThunk.c | 16 ++++++++++++- EmulatorPkg/Win/Host/WinThunk.c | 40 +++++++++++++++++++++++++++++++- 2 files changed, 54 insertions(+), 2 deletions(-) diff --git a/EmulatorPkg/Unix/Host/EmuThunk.c b/EmulatorPkg/Unix/Host/EmuThunk.c index 6422f056a6..e6879db650 100644 --- a/EmulatorPkg/Unix/Host/EmuThunk.c +++ b/EmulatorPkg/Unix/Host/EmuThunk.c @@ -9,7 +9,7 @@ it may cause the table to be initialized with the members at the end being set to zero. This is bad as jumping to zero will crash. -Copyright (c) 2004 - 2019, Intel Corporation. All rights reserved.<BR> +Copyright (c) 2004 - 2023, Intel Corporation. All rights reserved.<BR> Portions copyright (c) 2008 - 2011, Apple Inc. All rights reserved.<BR> SPDX-License-Identifier: BSD-2-Clause-Patent @@ -34,6 +34,9 @@ UINTN settimer_callback = 0; BOOLEAN gEmulatorInterruptEnabled = FALSE; +STATIC BOOLEAN mEmulatorStdInConfigured = FALSE; +STATIC struct termios mOldTty; + UINTN SecWriteStdErr ( IN UINT8 *Buffer, @@ -58,8 +61,15 @@ SecConfigStdIn ( // Need to turn off line buffering, ECHO, and make it unbuffered. // tcgetattr (STDIN_FILENO, &tty); + if (!mEmulatorStdInConfigured) { + // + // Save the original state of the TTY so it can be restored on exit + // + CopyMem (&mOldTty, &tty, sizeof (struct termios)); + } tty.c_lflag &= ~(ICANON | ECHO); tcsetattr (STDIN_FILENO, TCSANOW, &tty); + mEmulatorStdInConfigured = TRUE; // setvbuf (STDIN_FILENO, NULL, _IONBF, 0); @@ -338,6 +348,10 @@ SecExit ( UINTN Status ) { + // Reset the TTY back to its original state + if (mEmulatorStdInConfigured) { + tcsetattr (STDIN_FILENO, TCSANOW, &mOldTty); + } exit (Status); } diff --git a/EmulatorPkg/Win/Host/WinThunk.c b/EmulatorPkg/Win/Host/WinThunk.c index 008e5755db..90a6da2ece 100644 --- a/EmulatorPkg/Win/Host/WinThunk.c +++ b/EmulatorPkg/Win/Host/WinThunk.c @@ -1,6 +1,6 @@ /**@file -Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR> +Copyright (c) 2006 - 2023, Intel Corporation. All rights reserved.<BR> SPDX-License-Identifier: BSD-2-Clause-Patent Module Name: @@ -30,6 +30,12 @@ Abstract: #include "WinHost.h" +STATIC BOOLEAN mEmulatorStdInConfigured = FALSE; +STATIC DWORD mOldStdInMode; +#if defined (NTDDI_VERSION) && defined (NTDDI_WIN10_TH2) && (NTDDI_VERSION > NTDDI_WIN10_TH2) + STATIC DWORD mOldStdOutMode; +#endif + UINTN SecWriteStdErr ( IN UINT8 *Buffer, @@ -61,6 +67,12 @@ SecConfigStdIn ( Success = GetConsoleMode (GetStdHandle (STD_INPUT_HANDLE), &Mode); if (Success) { + if (!mEmulatorStdInConfigured) { + // + // Save the original state of the console so it can be restored on exit + // + mOldStdInMode = Mode; + } // // Disable buffer (line input), echo, mouse, window // @@ -82,6 +94,12 @@ SecConfigStdIn ( // if (Success) { Success = GetConsoleMode (GetStdHandle (STD_OUTPUT_HANDLE), &Mode); + if (!mEmulatorStdInConfigured) { + // + // Save the original state of the console so it can be restored on exit + // + mOldStdOutMode = Mode; + } if (Success) { Success = SetConsoleMode ( GetStdHandle (STD_OUTPUT_HANDLE), @@ -91,6 +109,9 @@ SecConfigStdIn ( } #endif + if (Success) { + mEmulatorStdInConfigured = TRUE; + } return Success ? EFI_SUCCESS : EFI_DEVICE_ERROR; } @@ -467,6 +488,23 @@ SecExit ( UINTN Status ) { + #if defined (NTDDI_VERSION) && defined (NTDDI_WIN10_TH2) && (NTDDI_VERSION > NTDDI_WIN10_TH2) + BOOL Success; + #endif + + if (mEmulatorStdInConfigured) { + // + // Reset the console back to its original state + // + #if defined (NTDDI_VERSION) && defined (NTDDI_WIN10_TH2) && (NTDDI_VERSION > NTDDI_WIN10_TH2) + Success = SetConsoleMode (GetStdHandle (STD_INPUT_HANDLE), mOldStdInMode); + if (Success) { + SetConsoleMode (GetStdHandle (STD_OUTPUT_HANDLE), mOldStdOutMode); + } + #else + SetConsoleMode (GetStdHandle (STD_INPUT_HANDLE), mOldStdInMode); + #endif + } exit ((int)Status); } -- 2.34.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#109080): https://edk2.groups.io/g/devel/message/109080 Mute This Topic: https://groups.io/mt/101602599/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [PATCH v1] EmulatorPkg: Fix Terminal Issues 2023-09-26 18:36 [edk2-devel] [PATCH v1] EmulatorPkg: Fix Terminal Issues Nate DeSimone @ 2023-09-26 20:03 ` Michael D Kinney 2023-09-26 20:37 ` Nate DeSimone 0 siblings, 1 reply; 7+ messages in thread From: Michael D Kinney @ 2023-09-26 20:03 UTC (permalink / raw) To: Desimone, Nathaniel L, devel@edk2.groups.io Cc: Andrew Fish, Ni, Ray, Chiu, Chasel, Kinney, Michael D Thanks Nate! I have noticed this issue for a while. One comment below. With that update: Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com> Mike > -----Original Message----- > From: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com> > Sent: Tuesday, September 26, 2023 11:36 AM > To: devel@edk2.groups.io > Cc: Andrew Fish <afish@apple.com>; Ni, Ray <ray.ni@intel.com>; Kinney, > Michael D <michael.d.kinney@intel.com>; Chiu, Chasel > <chasel.chiu@intel.com> > Subject: [PATCH v1] EmulatorPkg: Fix Terminal Issues > > After running EmulatorPkg, one will notice that their terminal acts > strangely. This is caused by the EmulatorPkg Host changing the > terminal > mode and not restoring the original mode, which is now fixed. > > Cc: Andrew Fish <afish@apple.com> > Cc: Ray Ni <ray.ni@intel.com> > Cc: Michael D Kinney <michael.d.kinney@intel.com> > Cc: Chasel Chiu <chasel.chiu@intel.com> > Signed-off-by: Nate DeSimone <nathaniel.l.desimone@intel.com> > --- > EmulatorPkg/Unix/Host/EmuThunk.c | 16 ++++++++++++- > EmulatorPkg/Win/Host/WinThunk.c | 40 > +++++++++++++++++++++++++++++++- > 2 files changed, 54 insertions(+), 2 deletions(-) > > diff --git a/EmulatorPkg/Unix/Host/EmuThunk.c > b/EmulatorPkg/Unix/Host/EmuThunk.c > index 6422f056a6..e6879db650 100644 > --- a/EmulatorPkg/Unix/Host/EmuThunk.c > +++ b/EmulatorPkg/Unix/Host/EmuThunk.c > @@ -9,7 +9,7 @@ > it may cause the table to be initialized with the members at the > end being > set to zero. This is bad as jumping to zero will crash. > > -Copyright (c) 2004 - 2019, Intel Corporation. All rights > reserved.<BR> > +Copyright (c) 2004 - 2023, Intel Corporation. All rights > reserved.<BR> > Portions copyright (c) 2008 - 2011, Apple Inc. All rights > reserved.<BR> > SPDX-License-Identifier: BSD-2-Clause-Patent > > @@ -34,6 +34,9 @@ UINTN settimer_callback = 0; > > BOOLEAN gEmulatorInterruptEnabled = FALSE; > > +STATIC BOOLEAN mEmulatorStdInConfigured = FALSE; > +STATIC struct termios mOldTty; > + > UINTN > SecWriteStdErr ( > IN UINT8 *Buffer, > @@ -58,8 +61,15 @@ SecConfigStdIn ( > // Need to turn off line buffering, ECHO, and make it unbuffered. > // > tcgetattr (STDIN_FILENO, &tty); > + if (!mEmulatorStdInConfigured) { > + // > + // Save the original state of the TTY so it can be restored on > exit > + // > + CopyMem (&mOldTty, &tty, sizeof (struct termios)); > + } > tty.c_lflag &= ~(ICANON | ECHO); > tcsetattr (STDIN_FILENO, TCSANOW, &tty); > + mEmulatorStdInConfigured = TRUE; > > // setvbuf (STDIN_FILENO, NULL, _IONBF, 0); > > @@ -338,6 +348,10 @@ SecExit ( > UINTN Status > ) > { > + // Reset the TTY back to its original state > + if (mEmulatorStdInConfigured) { > + tcsetattr (STDIN_FILENO, TCSANOW, &mOldTty); > + } > exit (Status); > } > > diff --git a/EmulatorPkg/Win/Host/WinThunk.c > b/EmulatorPkg/Win/Host/WinThunk.c > index 008e5755db..90a6da2ece 100644 > --- a/EmulatorPkg/Win/Host/WinThunk.c > +++ b/EmulatorPkg/Win/Host/WinThunk.c > @@ -1,6 +1,6 @@ > /**@file > > -Copyright (c) 2006 - 2018, Intel Corporation. All rights > reserved.<BR> > +Copyright (c) 2006 - 2023, Intel Corporation. All rights > reserved.<BR> > SPDX-License-Identifier: BSD-2-Clause-Patent > > Module Name: > @@ -30,6 +30,12 @@ Abstract: > > #include "WinHost.h" > > +STATIC BOOLEAN mEmulatorStdInConfigured = FALSE; > +STATIC DWORD mOldStdInMode; > +#if defined (NTDDI_VERSION) && defined (NTDDI_WIN10_TH2) && > (NTDDI_VERSION > NTDDI_WIN10_TH2) > + STATIC DWORD mOldStdOutMode; > +#endif > + > UINTN > SecWriteStdErr ( > IN UINT8 *Buffer, > @@ -61,6 +67,12 @@ SecConfigStdIn ( > > Success = GetConsoleMode (GetStdHandle (STD_INPUT_HANDLE), &Mode); > if (Success) { > + if (!mEmulatorStdInConfigured) { > + // > + // Save the original state of the console so it can be restored > on exit > + // > + mOldStdInMode = Mode; > + } > // > // Disable buffer (line input), echo, mouse, window > // > @@ -82,6 +94,12 @@ SecConfigStdIn ( > // > if (Success) { > Success = GetConsoleMode (GetStdHandle (STD_OUTPUT_HANDLE), > &Mode); > + if (!mEmulatorStdInConfigured) { > + // > + // Save the original state of the console so it can be restored > on exit > + // > + mOldStdOutMode = Mode; > + } > if (Success) { > Success = SetConsoleMode ( > GetStdHandle (STD_OUTPUT_HANDLE), > @@ -91,6 +109,9 @@ SecConfigStdIn ( > } > > #endif > + if (Success) { > + mEmulatorStdInConfigured = TRUE; > + } > return Success ? EFI_SUCCESS : EFI_DEVICE_ERROR; > } > > @@ -467,6 +488,23 @@ SecExit ( > UINTN Status > ) > { > + #if defined (NTDDI_VERSION) && defined (NTDDI_WIN10_TH2) && > (NTDDI_VERSION > NTDDI_WIN10_TH2) > + BOOL Success; > + #endif > + > + if (mEmulatorStdInConfigured) { > + // > + // Reset the console back to its original state > + // > + #if defined (NTDDI_VERSION) && defined (NTDDI_WIN10_TH2) && > (NTDDI_VERSION > NTDDI_WIN10_TH2) I think the BOOL Success variable could be added here and reduce the #if statements > + Success = SetConsoleMode (GetStdHandle (STD_INPUT_HANDLE), > mOldStdInMode); > + if (Success) { > + SetConsoleMode (GetStdHandle (STD_OUTPUT_HANDLE), > mOldStdOutMode); > + } > + #else > + SetConsoleMode (GetStdHandle (STD_INPUT_HANDLE), mOldStdInMode); > + #endif > + } > exit ((int)Status); > } > > -- > 2.34.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#109087): https://edk2.groups.io/g/devel/message/109087 Mute This Topic: https://groups.io/mt/101602599/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [PATCH v1] EmulatorPkg: Fix Terminal Issues 2023-09-26 20:03 ` Michael D Kinney @ 2023-09-26 20:37 ` Nate DeSimone 2023-09-26 20:39 ` Nate DeSimone 0 siblings, 1 reply; 7+ messages in thread From: Nate DeSimone @ 2023-09-26 20:37 UTC (permalink / raw) To: Kinney, Michael D, devel@edk2.groups.io Cc: Andrew Fish, Ni, Ray, Chiu, Chasel Hi Mike, Unfortunately, that change will generate the following warning on GCC4.6+ warning: variable "Success" set but not used [-Wunused-but-set-variable] Hence why I wrote it that way. Let me know if you would like me to make a different change before committing. Thanks, Nate -----Original Message----- From: Kinney, Michael D <michael.d.kinney@intel.com> Sent: Tuesday, September 26, 2023 1:03 PM To: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; devel@edk2.groups.io Cc: Andrew Fish <afish@apple.com>; Ni, Ray <ray.ni@intel.com>; Chiu, Chasel <chasel.chiu@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com> Subject: RE: [PATCH v1] EmulatorPkg: Fix Terminal Issues Thanks Nate! I have noticed this issue for a while. One comment below. With that update: Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com> Mike > -----Original Message----- > From: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com> > Sent: Tuesday, September 26, 2023 11:36 AM > To: devel@edk2.groups.io > Cc: Andrew Fish <afish@apple.com>; Ni, Ray <ray.ni@intel.com>; Kinney, > Michael D <michael.d.kinney@intel.com>; Chiu, Chasel > <chasel.chiu@intel.com> > Subject: [PATCH v1] EmulatorPkg: Fix Terminal Issues > > After running EmulatorPkg, one will notice that their terminal acts > strangely. This is caused by the EmulatorPkg Host changing the > terminal mode and not restoring the original mode, which is now fixed. > > Cc: Andrew Fish <afish@apple.com> > Cc: Ray Ni <ray.ni@intel.com> > Cc: Michael D Kinney <michael.d.kinney@intel.com> > Cc: Chasel Chiu <chasel.chiu@intel.com> > Signed-off-by: Nate DeSimone <nathaniel.l.desimone@intel.com> > --- > EmulatorPkg/Unix/Host/EmuThunk.c | 16 ++++++++++++- > EmulatorPkg/Win/Host/WinThunk.c | 40 > +++++++++++++++++++++++++++++++- > 2 files changed, 54 insertions(+), 2 deletions(-) > > diff --git a/EmulatorPkg/Unix/Host/EmuThunk.c > b/EmulatorPkg/Unix/Host/EmuThunk.c > index 6422f056a6..e6879db650 100644 > --- a/EmulatorPkg/Unix/Host/EmuThunk.c > +++ b/EmulatorPkg/Unix/Host/EmuThunk.c > @@ -9,7 +9,7 @@ > it may cause the table to be initialized with the members at the > end being > set to zero. This is bad as jumping to zero will crash. > > -Copyright (c) 2004 - 2019, Intel Corporation. All rights > reserved.<BR> > +Copyright (c) 2004 - 2023, Intel Corporation. All rights > reserved.<BR> > Portions copyright (c) 2008 - 2011, Apple Inc. All rights > reserved.<BR> > SPDX-License-Identifier: BSD-2-Clause-Patent > > @@ -34,6 +34,9 @@ UINTN settimer_callback = 0; > > BOOLEAN gEmulatorInterruptEnabled = FALSE; > > +STATIC BOOLEAN mEmulatorStdInConfigured = FALSE; STATIC struct > +termios mOldTty; > + > UINTN > SecWriteStdErr ( > IN UINT8 *Buffer, > @@ -58,8 +61,15 @@ SecConfigStdIn ( > // Need to turn off line buffering, ECHO, and make it unbuffered. > // > tcgetattr (STDIN_FILENO, &tty); > + if (!mEmulatorStdInConfigured) { > + // > + // Save the original state of the TTY so it can be restored on > exit > + // > + CopyMem (&mOldTty, &tty, sizeof (struct termios)); } > tty.c_lflag &= ~(ICANON | ECHO); > tcsetattr (STDIN_FILENO, TCSANOW, &tty); > + mEmulatorStdInConfigured = TRUE; > > // setvbuf (STDIN_FILENO, NULL, _IONBF, 0); > > @@ -338,6 +348,10 @@ SecExit ( > UINTN Status > ) > { > + // Reset the TTY back to its original state if > + (mEmulatorStdInConfigured) { > + tcsetattr (STDIN_FILENO, TCSANOW, &mOldTty); } > exit (Status); > } > > diff --git a/EmulatorPkg/Win/Host/WinThunk.c > b/EmulatorPkg/Win/Host/WinThunk.c index 008e5755db..90a6da2ece 100644 > --- a/EmulatorPkg/Win/Host/WinThunk.c > +++ b/EmulatorPkg/Win/Host/WinThunk.c > @@ -1,6 +1,6 @@ > /**@file > > -Copyright (c) 2006 - 2018, Intel Corporation. All rights > reserved.<BR> > +Copyright (c) 2006 - 2023, Intel Corporation. All rights > reserved.<BR> > SPDX-License-Identifier: BSD-2-Clause-Patent > > Module Name: > @@ -30,6 +30,12 @@ Abstract: > > #include "WinHost.h" > > +STATIC BOOLEAN mEmulatorStdInConfigured = FALSE; STATIC DWORD > +mOldStdInMode; #if defined (NTDDI_VERSION) && defined > +(NTDDI_WIN10_TH2) && > (NTDDI_VERSION > NTDDI_WIN10_TH2) > + STATIC DWORD mOldStdOutMode; > +#endif > + > UINTN > SecWriteStdErr ( > IN UINT8 *Buffer, > @@ -61,6 +67,12 @@ SecConfigStdIn ( > > Success = GetConsoleMode (GetStdHandle (STD_INPUT_HANDLE), &Mode); > if (Success) { > + if (!mEmulatorStdInConfigured) { > + // > + // Save the original state of the console so it can be restored > on exit > + // > + mOldStdInMode = Mode; > + } > // > // Disable buffer (line input), echo, mouse, window > // > @@ -82,6 +94,12 @@ SecConfigStdIn ( > // > if (Success) { > Success = GetConsoleMode (GetStdHandle (STD_OUTPUT_HANDLE), > &Mode); > + if (!mEmulatorStdInConfigured) { > + // > + // Save the original state of the console so it can be restored > on exit > + // > + mOldStdOutMode = Mode; > + } > if (Success) { > Success = SetConsoleMode ( > GetStdHandle (STD_OUTPUT_HANDLE), @@ -91,6 +109,9 > @@ SecConfigStdIn ( > } > > #endif > + if (Success) { > + mEmulatorStdInConfigured = TRUE; > + } > return Success ? EFI_SUCCESS : EFI_DEVICE_ERROR; } > > @@ -467,6 +488,23 @@ SecExit ( > UINTN Status > ) > { > + #if defined (NTDDI_VERSION) && defined (NTDDI_WIN10_TH2) && > (NTDDI_VERSION > NTDDI_WIN10_TH2) > + BOOL Success; > + #endif > + > + if (mEmulatorStdInConfigured) { > + // > + // Reset the console back to its original state > + // > + #if defined (NTDDI_VERSION) && defined (NTDDI_WIN10_TH2) && > (NTDDI_VERSION > NTDDI_WIN10_TH2) I think the BOOL Success variable could be added here and reduce the #if statements > + Success = SetConsoleMode (GetStdHandle (STD_INPUT_HANDLE), > mOldStdInMode); > + if (Success) { > + SetConsoleMode (GetStdHandle (STD_OUTPUT_HANDLE), > mOldStdOutMode); > + } > + #else > + SetConsoleMode (GetStdHandle (STD_INPUT_HANDLE), mOldStdInMode); > + #endif } > exit ((int)Status); > } > > -- > 2.34.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#109090): https://edk2.groups.io/g/devel/message/109090 Mute This Topic: https://groups.io/mt/101602599/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [PATCH v1] EmulatorPkg: Fix Terminal Issues 2023-09-26 20:37 ` Nate DeSimone @ 2023-09-26 20:39 ` Nate DeSimone 2023-09-26 20:50 ` Nate DeSimone 0 siblings, 1 reply; 7+ messages in thread From: Nate DeSimone @ 2023-09-26 20:39 UTC (permalink / raw) To: Kinney, Michael D, devel@edk2.groups.io Cc: Andrew Fish, Ni, Ray, Chiu, Chasel Oh, never mind, I see you are suggesting a C99 style variable declaration. Do we need to worry about old compiler that want all variable declarations at the top still? Thanks, Nate -----Original Message----- From: Desimone, Nathaniel L Sent: Tuesday, September 26, 2023 1:38 PM To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io Cc: Andrew Fish <afish@apple.com>; Ni, Ray <ray.ni@intel.com>; Chiu, Chasel <chasel.chiu@intel.com> Subject: RE: [PATCH v1] EmulatorPkg: Fix Terminal Issues Hi Mike, Unfortunately, that change will generate the following warning on GCC4.6+ warning: variable "Success" set but not used [-Wunused-but-set-variable] Hence why I wrote it that way. Let me know if you would like me to make a different change before committing. Thanks, Nate -----Original Message----- From: Kinney, Michael D <michael.d.kinney@intel.com> Sent: Tuesday, September 26, 2023 1:03 PM To: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; devel@edk2.groups.io Cc: Andrew Fish <afish@apple.com>; Ni, Ray <ray.ni@intel.com>; Chiu, Chasel <chasel.chiu@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com> Subject: RE: [PATCH v1] EmulatorPkg: Fix Terminal Issues Thanks Nate! I have noticed this issue for a while. One comment below. With that update: Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com> Mike > -----Original Message----- > From: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com> > Sent: Tuesday, September 26, 2023 11:36 AM > To: devel@edk2.groups.io > Cc: Andrew Fish <afish@apple.com>; Ni, Ray <ray.ni@intel.com>; Kinney, > Michael D <michael.d.kinney@intel.com>; Chiu, Chasel > <chasel.chiu@intel.com> > Subject: [PATCH v1] EmulatorPkg: Fix Terminal Issues > > After running EmulatorPkg, one will notice that their terminal acts > strangely. This is caused by the EmulatorPkg Host changing the > terminal mode and not restoring the original mode, which is now fixed. > > Cc: Andrew Fish <afish@apple.com> > Cc: Ray Ni <ray.ni@intel.com> > Cc: Michael D Kinney <michael.d.kinney@intel.com> > Cc: Chasel Chiu <chasel.chiu@intel.com> > Signed-off-by: Nate DeSimone <nathaniel.l.desimone@intel.com> > --- > EmulatorPkg/Unix/Host/EmuThunk.c | 16 ++++++++++++- > EmulatorPkg/Win/Host/WinThunk.c | 40 > +++++++++++++++++++++++++++++++- > 2 files changed, 54 insertions(+), 2 deletions(-) > > diff --git a/EmulatorPkg/Unix/Host/EmuThunk.c > b/EmulatorPkg/Unix/Host/EmuThunk.c > index 6422f056a6..e6879db650 100644 > --- a/EmulatorPkg/Unix/Host/EmuThunk.c > +++ b/EmulatorPkg/Unix/Host/EmuThunk.c > @@ -9,7 +9,7 @@ > it may cause the table to be initialized with the members at the > end being > set to zero. This is bad as jumping to zero will crash. > > -Copyright (c) 2004 - 2019, Intel Corporation. All rights > reserved.<BR> > +Copyright (c) 2004 - 2023, Intel Corporation. All rights > reserved.<BR> > Portions copyright (c) 2008 - 2011, Apple Inc. All rights > reserved.<BR> > SPDX-License-Identifier: BSD-2-Clause-Patent > > @@ -34,6 +34,9 @@ UINTN settimer_callback = 0; > > BOOLEAN gEmulatorInterruptEnabled = FALSE; > > +STATIC BOOLEAN mEmulatorStdInConfigured = FALSE; STATIC struct > +termios mOldTty; > + > UINTN > SecWriteStdErr ( > IN UINT8 *Buffer, > @@ -58,8 +61,15 @@ SecConfigStdIn ( > // Need to turn off line buffering, ECHO, and make it unbuffered. > // > tcgetattr (STDIN_FILENO, &tty); > + if (!mEmulatorStdInConfigured) { > + // > + // Save the original state of the TTY so it can be restored on > exit > + // > + CopyMem (&mOldTty, &tty, sizeof (struct termios)); } > tty.c_lflag &= ~(ICANON | ECHO); > tcsetattr (STDIN_FILENO, TCSANOW, &tty); > + mEmulatorStdInConfigured = TRUE; > > // setvbuf (STDIN_FILENO, NULL, _IONBF, 0); > > @@ -338,6 +348,10 @@ SecExit ( > UINTN Status > ) > { > + // Reset the TTY back to its original state if > + (mEmulatorStdInConfigured) { > + tcsetattr (STDIN_FILENO, TCSANOW, &mOldTty); } > exit (Status); > } > > diff --git a/EmulatorPkg/Win/Host/WinThunk.c > b/EmulatorPkg/Win/Host/WinThunk.c index 008e5755db..90a6da2ece 100644 > --- a/EmulatorPkg/Win/Host/WinThunk.c > +++ b/EmulatorPkg/Win/Host/WinThunk.c > @@ -1,6 +1,6 @@ > /**@file > > -Copyright (c) 2006 - 2018, Intel Corporation. All rights > reserved.<BR> > +Copyright (c) 2006 - 2023, Intel Corporation. All rights > reserved.<BR> > SPDX-License-Identifier: BSD-2-Clause-Patent > > Module Name: > @@ -30,6 +30,12 @@ Abstract: > > #include "WinHost.h" > > +STATIC BOOLEAN mEmulatorStdInConfigured = FALSE; STATIC DWORD > +mOldStdInMode; #if defined (NTDDI_VERSION) && defined > +(NTDDI_WIN10_TH2) && > (NTDDI_VERSION > NTDDI_WIN10_TH2) > + STATIC DWORD mOldStdOutMode; > +#endif > + > UINTN > SecWriteStdErr ( > IN UINT8 *Buffer, > @@ -61,6 +67,12 @@ SecConfigStdIn ( > > Success = GetConsoleMode (GetStdHandle (STD_INPUT_HANDLE), &Mode); > if (Success) { > + if (!mEmulatorStdInConfigured) { > + // > + // Save the original state of the console so it can be restored > on exit > + // > + mOldStdInMode = Mode; > + } > // > // Disable buffer (line input), echo, mouse, window > // > @@ -82,6 +94,12 @@ SecConfigStdIn ( > // > if (Success) { > Success = GetConsoleMode (GetStdHandle (STD_OUTPUT_HANDLE), > &Mode); > + if (!mEmulatorStdInConfigured) { > + // > + // Save the original state of the console so it can be restored > on exit > + // > + mOldStdOutMode = Mode; > + } > if (Success) { > Success = SetConsoleMode ( > GetStdHandle (STD_OUTPUT_HANDLE), @@ -91,6 +109,9 > @@ SecConfigStdIn ( > } > > #endif > + if (Success) { > + mEmulatorStdInConfigured = TRUE; > + } > return Success ? EFI_SUCCESS : EFI_DEVICE_ERROR; } > > @@ -467,6 +488,23 @@ SecExit ( > UINTN Status > ) > { > + #if defined (NTDDI_VERSION) && defined (NTDDI_WIN10_TH2) && > (NTDDI_VERSION > NTDDI_WIN10_TH2) > + BOOL Success; > + #endif > + > + if (mEmulatorStdInConfigured) { > + // > + // Reset the console back to its original state > + // > + #if defined (NTDDI_VERSION) && defined (NTDDI_WIN10_TH2) && > (NTDDI_VERSION > NTDDI_WIN10_TH2) I think the BOOL Success variable could be added here and reduce the #if statements > + Success = SetConsoleMode (GetStdHandle (STD_INPUT_HANDLE), > mOldStdInMode); > + if (Success) { > + SetConsoleMode (GetStdHandle (STD_OUTPUT_HANDLE), > mOldStdOutMode); > + } > + #else > + SetConsoleMode (GetStdHandle (STD_INPUT_HANDLE), mOldStdInMode); > + #endif } > exit ((int)Status); > } > > -- > 2.34.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#109091): https://edk2.groups.io/g/devel/message/109091 Mute This Topic: https://groups.io/mt/101602599/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [PATCH v1] EmulatorPkg: Fix Terminal Issues 2023-09-26 20:39 ` Nate DeSimone @ 2023-09-26 20:50 ` Nate DeSimone 2023-09-26 21:42 ` Michael D Kinney 0 siblings, 1 reply; 7+ messages in thread From: Nate DeSimone @ 2023-09-26 20:50 UTC (permalink / raw) To: Kinney, Michael D, devel@edk2.groups.io Cc: Andrew Fish, Ni, Ray, Chiu, Chasel Looks like it compiles fine in VS2015, and we just removed the tools_def entries for every VC++ version older than that. And C99 has been in gcc for a very long time and clang forever. So perhaps we can start using C99 style variable declarations finally? Thanks, Nate -----Original Message----- From: Desimone, Nathaniel L Sent: Tuesday, September 26, 2023 1:39 PM To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io Cc: Andrew Fish <afish@apple.com>; Ni, Ray <ray.ni@intel.com>; Chiu, Chasel <chasel.chiu@intel.com> Subject: RE: [PATCH v1] EmulatorPkg: Fix Terminal Issues Oh, never mind, I see you are suggesting a C99 style variable declaration. Do we need to worry about old compiler that want all variable declarations at the top still? Thanks, Nate -----Original Message----- From: Desimone, Nathaniel L Sent: Tuesday, September 26, 2023 1:38 PM To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io Cc: Andrew Fish <afish@apple.com>; Ni, Ray <ray.ni@intel.com>; Chiu, Chasel <chasel.chiu@intel.com> Subject: RE: [PATCH v1] EmulatorPkg: Fix Terminal Issues Hi Mike, Unfortunately, that change will generate the following warning on GCC4.6+ warning: variable "Success" set but not used [-Wunused-but-set-variable] Hence why I wrote it that way. Let me know if you would like me to make a different change before committing. Thanks, Nate -----Original Message----- From: Kinney, Michael D <michael.d.kinney@intel.com> Sent: Tuesday, September 26, 2023 1:03 PM To: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; devel@edk2.groups.io Cc: Andrew Fish <afish@apple.com>; Ni, Ray <ray.ni@intel.com>; Chiu, Chasel <chasel.chiu@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com> Subject: RE: [PATCH v1] EmulatorPkg: Fix Terminal Issues Thanks Nate! I have noticed this issue for a while. One comment below. With that update: Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com> Mike > -----Original Message----- > From: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com> > Sent: Tuesday, September 26, 2023 11:36 AM > To: devel@edk2.groups.io > Cc: Andrew Fish <afish@apple.com>; Ni, Ray <ray.ni@intel.com>; Kinney, > Michael D <michael.d.kinney@intel.com>; Chiu, Chasel > <chasel.chiu@intel.com> > Subject: [PATCH v1] EmulatorPkg: Fix Terminal Issues > > After running EmulatorPkg, one will notice that their terminal acts > strangely. This is caused by the EmulatorPkg Host changing the > terminal mode and not restoring the original mode, which is now fixed. > > Cc: Andrew Fish <afish@apple.com> > Cc: Ray Ni <ray.ni@intel.com> > Cc: Michael D Kinney <michael.d.kinney@intel.com> > Cc: Chasel Chiu <chasel.chiu@intel.com> > Signed-off-by: Nate DeSimone <nathaniel.l.desimone@intel.com> > --- > EmulatorPkg/Unix/Host/EmuThunk.c | 16 ++++++++++++- > EmulatorPkg/Win/Host/WinThunk.c | 40 > +++++++++++++++++++++++++++++++- > 2 files changed, 54 insertions(+), 2 deletions(-) > > diff --git a/EmulatorPkg/Unix/Host/EmuThunk.c > b/EmulatorPkg/Unix/Host/EmuThunk.c > index 6422f056a6..e6879db650 100644 > --- a/EmulatorPkg/Unix/Host/EmuThunk.c > +++ b/EmulatorPkg/Unix/Host/EmuThunk.c > @@ -9,7 +9,7 @@ > it may cause the table to be initialized with the members at the > end being > set to zero. This is bad as jumping to zero will crash. > > -Copyright (c) 2004 - 2019, Intel Corporation. All rights > reserved.<BR> > +Copyright (c) 2004 - 2023, Intel Corporation. All rights > reserved.<BR> > Portions copyright (c) 2008 - 2011, Apple Inc. All rights > reserved.<BR> > SPDX-License-Identifier: BSD-2-Clause-Patent > > @@ -34,6 +34,9 @@ UINTN settimer_callback = 0; > > BOOLEAN gEmulatorInterruptEnabled = FALSE; > > +STATIC BOOLEAN mEmulatorStdInConfigured = FALSE; STATIC struct > +termios mOldTty; > + > UINTN > SecWriteStdErr ( > IN UINT8 *Buffer, > @@ -58,8 +61,15 @@ SecConfigStdIn ( > // Need to turn off line buffering, ECHO, and make it unbuffered. > // > tcgetattr (STDIN_FILENO, &tty); > + if (!mEmulatorStdInConfigured) { > + // > + // Save the original state of the TTY so it can be restored on > exit > + // > + CopyMem (&mOldTty, &tty, sizeof (struct termios)); } > tty.c_lflag &= ~(ICANON | ECHO); > tcsetattr (STDIN_FILENO, TCSANOW, &tty); > + mEmulatorStdInConfigured = TRUE; > > // setvbuf (STDIN_FILENO, NULL, _IONBF, 0); > > @@ -338,6 +348,10 @@ SecExit ( > UINTN Status > ) > { > + // Reset the TTY back to its original state if > + (mEmulatorStdInConfigured) { > + tcsetattr (STDIN_FILENO, TCSANOW, &mOldTty); } > exit (Status); > } > > diff --git a/EmulatorPkg/Win/Host/WinThunk.c > b/EmulatorPkg/Win/Host/WinThunk.c index 008e5755db..90a6da2ece 100644 > --- a/EmulatorPkg/Win/Host/WinThunk.c > +++ b/EmulatorPkg/Win/Host/WinThunk.c > @@ -1,6 +1,6 @@ > /**@file > > -Copyright (c) 2006 - 2018, Intel Corporation. All rights > reserved.<BR> > +Copyright (c) 2006 - 2023, Intel Corporation. All rights > reserved.<BR> > SPDX-License-Identifier: BSD-2-Clause-Patent > > Module Name: > @@ -30,6 +30,12 @@ Abstract: > > #include "WinHost.h" > > +STATIC BOOLEAN mEmulatorStdInConfigured = FALSE; STATIC DWORD > +mOldStdInMode; #if defined (NTDDI_VERSION) && defined > +(NTDDI_WIN10_TH2) && > (NTDDI_VERSION > NTDDI_WIN10_TH2) > + STATIC DWORD mOldStdOutMode; > +#endif > + > UINTN > SecWriteStdErr ( > IN UINT8 *Buffer, > @@ -61,6 +67,12 @@ SecConfigStdIn ( > > Success = GetConsoleMode (GetStdHandle (STD_INPUT_HANDLE), &Mode); > if (Success) { > + if (!mEmulatorStdInConfigured) { > + // > + // Save the original state of the console so it can be restored > on exit > + // > + mOldStdInMode = Mode; > + } > // > // Disable buffer (line input), echo, mouse, window > // > @@ -82,6 +94,12 @@ SecConfigStdIn ( > // > if (Success) { > Success = GetConsoleMode (GetStdHandle (STD_OUTPUT_HANDLE), > &Mode); > + if (!mEmulatorStdInConfigured) { > + // > + // Save the original state of the console so it can be restored > on exit > + // > + mOldStdOutMode = Mode; > + } > if (Success) { > Success = SetConsoleMode ( > GetStdHandle (STD_OUTPUT_HANDLE), @@ -91,6 +109,9 > @@ SecConfigStdIn ( > } > > #endif > + if (Success) { > + mEmulatorStdInConfigured = TRUE; > + } > return Success ? EFI_SUCCESS : EFI_DEVICE_ERROR; } > > @@ -467,6 +488,23 @@ SecExit ( > UINTN Status > ) > { > + #if defined (NTDDI_VERSION) && defined (NTDDI_WIN10_TH2) && > (NTDDI_VERSION > NTDDI_WIN10_TH2) > + BOOL Success; > + #endif > + > + if (mEmulatorStdInConfigured) { > + // > + // Reset the console back to its original state > + // > + #if defined (NTDDI_VERSION) && defined (NTDDI_WIN10_TH2) && > (NTDDI_VERSION > NTDDI_WIN10_TH2) I think the BOOL Success variable could be added here and reduce the #if statements > + Success = SetConsoleMode (GetStdHandle (STD_INPUT_HANDLE), > mOldStdInMode); > + if (Success) { > + SetConsoleMode (GetStdHandle (STD_OUTPUT_HANDLE), > mOldStdOutMode); > + } > + #else > + SetConsoleMode (GetStdHandle (STD_INPUT_HANDLE), mOldStdInMode); > + #endif } > exit ((int)Status); > } > > -- > 2.34.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#109092): https://edk2.groups.io/g/devel/message/109092 Mute This Topic: https://groups.io/mt/101602599/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [PATCH v1] EmulatorPkg: Fix Terminal Issues 2023-09-26 20:50 ` Nate DeSimone @ 2023-09-26 21:42 ` Michael D Kinney 2023-09-27 2:59 ` Nate DeSimone 0 siblings, 1 reply; 7+ messages in thread From: Michael D Kinney @ 2023-09-26 21:42 UTC (permalink / raw) To: Desimone, Nathaniel L, devel@edk2.groups.io Cc: Andrew Fish, Ni, Ray, Chiu, Chasel, Kinney, Michael D Hi Nate, Declaring all the local variables at the top of the function is really a coding style recommendation documented in the EDK II C Coding Style Specification. End of Section 5.4.1.1. Declaring local in other scopes is strongly discouraged. https://tianocore-docs.github.io/edk2-CCodingStandardsSpecification/draft/5_source_files/54_code_file_structure.html#54-code-file-structure Block (local) Scope Formal parameters in function definitions and data defined within compound statements have block scope. Any group of statements that are encompassed within a pair of braces, { }, is a compound statement. The body of a function is also a compound statement. Compound statements can be nested, with each creating a new scope. Data declarations may follow the opening brace of a compound statement, regardless of nesting depth, and before any code generating statements have been entered. Other than at the outermost block of a function body, this type of declaration is strongly discouraged. This specific change here is in a windows specific file and improving readability of the windows specific version checks in #if statements warrants an exception. Mike > -----Original Message----- > From: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com> > Sent: Tuesday, September 26, 2023 1:51 PM > To: Kinney, Michael D <michael.d.kinney@intel.com>; > devel@edk2.groups.io > Cc: Andrew Fish <afish@apple.com>; Ni, Ray <ray.ni@intel.com>; Chiu, > Chasel <chasel.chiu@intel.com> > Subject: RE: [PATCH v1] EmulatorPkg: Fix Terminal Issues > > Looks like it compiles fine in VS2015, and we just removed the > tools_def entries for every VC++ version older than that. And C99 has > been in gcc for a very long time and clang forever. So perhaps we can > start using C99 style variable declarations finally? > > Thanks, > Nate > > -----Original Message----- > From: Desimone, Nathaniel L > Sent: Tuesday, September 26, 2023 1:39 PM > To: Kinney, Michael D <michael.d.kinney@intel.com>; > devel@edk2.groups.io > Cc: Andrew Fish <afish@apple.com>; Ni, Ray <ray.ni@intel.com>; Chiu, > Chasel <chasel.chiu@intel.com> > Subject: RE: [PATCH v1] EmulatorPkg: Fix Terminal Issues > > Oh, never mind, I see you are suggesting a C99 style variable > declaration. Do we need to worry about old compiler that want all > variable declarations at the top still? > > Thanks, > Nate > > -----Original Message----- > From: Desimone, Nathaniel L > Sent: Tuesday, September 26, 2023 1:38 PM > To: Kinney, Michael D <michael.d.kinney@intel.com>; > devel@edk2.groups.io > Cc: Andrew Fish <afish@apple.com>; Ni, Ray <ray.ni@intel.com>; Chiu, > Chasel <chasel.chiu@intel.com> > Subject: RE: [PATCH v1] EmulatorPkg: Fix Terminal Issues > > Hi Mike, > > Unfortunately, that change will generate the following warning on > GCC4.6+ > > warning: variable "Success" set but not used [-Wunused-but-set- > variable] > > Hence why I wrote it that way. Let me know if you would like me to > make a different change before committing. > > Thanks, > Nate > > -----Original Message----- > From: Kinney, Michael D <michael.d.kinney@intel.com> > Sent: Tuesday, September 26, 2023 1:03 PM > To: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; > devel@edk2.groups.io > Cc: Andrew Fish <afish@apple.com>; Ni, Ray <ray.ni@intel.com>; Chiu, > Chasel <chasel.chiu@intel.com>; Kinney, Michael D > <michael.d.kinney@intel.com> > Subject: RE: [PATCH v1] EmulatorPkg: Fix Terminal Issues > > Thanks Nate! > > I have noticed this issue for a while. > > One comment below. With that update: > > Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com> > > Mike > > > -----Original Message----- > > From: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com> > > Sent: Tuesday, September 26, 2023 11:36 AM > > To: devel@edk2.groups.io > > Cc: Andrew Fish <afish@apple.com>; Ni, Ray <ray.ni@intel.com>; > Kinney, > > Michael D <michael.d.kinney@intel.com>; Chiu, Chasel > > <chasel.chiu@intel.com> > > Subject: [PATCH v1] EmulatorPkg: Fix Terminal Issues > > > > After running EmulatorPkg, one will notice that their terminal acts > > strangely. This is caused by the EmulatorPkg Host changing the > > terminal mode and not restoring the original mode, which is now > fixed. > > > > Cc: Andrew Fish <afish@apple.com> > > Cc: Ray Ni <ray.ni@intel.com> > > Cc: Michael D Kinney <michael.d.kinney@intel.com> > > Cc: Chasel Chiu <chasel.chiu@intel.com> > > Signed-off-by: Nate DeSimone <nathaniel.l.desimone@intel.com> > > --- > > EmulatorPkg/Unix/Host/EmuThunk.c | 16 ++++++++++++- > > EmulatorPkg/Win/Host/WinThunk.c | 40 > > +++++++++++++++++++++++++++++++- > > 2 files changed, 54 insertions(+), 2 deletions(-) > > > > diff --git a/EmulatorPkg/Unix/Host/EmuThunk.c > > b/EmulatorPkg/Unix/Host/EmuThunk.c > > index 6422f056a6..e6879db650 100644 > > --- a/EmulatorPkg/Unix/Host/EmuThunk.c > > +++ b/EmulatorPkg/Unix/Host/EmuThunk.c > > @@ -9,7 +9,7 @@ > > it may cause the table to be initialized with the members at the > > end being > > set to zero. This is bad as jumping to zero will crash. > > > > -Copyright (c) 2004 - 2019, Intel Corporation. All rights > > reserved.<BR> > > +Copyright (c) 2004 - 2023, Intel Corporation. All rights > > reserved.<BR> > > Portions copyright (c) 2008 - 2011, Apple Inc. All rights > > reserved.<BR> > > SPDX-License-Identifier: BSD-2-Clause-Patent > > > > @@ -34,6 +34,9 @@ UINTN settimer_callback = 0; > > > > BOOLEAN gEmulatorInterruptEnabled = FALSE; > > > > +STATIC BOOLEAN mEmulatorStdInConfigured = FALSE; STATIC struct > > +termios mOldTty; > > + > > UINTN > > SecWriteStdErr ( > > IN UINT8 *Buffer, > > @@ -58,8 +61,15 @@ SecConfigStdIn ( > > // Need to turn off line buffering, ECHO, and make it unbuffered. > > // > > tcgetattr (STDIN_FILENO, &tty); > > + if (!mEmulatorStdInConfigured) { > > + // > > + // Save the original state of the TTY so it can be restored on > > exit > > + // > > + CopyMem (&mOldTty, &tty, sizeof (struct termios)); } > > tty.c_lflag &= ~(ICANON | ECHO); > > tcsetattr (STDIN_FILENO, TCSANOW, &tty); > > + mEmulatorStdInConfigured = TRUE; > > > > // setvbuf (STDIN_FILENO, NULL, _IONBF, 0); > > > > @@ -338,6 +348,10 @@ SecExit ( > > UINTN Status > > ) > > { > > + // Reset the TTY back to its original state if > > + (mEmulatorStdInConfigured) { > > + tcsetattr (STDIN_FILENO, TCSANOW, &mOldTty); } > > exit (Status); > > } > > > > diff --git a/EmulatorPkg/Win/Host/WinThunk.c > > b/EmulatorPkg/Win/Host/WinThunk.c index 008e5755db..90a6da2ece > 100644 > > --- a/EmulatorPkg/Win/Host/WinThunk.c > > +++ b/EmulatorPkg/Win/Host/WinThunk.c > > @@ -1,6 +1,6 @@ > > /**@file > > > > -Copyright (c) 2006 - 2018, Intel Corporation. All rights > > reserved.<BR> > > +Copyright (c) 2006 - 2023, Intel Corporation. All rights > > reserved.<BR> > > SPDX-License-Identifier: BSD-2-Clause-Patent > > > > Module Name: > > @@ -30,6 +30,12 @@ Abstract: > > > > #include "WinHost.h" > > > > +STATIC BOOLEAN mEmulatorStdInConfigured = FALSE; STATIC DWORD > > +mOldStdInMode; #if defined (NTDDI_VERSION) && defined > > +(NTDDI_WIN10_TH2) && > > (NTDDI_VERSION > NTDDI_WIN10_TH2) > > + STATIC DWORD mOldStdOutMode; > > +#endif > > + > > UINTN > > SecWriteStdErr ( > > IN UINT8 *Buffer, > > @@ -61,6 +67,12 @@ SecConfigStdIn ( > > > > Success = GetConsoleMode (GetStdHandle (STD_INPUT_HANDLE), > &Mode); > > if (Success) { > > + if (!mEmulatorStdInConfigured) { > > + // > > + // Save the original state of the console so it can be > restored > > on exit > > + // > > + mOldStdInMode = Mode; > > + } > > // > > // Disable buffer (line input), echo, mouse, window > > // > > @@ -82,6 +94,12 @@ SecConfigStdIn ( > > // > > if (Success) { > > Success = GetConsoleMode (GetStdHandle (STD_OUTPUT_HANDLE), > > &Mode); > > + if (!mEmulatorStdInConfigured) { > > + // > > + // Save the original state of the console so it can be > restored > > on exit > > + // > > + mOldStdOutMode = Mode; > > + } > > if (Success) { > > Success = SetConsoleMode ( > > GetStdHandle (STD_OUTPUT_HANDLE), @@ -91,6 +109,9 > > @@ SecConfigStdIn ( > > } > > > > #endif > > + if (Success) { > > + mEmulatorStdInConfigured = TRUE; > > + } > > return Success ? EFI_SUCCESS : EFI_DEVICE_ERROR; } > > > > @@ -467,6 +488,23 @@ SecExit ( > > UINTN Status > > ) > > { > > + #if defined (NTDDI_VERSION) && defined (NTDDI_WIN10_TH2) && > > (NTDDI_VERSION > NTDDI_WIN10_TH2) > > + BOOL Success; > > + #endif > > + > > + if (mEmulatorStdInConfigured) { > > + // > > + // Reset the console back to its original state > > + // > > + #if defined (NTDDI_VERSION) && defined (NTDDI_WIN10_TH2) && > > (NTDDI_VERSION > NTDDI_WIN10_TH2) > > I think the BOOL Success variable could be added here and reduce the > #if statements > > > + Success = SetConsoleMode (GetStdHandle (STD_INPUT_HANDLE), > > mOldStdInMode); > > + if (Success) { > > + SetConsoleMode (GetStdHandle (STD_OUTPUT_HANDLE), > > mOldStdOutMode); > > + } > > + #else > > + SetConsoleMode (GetStdHandle (STD_INPUT_HANDLE), mOldStdInMode); > > + #endif } > > exit ((int)Status); > > } > > > > -- > > 2.34.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#109094): https://edk2.groups.io/g/devel/message/109094 Mute This Topic: https://groups.io/mt/101602599/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [PATCH v1] EmulatorPkg: Fix Terminal Issues 2023-09-26 21:42 ` Michael D Kinney @ 2023-09-27 2:59 ` Nate DeSimone 0 siblings, 0 replies; 7+ messages in thread From: Nate DeSimone @ 2023-09-27 2:59 UTC (permalink / raw) To: Kinney, Michael D, devel@edk2.groups.io Cc: Andrew Fish, Ni, Ray, Chiu, Chasel Thanks for the reminder/education on the coding style. I knew my habit of putting variables at the top of functions came from somewhere. Pushed: https://github.com/tianocore/edk2/commit/ad1c039 -----Original Message----- From: Kinney, Michael D <michael.d.kinney@intel.com> Sent: Tuesday, September 26, 2023 2:43 PM To: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; devel@edk2.groups.io Cc: Andrew Fish <afish@apple.com>; Ni, Ray <ray.ni@intel.com>; Chiu, Chasel <chasel.chiu@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com> Subject: RE: [PATCH v1] EmulatorPkg: Fix Terminal Issues Hi Nate, Declaring all the local variables at the top of the function is really a coding style recommendation documented in the EDK II C Coding Style Specification. End of Section 5.4.1.1. Declaring local in other scopes is strongly discouraged. https://tianocore-docs.github.io/edk2-CCodingStandardsSpecification/draft/5_source_files/54_code_file_structure.html#54-code-file-structure Block (local) Scope Formal parameters in function definitions and data defined within compound statements have block scope. Any group of statements that are encompassed within a pair of braces, { }, is a compound statement. The body of a function is also a compound statement. Compound statements can be nested, with each creating a new scope. Data declarations may follow the opening brace of a compound statement, regardless of nesting depth, and before any code generating statements have been entered. Other than at the outermost block of a function body, this type of declaration is strongly discouraged. This specific change here is in a windows specific file and improving readability of the windows specific version checks in #if statements warrants an exception. Mike > -----Original Message----- > From: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com> > Sent: Tuesday, September 26, 2023 1:51 PM > To: Kinney, Michael D <michael.d.kinney@intel.com>; > devel@edk2.groups.io > Cc: Andrew Fish <afish@apple.com>; Ni, Ray <ray.ni@intel.com>; Chiu, > Chasel <chasel.chiu@intel.com> > Subject: RE: [PATCH v1] EmulatorPkg: Fix Terminal Issues > > Looks like it compiles fine in VS2015, and we just removed the > tools_def entries for every VC++ version older than that. And C99 has > been in gcc for a very long time and clang forever. So perhaps we can > start using C99 style variable declarations finally? > > Thanks, > Nate > > -----Original Message----- > From: Desimone, Nathaniel L > Sent: Tuesday, September 26, 2023 1:39 PM > To: Kinney, Michael D <michael.d.kinney@intel.com>; > devel@edk2.groups.io > Cc: Andrew Fish <afish@apple.com>; Ni, Ray <ray.ni@intel.com>; Chiu, > Chasel <chasel.chiu@intel.com> > Subject: RE: [PATCH v1] EmulatorPkg: Fix Terminal Issues > > Oh, never mind, I see you are suggesting a C99 style variable > declaration. Do we need to worry about old compiler that want all > variable declarations at the top still? > > Thanks, > Nate > > -----Original Message----- > From: Desimone, Nathaniel L > Sent: Tuesday, September 26, 2023 1:38 PM > To: Kinney, Michael D <michael.d.kinney@intel.com>; > devel@edk2.groups.io > Cc: Andrew Fish <afish@apple.com>; Ni, Ray <ray.ni@intel.com>; Chiu, > Chasel <chasel.chiu@intel.com> > Subject: RE: [PATCH v1] EmulatorPkg: Fix Terminal Issues > > Hi Mike, > > Unfortunately, that change will generate the following warning on > GCC4.6+ > > warning: variable "Success" set but not used [-Wunused-but-set- > variable] > > Hence why I wrote it that way. Let me know if you would like me to > make a different change before committing. > > Thanks, > Nate > > -----Original Message----- > From: Kinney, Michael D <michael.d.kinney@intel.com> > Sent: Tuesday, September 26, 2023 1:03 PM > To: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; > devel@edk2.groups.io > Cc: Andrew Fish <afish@apple.com>; Ni, Ray <ray.ni@intel.com>; Chiu, > Chasel <chasel.chiu@intel.com>; Kinney, Michael D > <michael.d.kinney@intel.com> > Subject: RE: [PATCH v1] EmulatorPkg: Fix Terminal Issues > > Thanks Nate! > > I have noticed this issue for a while. > > One comment below. With that update: > > Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com> > > Mike > > > -----Original Message----- > > From: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com> > > Sent: Tuesday, September 26, 2023 11:36 AM > > To: devel@edk2.groups.io > > Cc: Andrew Fish <afish@apple.com>; Ni, Ray <ray.ni@intel.com>; > Kinney, > > Michael D <michael.d.kinney@intel.com>; Chiu, Chasel > > <chasel.chiu@intel.com> > > Subject: [PATCH v1] EmulatorPkg: Fix Terminal Issues > > > > After running EmulatorPkg, one will notice that their terminal acts > > strangely. This is caused by the EmulatorPkg Host changing the > > terminal mode and not restoring the original mode, which is now > fixed. > > > > Cc: Andrew Fish <afish@apple.com> > > Cc: Ray Ni <ray.ni@intel.com> > > Cc: Michael D Kinney <michael.d.kinney@intel.com> > > Cc: Chasel Chiu <chasel.chiu@intel.com> > > Signed-off-by: Nate DeSimone <nathaniel.l.desimone@intel.com> > > --- > > EmulatorPkg/Unix/Host/EmuThunk.c | 16 ++++++++++++- > > EmulatorPkg/Win/Host/WinThunk.c | 40 > > +++++++++++++++++++++++++++++++- > > 2 files changed, 54 insertions(+), 2 deletions(-) > > > > diff --git a/EmulatorPkg/Unix/Host/EmuThunk.c > > b/EmulatorPkg/Unix/Host/EmuThunk.c > > index 6422f056a6..e6879db650 100644 > > --- a/EmulatorPkg/Unix/Host/EmuThunk.c > > +++ b/EmulatorPkg/Unix/Host/EmuThunk.c > > @@ -9,7 +9,7 @@ > > it may cause the table to be initialized with the members at the > > end being > > set to zero. This is bad as jumping to zero will crash. > > > > -Copyright (c) 2004 - 2019, Intel Corporation. All rights > > reserved.<BR> > > +Copyright (c) 2004 - 2023, Intel Corporation. All rights > > reserved.<BR> > > Portions copyright (c) 2008 - 2011, Apple Inc. All rights > > reserved.<BR> > > SPDX-License-Identifier: BSD-2-Clause-Patent > > > > @@ -34,6 +34,9 @@ UINTN settimer_callback = 0; > > > > BOOLEAN gEmulatorInterruptEnabled = FALSE; > > > > +STATIC BOOLEAN mEmulatorStdInConfigured = FALSE; STATIC struct > > +termios mOldTty; > > + > > UINTN > > SecWriteStdErr ( > > IN UINT8 *Buffer, > > @@ -58,8 +61,15 @@ SecConfigStdIn ( > > // Need to turn off line buffering, ECHO, and make it unbuffered. > > // > > tcgetattr (STDIN_FILENO, &tty); > > + if (!mEmulatorStdInConfigured) { > > + // > > + // Save the original state of the TTY so it can be restored on > > exit > > + // > > + CopyMem (&mOldTty, &tty, sizeof (struct termios)); } > > tty.c_lflag &= ~(ICANON | ECHO); > > tcsetattr (STDIN_FILENO, TCSANOW, &tty); > > + mEmulatorStdInConfigured = TRUE; > > > > // setvbuf (STDIN_FILENO, NULL, _IONBF, 0); > > > > @@ -338,6 +348,10 @@ SecExit ( > > UINTN Status > > ) > > { > > + // Reset the TTY back to its original state if > > + (mEmulatorStdInConfigured) { > > + tcsetattr (STDIN_FILENO, TCSANOW, &mOldTty); } > > exit (Status); > > } > > > > diff --git a/EmulatorPkg/Win/Host/WinThunk.c > > b/EmulatorPkg/Win/Host/WinThunk.c index 008e5755db..90a6da2ece > 100644 > > --- a/EmulatorPkg/Win/Host/WinThunk.c > > +++ b/EmulatorPkg/Win/Host/WinThunk.c > > @@ -1,6 +1,6 @@ > > /**@file > > > > -Copyright (c) 2006 - 2018, Intel Corporation. All rights > > reserved.<BR> > > +Copyright (c) 2006 - 2023, Intel Corporation. All rights > > reserved.<BR> > > SPDX-License-Identifier: BSD-2-Clause-Patent > > > > Module Name: > > @@ -30,6 +30,12 @@ Abstract: > > > > #include "WinHost.h" > > > > +STATIC BOOLEAN mEmulatorStdInConfigured = FALSE; STATIC DWORD > > +mOldStdInMode; #if defined (NTDDI_VERSION) && defined > > +(NTDDI_WIN10_TH2) && > > (NTDDI_VERSION > NTDDI_WIN10_TH2) > > + STATIC DWORD mOldStdOutMode; > > +#endif > > + > > UINTN > > SecWriteStdErr ( > > IN UINT8 *Buffer, > > @@ -61,6 +67,12 @@ SecConfigStdIn ( > > > > Success = GetConsoleMode (GetStdHandle (STD_INPUT_HANDLE), > &Mode); > > if (Success) { > > + if (!mEmulatorStdInConfigured) { > > + // > > + // Save the original state of the console so it can be > restored > > on exit > > + // > > + mOldStdInMode = Mode; > > + } > > // > > // Disable buffer (line input), echo, mouse, window > > // > > @@ -82,6 +94,12 @@ SecConfigStdIn ( > > // > > if (Success) { > > Success = GetConsoleMode (GetStdHandle (STD_OUTPUT_HANDLE), > > &Mode); > > + if (!mEmulatorStdInConfigured) { > > + // > > + // Save the original state of the console so it can be > restored > > on exit > > + // > > + mOldStdOutMode = Mode; > > + } > > if (Success) { > > Success = SetConsoleMode ( > > GetStdHandle (STD_OUTPUT_HANDLE), @@ -91,6 +109,9 > > @@ SecConfigStdIn ( > > } > > > > #endif > > + if (Success) { > > + mEmulatorStdInConfigured = TRUE; } > > return Success ? EFI_SUCCESS : EFI_DEVICE_ERROR; } > > > > @@ -467,6 +488,23 @@ SecExit ( > > UINTN Status > > ) > > { > > + #if defined (NTDDI_VERSION) && defined (NTDDI_WIN10_TH2) && > > (NTDDI_VERSION > NTDDI_WIN10_TH2) > > + BOOL Success; > > + #endif > > + > > + if (mEmulatorStdInConfigured) { > > + // > > + // Reset the console back to its original state > > + // > > + #if defined (NTDDI_VERSION) && defined (NTDDI_WIN10_TH2) && > > (NTDDI_VERSION > NTDDI_WIN10_TH2) > > I think the BOOL Success variable could be added here and reduce the > #if statements > > > + Success = SetConsoleMode (GetStdHandle (STD_INPUT_HANDLE), > > mOldStdInMode); > > + if (Success) { > > + SetConsoleMode (GetStdHandle (STD_OUTPUT_HANDLE), > > mOldStdOutMode); > > + } > > + #else > > + SetConsoleMode (GetStdHandle (STD_INPUT_HANDLE), mOldStdInMode); > > + #endif } > > exit ((int)Status); > > } > > > > -- > > 2.34.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#109099): https://edk2.groups.io/g/devel/message/109099 Mute This Topic: https://groups.io/mt/101602599/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-09-27 2:59 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-09-26 18:36 [edk2-devel] [PATCH v1] EmulatorPkg: Fix Terminal Issues Nate DeSimone 2023-09-26 20:03 ` Michael D Kinney 2023-09-26 20:37 ` Nate DeSimone 2023-09-26 20:39 ` Nate DeSimone 2023-09-26 20:50 ` Nate DeSimone 2023-09-26 21:42 ` Michael D Kinney 2023-09-27 2:59 ` Nate DeSimone
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox