EmulatorPkg/Unix/Host/EmuThunk.c | 16 ++++++++++++- EmulatorPkg/Win/Host/WinThunk.c | 40 +++++++++++++++++++++++++++++++- 2 files changed, 54 insertions(+), 2 deletions(-)
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.