Re: [PATCH v2 1/2] efi: Add efitextmode command for getting/setting the text mode resolution

[Daniel Kiper]

On Fri, May 13, 2022 at 12:54:11PM -0500, Glenn Washburn wrote:
> This command is meant to behave similarly to the 'mode' command of the EFI
> Shell application. One difference is that to set the mode the mode number
> is given, not the rows and columns of the desired mode. Also supported are
> the arguments "min" and "max", which set the mode to the minimum and
> maximum mode respectively as calculated by the columns * rows of that mode.
>
> Signed-off-by: Glenn Washburn <development@efficientek.com>
> Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>
> Tested-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  grub-core/Makefile.core.def          |   6 ++
>  grub-core/commands/efi/efitextmode.c | 118 +++++++++++++++++++++++++++
>  2 files changed, 124 insertions(+)
>  create mode 100644 grub-core/commands/efi/efitextmode.c
>
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index 726f51be7..b22e48f0f 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -813,6 +813,12 @@ module = {
>    enable = efi;
>  };
>
> +module = {
> +  name = efitextmode;
> +  efi = commands/efi/efitextmode.c;
> +  enable = efi;
> +};
> +
>  module = {
>    name = blocklist;
>    common = commands/blocklist.c;
> diff --git a/grub-core/commands/efi/efitextmode.c 
> b/grub-core/commands/efi/efitextmode.c
> new file mode 100644
> index 000000000..fb72aa6f3
> --- /dev/null
> +++ b/grub-core/commands/efi/efitextmode.c
> @@ -0,0 +1,118 @@
> +/* efitextmode.c - command to get/set text mode resolution */
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2003,2005,2006,2007,2009  Free Software Foundation, Inc.

s/2003,2005,2006,2007,2009/2022/

> + *  GRUB is free software: you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation, either version 3 of the License, or
> + *  (at your option) any later version.
> + *
> + *  GRUB is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.

Please add one line here describing what this module does. Good example
is in grub-core/kern/efi/sb.c.

> + */
> +
> +#include <grub/dl.h>
> +#include <grub/misc.h>
> +#include <grub/mm.h>
> +#include <grub/command.h>
> +#include <grub/i18n.h>
> +#include <grub/efi/efi.h>
> +#include <grub/efi/api.h>
> +
> +GRUB_MOD_LICENSE ("GPLv3+");
> +
> +static grub_err_t
> +grub_cmd_efitextmode (grub_command_t cmd __attribute__ ((unused)),
> +                   int argc, char **args)
> +{
> +  grub_efi_simple_text_output_interface_t *o;
> +  unsigned long mode;
> +  const char *p = NULL;
> +  grub_efi_status_t status;
> +  grub_efi_uintn_t columns, rows;
> +  grub_efi_int32_t i;
> +
> +  if (argc > 1)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("at most one argument 
> expected"));
> +
> +  o = grub_efi_system_table->con_out;

I think you can do it in definition above.

> +
> +  if (argc == 1)
> +    {
> +      if (grub_strcmp (args[0], "min") == 0)
> +     mode = 0;
> +      else if (grub_strcmp (args[0], "max") == 0)
> +     mode = o->mode->max_mode - 1;

Are your sure "o" and "o->mode" are always not NULL?

> +      else
> +     {
> +       mode = grub_strtoul (args[0], &p, 0);
> +
> +       if (grub_errno != GRUB_ERR_NONE)
> +         return grub_errno;

Please drop this check. It is redundant as we chatted earlier.

> +       if (*args[0] == '\0' || *p != '\0')
> +         return grub_error (GRUB_ERR_BAD_ARGUMENT,
> +                            N_("non-numeric or invalid mode `%s'"),
> +                            args[0]);
> +     }
> +
> +      if (mode < (unsigned long) o->mode->max_mode)
> +     {
> +       if (mode != (unsigned long) o->mode->mode)
> +         {
> +           status = efi_call_2 (o->set_mode, o, (grub_efi_int32_t) mode);
> +           if (status == GRUB_EFI_SUCCESS)
> +             ;
> +           else if (status == GRUB_EFI_DEVICE_ERROR)
> +             return grub_error (GRUB_ERR_BAD_DEVICE,
> +                                N_("device error: could not set requested"
> +                                   " mode"));

Please do not wrap error msg here. I am OK with lines a bit longer than
80 chars. And I think you can put args[0] in one line with error msg
above too.

> +           else if (status == GRUB_EFI_UNSUPPORTED)
> +             return grub_error (GRUB_ERR_OUT_OF_RANGE,
> +                                N_("invalid mode: number not valid"));
> +           else
> +             return grub_error (GRUB_ERR_BAD_OS,
> +                                N_("unexpected EFI error number: `%u'"),
> +                                (unsigned) status);
> +         }
> +     }
> +      else
> +     return grub_error (GRUB_ERR_BAD_ARGUMENT,
> +                        N_("invalid mode: `%lu' is greater than maximum"
> +                           " mode `%lu'"),

Ditto.

> +                        mode, (unsigned long) o->mode->max_mode);
> +    }
> +
> +  if (argc == 0)
> +    {
> +      grub_printf_ (N_("Available modes for console output device.\n"));
> +
> +      for (i=0; i < o->mode->max_mode; i++)

i = 0; Missing spaces...

> +     if (GRUB_EFI_SUCCESS == efi_call_4 (o->query_mode, o, i,
> +                                         &columns, &rows))
> +       grub_printf_ (N_(" [%lu]  Col %5u Row %5u %c\n"),
> +                     (unsigned long) i, (unsigned) columns, (unsigned) rows,

Could we avoid this casts? Same above. I think we can introduce
PRIxGRUB_EFI_UINT32_T, etc. They should land in include/grub/efi/api.h
somewhere around grub_efi_uint*_t typedefs.

> +                     (i == o->mode->mode) ? '*' : ' ');
> +    }
> +
> +  return GRUB_ERR_NONE;
> +}
> +
> +static grub_command_t cmd;
> +

Please drop this ^L.

> +GRUB_MOD_INIT(cmp)

GRUB_MOD_INIT (cmp)
> +{
> +  cmd = grub_register_command ("efitextmode", grub_cmd_efitextmode,
> +                            N_("[min|max|mode_num]"), N_("Get or set EFI 
> text mode."));
> +}
> +
> +GRUB_MOD_FINI(cmp)

GRUB_MOD_FINI (cmp)

> +{
> +  grub_unregister_command (cmd);
> +}

Daniel