Re: [PATCH 4/4] EFI: Do not set text-mode until we actually need it

[Daniel Kiper]

On Thu, Apr 05, 2018 at 08:05:47PM +0200, Hans de Goede wrote:
> Hi,
>
> On 05-04-18 14:34, Daniel Kiper wrote:
> >On Wed, Mar 28, 2018 at 04:50:28PM +0200, Hans de Goede wrote:
> >>If we're running with a hidden menu we may never need text mode, so do not
> >>change the video-mode to text until we actually need it.
> >>
> >>Signed-off-by: Hans de Goede <address@hidden>
> >>---
> >>  grub-core/term/efi/console.c | 72 +++++++++++++++++++++++-------------
> >>  1 file changed, 47 insertions(+), 25 deletions(-)
> >>
> >>diff --git a/grub-core/term/efi/console.c b/grub-core/term/efi/console.c
> >>index 02f64ea74..d9fd7cf48 100644
> >>--- a/grub-core/term/efi/console.c
> >>+++ b/grub-core/term/efi/console.c
> >>@@ -24,6 +24,11 @@
> >>  #include <grub/efi/api.h>
> >>  #include <grub/efi/console.h>
> >>
> >>+static grub_err_t grub_prepare_for_text_output(struct grub_term_output 
> >>*term);
> >
> >Please drop this forward declaration and put the function definition before 
> >the callers.
>
> I did not do that initially because that requires also moving
> grub_console_setcolorstate() and grub_console_setcursor() to
> higher in the file (or do a forward declaration for those).
>
> I'll add a preparation patch to v2 of the series moving just
> those 2 up to higher in the file.

Great!

> >>+static int text_mode_available = -1;
> >
> >Could you use bool type here? If yes please define grub_bool_t as stdbool.h
> >does (grub/bool.h?) and replace its usage in grub-core/term/tparm.c as 
> >separate
> >patch. If not bool please use enum here.
>
> There are 3 states, so a bool is not going to work I will
> switch to an enum for v2.

Perfect!

> >>+static int text_colorstate = -1;
>
> There already is a grub_term_color_state enum for this, which
> defines values 0-2, I want to know if setcolorstate has been
> called and text_colorstate contains a valid value, so I made
> this an int and use -1 to mean not set / invalid.
>
> I can see a number of solutions here:
>
> 1) Leave as is
> 2) Use:
>
> static grub_term_color_state text_colorstate = -1;
>
> Assuming the compiler will not warn about putting -1 in there.
>
> 3) Extend the grub_term_color_state like this:
>
> typedef enum
>   {
>     /* Used for uninitialized grub_term_color_state variables */
>     GRUB_TERM_COLOR_UNDEFINED = -1,
>     /* The color used to display all text that does not use the
>        user defined colors below.  */
>     GRUB_TERM_COLOR_STANDARD = 0,
>     /* The user defined colors for normal text.  */
>     GRUB_TERM_COLOR_NORMAL,
>     /* The user defined colors for highlighted text.  */
>     GRUB_TERM_COLOR_HIGHLIGHT
>   }
> grub_term_color_state;
>
> Which solution would you prefer?

3rd, enum please.

Thanks,

Daniel