Re: [PATCH 1/2] EFI: console: Do not set colorstate until the first text output

[Robbie Harwood <rharwood@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2257 bytes --]

Javier Martinez Canillas <javierm@redhat.com> writes:

> Hello Hans,
>
> Thanks for the patch.
>
> On 1/28/22 12:43, Hans de Goede wrote:
>> GRUB_MOD_INIT(normal) does an unconditional:
>> 
>> grub_env_set ("color_normal", "light-gray/black");
>> 
>> which triggers a grub_term_setcolorstate() call. The original version
>> of the "efi/console: Do not set text-mode until we actually need it" patch:
>> https://lists.gnu.org/archive/html/grub-devel/2018-03/msg00125.html
>> 
>> Protected against this by caching the requested state in
>> grub_console_setcolorstate () and then only applying it when the first
>> text output actually happens. During refactoring to move the
>> grub_console_setcolorstate () up higher in the grub-core/term/efi/console.c
>> file the code to cache the color-state + bail early was accidentally
>> dropped.
>> 
>> Restore the cache the color-state + bail early behavior from the original.
>> 
>> Cc: Javier Martinez Canillas <javierm@redhat.com>
>> Fixes: 2d7c3abd871f ("efi/console: Do not set text-mode until we actually need it")
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  grub-core/term/efi/console.c | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>> 
>> diff --git a/grub-core/term/efi/console.c b/grub-core/term/efi/console.c
>> index 2f1ae85ba..c44b2ac31 100644
>> --- a/grub-core/term/efi/console.c
>> +++ b/grub-core/term/efi/console.c
>> @@ -82,6 +82,16 @@ grub_console_setcolorstate (struct grub_term_output *term
>>  {
>>    grub_efi_simple_text_output_interface_t *o;
>>  
>> +  if (grub_efi_is_finished || text_mode != GRUB_TEXT_MODE_AVAILABLE)
>> +    {
>> +      /*
>> +       * Cache colorstate changes before the first text-output, this avoids
>> +       * "color_normal" environment writes causing a switch to textmode.
>> +       */
>> +      text_colorstate = state;
>> +      return;
>> +    }
>> +
>>    if (grub_efi_is_finished)
>>      return;
>>
>
> Indeed, sorry for messing this when addressing the concerns raised by
> Daniel in your original patch. The fix looks good to me.
>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Reviewed-by: Robbie Harwood <rharwood@redhat.com>

Be well,
--Robbie

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]