public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [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