Re: [edk2-devel] [PATCH v1] EmulatorPkg: Fix Terminal Issues
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
Sent: Tuesday, September 26, 2023 2:43 PM
To: Desimone, Nathaniel L ; devel@edk2.groups.io
Cc: Andrew Fish ; Ni, Ray ; Chiu, Chasel
; Kinney, Michael D
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
> Sent: Tuesday, September 26, 2023 1:51 PM
> To: Kinney, Michael D ;
> devel@edk2.groups.io
> Cc: Andrew Fish ; Ni, Ray ; Chiu,
> Chasel
> 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 ;
> devel@edk2.groups.io
> Cc: Andrew Fish ; Ni, Ray ; Chiu,
> Chasel
> 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 ;
> devel@edk2.groups.io
> Cc: Andrew Fish ; Ni, Ray ; Chiu,
> Chasel
> 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
> Sent: Tuesday, September 26, 2023 1:03 PM
> To: Desimone, Nathaniel L ;
> devel@edk2.groups.io
> Cc: Andrew Fish ; Ni, Ray ; Chiu,
> Chasel ; Kinney, Michael D
>
> 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
>
> Mike
>
> > -Original Message-
> > From: Desimone, Nathaniel L
> > Sent: Tuesday, September 26, 2023 11:36 AM
> > To: devel@edk2.groups.io
> > Cc: Andrew Fish ; Ni, Ray ;
> Kinney,
> > Michael D ; Chiu, Chasel
> >
> > 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
> > Cc: Ray Ni
> > Cc: Michael D Kinney
> > Cc: Chasel Chiu
> > Signed-off-by: Nate DeSimone
> > ---
> > 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
> >
Re: [edk2-devel] [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
> Sent: Tuesday, September 26, 2023 1:51 PM
> To: Kinney, Michael D ;
> devel@edk2.groups.io
> Cc: Andrew Fish ; Ni, Ray ; Chiu,
> Chasel
> 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 ;
> devel@edk2.groups.io
> Cc: Andrew Fish ; Ni, Ray ; Chiu,
> Chasel
> 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 ;
> devel@edk2.groups.io
> Cc: Andrew Fish ; Ni, Ray ; Chiu,
> Chasel
> 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
> Sent: Tuesday, September 26, 2023 1:03 PM
> To: Desimone, Nathaniel L ;
> devel@edk2.groups.io
> Cc: Andrew Fish ; Ni, Ray ; Chiu,
> Chasel ; Kinney, Michael D
>
> 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
>
> Mike
>
> > -Original Message-
> > From: Desimone, Nathaniel L
> > Sent: Tuesday, September 26, 2023 11:36 AM
> > To: devel@edk2.groups.io
> > Cc: Andrew Fish ; Ni, Ray ;
> Kinney,
> > Michael D ; Chiu, Chasel
> >
> > 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
> > Cc: Ray Ni
> > Cc: Michael D Kinney
> > Cc: Chasel Chiu
> > Signed-off-by: Nate DeSimone
> > ---
> > 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.
> > +Copyright (c) 2004 - 2023, Intel Corporation. All rights
> > reserved.
> > Portions copyright (c) 2008 - 2011, Apple Inc. All rights
> > reserved.
> > 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
Re: [edk2-devel] [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 ; devel@edk2.groups.io
Cc: Andrew Fish ; Ni, Ray ; Chiu, Chasel
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 ; devel@edk2.groups.io
Cc: Andrew Fish ; Ni, Ray ; Chiu, Chasel
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
Sent: Tuesday, September 26, 2023 1:03 PM
To: Desimone, Nathaniel L ; devel@edk2.groups.io
Cc: Andrew Fish ; Ni, Ray ; Chiu, Chasel
; Kinney, Michael D
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
Mike
> -Original Message-
> From: Desimone, Nathaniel L
> Sent: Tuesday, September 26, 2023 11:36 AM
> To: devel@edk2.groups.io
> Cc: Andrew Fish ; Ni, Ray ; Kinney,
> Michael D ; Chiu, Chasel
>
> 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
> Cc: Ray Ni
> Cc: Michael D Kinney
> Cc: Chasel Chiu
> Signed-off-by: Nate DeSimone
> ---
> 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.
> +Copyright (c) 2004 - 2023, Intel Corporation. All rights
> reserved.
> Portions copyright (c) 2008 - 2011, Apple Inc. All rights
> reserved.
> 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, );
> + if (!mEmulatorStdInConfigured) {
> +//
> +// Save the original state of the TTY so it can be restored on
> exit
> +//
> +CopyMem (, , sizeof (struct termios)); }
>tty.c_lflag &= ~(ICANON | ECHO);
>tcsetattr (STDIN_FILENO, TCSANOW, );
> + 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, ); }
>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.
> +Copyright (c) 2006 - 2023, Intel Corporation. All rights
> reserved.
> 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), );
>if (Success) {
> +if (!mEmulatorStdInConfigured) {
> + //
Re: [edk2-devel] [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 ; devel@edk2.groups.io
Cc: Andrew Fish ; Ni, Ray ; Chiu, Chasel
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
Sent: Tuesday, September 26, 2023 1:03 PM
To: Desimone, Nathaniel L ; devel@edk2.groups.io
Cc: Andrew Fish ; Ni, Ray ; Chiu, Chasel
; Kinney, Michael D
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
Mike
> -Original Message-
> From: Desimone, Nathaniel L
> Sent: Tuesday, September 26, 2023 11:36 AM
> To: devel@edk2.groups.io
> Cc: Andrew Fish ; Ni, Ray ; Kinney,
> Michael D ; Chiu, Chasel
>
> 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
> Cc: Ray Ni
> Cc: Michael D Kinney
> Cc: Chasel Chiu
> Signed-off-by: Nate DeSimone
> ---
> 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.
> +Copyright (c) 2004 - 2023, Intel Corporation. All rights
> reserved.
> Portions copyright (c) 2008 - 2011, Apple Inc. All rights
> reserved.
> 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, );
> + if (!mEmulatorStdInConfigured) {
> +//
> +// Save the original state of the TTY so it can be restored on
> exit
> +//
> +CopyMem (, , sizeof (struct termios)); }
>tty.c_lflag &= ~(ICANON | ECHO);
>tcsetattr (STDIN_FILENO, TCSANOW, );
> + 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, ); }
>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.
> +Copyright (c) 2006 - 2023, Intel Corporation. All rights
> reserved.
> 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), );
>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),
> );
> +if (!mEmulatorStdInConfigured) {
> + //
> + // Save the original state of the console so it can be restored
> on exit
> + //
> +
Re: [edk2-devel] [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
Sent: Tuesday, September 26, 2023 1:03 PM
To: Desimone, Nathaniel L ; devel@edk2.groups.io
Cc: Andrew Fish ; Ni, Ray ; Chiu, Chasel
; Kinney, Michael D
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
Mike
> -Original Message-
> From: Desimone, Nathaniel L
> Sent: Tuesday, September 26, 2023 11:36 AM
> To: devel@edk2.groups.io
> Cc: Andrew Fish ; Ni, Ray ; Kinney,
> Michael D ; Chiu, Chasel
>
> 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
> Cc: Ray Ni
> Cc: Michael D Kinney
> Cc: Chasel Chiu
> Signed-off-by: Nate DeSimone
> ---
> 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.
> +Copyright (c) 2004 - 2023, Intel Corporation. All rights
> reserved.
> Portions copyright (c) 2008 - 2011, Apple Inc. All rights
> reserved.
> 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, );
> + if (!mEmulatorStdInConfigured) {
> +//
> +// Save the original state of the TTY so it can be restored on
> exit
> +//
> +CopyMem (, , sizeof (struct termios)); }
>tty.c_lflag &= ~(ICANON | ECHO);
>tcsetattr (STDIN_FILENO, TCSANOW, );
> + 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, ); }
>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.
> +Copyright (c) 2006 - 2023, Intel Corporation. All rights
> reserved.
> 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), );
>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),
> );
> +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) &&
Re: [edk2-devel] [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
Mike
> -Original Message-
> From: Desimone, Nathaniel L
> Sent: Tuesday, September 26, 2023 11:36 AM
> To: devel@edk2.groups.io
> Cc: Andrew Fish ; Ni, Ray ; Kinney,
> Michael D ; Chiu, Chasel
>
> 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
> Cc: Ray Ni
> Cc: Michael D Kinney
> Cc: Chasel Chiu
> Signed-off-by: Nate DeSimone
> ---
> 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.
> +Copyright (c) 2004 - 2023, Intel Corporation. All rights
> reserved.
> Portions copyright (c) 2008 - 2011, Apple Inc. All rights
> reserved.
> 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, );
> + if (!mEmulatorStdInConfigured) {
> +//
> +// Save the original state of the TTY so it can be restored on
> exit
> +//
> +CopyMem (, , sizeof (struct termios));
> + }
>tty.c_lflag &= ~(ICANON | ECHO);
>tcsetattr (STDIN_FILENO, TCSANOW, );
> + 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, );
> + }
>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.
> +Copyright (c) 2006 - 2023, Intel Corporation. All rights
> reserved.
> 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), );
>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),
> );
> +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
