* [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