Re: [PATCH] Enable pager by default

[Javier Martinez Canillas]

Hello Daniel,

On 10/24/19 4:50 PM, Daniel Kiper wrote:

[snip]

>>
>> From 7c4da6295ebd3a034d1f7e32099eab33efa465d4 Mon Sep 17 00:00:00 2001
>> From: Javier Martinez Canillas <address@hidden>
>> Date: Tue, 22 Oct 2019 15:35:12 +0200
>> Subject: [PATCH v2] Add a GRUB_COMMAND_FLAG_PAGINATED to request paginated
>>  output for commands
>>
>> When user enters into the GRUB shell and tries to use help command, lot of
>> information is scrolled out of screen and the user doesn't have chance to
>> read it. Also, there isn't any information about 'set pager=1' at the end
>> of the help output, to tell the user how scrolling could be enabled.
>>
>> Since the out for some commands may not fit into a screen, add a new flag
>> GRUB_COMMAND_FLAG_PAGINATED that can be used when registering commands that
>> can be used to request the pager to be enabled while executing the handler.
>>
>> Signed-off-by: Javier Martinez Canillas <address@hidden>
> 
> In general I like the idea but I think patch requires some polishing...
> 
> Hmmm... Still thinking about "-p" flag which allows user to choose
> between pager on/off. Or something which I proposed in the email to
> Michael...
>

I'm OK with any of the other two approaches too. But it seems that Vladimir
is worried about the added complexity for these.

I honestly think that the approach in this patch is the least bad option since
it doesn't require a special configuration or the user to do anything, just
execute the command printing a lot of stuff and get paginated output by default.

Can't think why users would want to execute commands that print a lot of 
messages
without the output being paginated, or why they would want to execute a command
like help in in batch mode. It's only useful in interactive mode.

And also the patch is quite trivial, it already uses all the existing code for
the pager option.

But I'll wait for the discussion to settle and a solution to be agreed upon,
before posting a new patch for this.

>> ---
>>
>> Changes in v2:
>> - Only enable pager temporarily while printing output for commands that asked
>>   for paginated output instead of doing it globally using the pager variable.
>>
>>  grub-core/commands/help.c    |  3 ++-
>>  grub-core/commands/minicmd.c | 13 ++++++++-----
>>  grub-core/normal/dyncmd.c    |  7 +++++++
>>  grub-core/normal/term.c      | 17 +++++++++++++++++
>>  grub-core/script/execute.c   |  7 +++++++
>>  include/grub/command.h       |  4 +++-
>>  include/grub/normal.h        |  3 +++
>>  7 files changed, 47 insertions(+), 7 deletions(-)
>>
>> diff --git a/grub-core/commands/help.c b/grub-core/commands/help.c
>> index f0be89baa19..6e20e1447a8 100644
>> --- a/grub-core/commands/help.c
>> +++ b/grub-core/commands/help.c
>> @@ -142,7 +142,8 @@ static grub_extcmd_t cmd;
>>  
>>  GRUB_MOD_INIT(help)
>>  {
>> -  cmd = grub_register_extcmd ("help", grub_cmd_help, 0,
>> +  cmd = grub_register_extcmd ("help", grub_cmd_help,
>> +                          GRUB_COMMAND_FLAG_PAGINATED,
>>                            N_("[PATTERN ...]"),
>>                            N_("Show a help message."), 0);
>>  }
>> diff --git a/grub-core/commands/minicmd.c b/grub-core/commands/minicmd.c
>> index 6bbce3128cf..e6a9242bfa6 100644
>> --- a/grub-core/commands/minicmd.c
>> +++ b/grub-core/commands/minicmd.c
>> @@ -21,6 +21,7 @@
>>  #include <grub/mm.h>
>>  #include <grub/err.h>
>>  #include <grub/env.h>
>> +#include <grub/extcmd.h>
>>  #include <grub/misc.h>
>>  #include <grub/file.h>
>>  #include <grub/disk.h>
>> @@ -75,7 +76,7 @@ grub_mini_cmd_cat (struct grub_command *cmd __attribute__ 
>> ((unused)),
>>
>>  /* help */
>>  static grub_err_t
>> -grub_mini_cmd_help (struct grub_command *cmd __attribute__ ((unused)),
>> +grub_mini_cmd_help (grub_extcmd_context_t ctxt __attribute__ ((unused)),
>>                  int argc __attribute__ ((unused)),
>>                  char *argv[] __attribute__ ((unused)))
>>  {
>> @@ -188,7 +189,8 @@ grub_mini_cmd_exit (struct grub_command *cmd 
>> __attribute__ ((unused)),
>>    /* Not reached.  */
>>  }
>>
>> -static grub_command_t cmd_cat, cmd_help;
>> +static grub_command_t cmd_cat;
>> +static grub_extcmd_t cmd_help;
>>  static grub_command_t cmd_dump, cmd_rmmod, cmd_lsmod, cmd_exit;
>>
>>  GRUB_MOD_INIT(minicmd)
>> @@ -197,8 +199,9 @@ GRUB_MOD_INIT(minicmd)
>>      grub_register_command ("cat", grub_mini_cmd_cat,
>>                         N_("FILE"), N_("Show the contents of a file."));
>>    cmd_help =
>> -    grub_register_command ("help", grub_mini_cmd_help,
>> -                       0, N_("Show this message."));
>> +    grub_register_extcmd ("help", grub_mini_cmd_help,
>> +                          GRUB_COMMAND_FLAG_PAGINATED,
>> +                          0, N_("Show this message."), 0);
>>    cmd_dump =
>>      grub_register_command ("dump", grub_mini_cmd_dump,
>>                         N_("ADDR [SIZE]"), N_("Show memory contents."));
>> @@ -216,7 +219,7 @@ GRUB_MOD_INIT(minicmd)
>>  GRUB_MOD_FINI(minicmd)
>>  {
>>    grub_unregister_command (cmd_cat);
>> -  grub_unregister_command (cmd_help);
>> +  grub_unregister_extcmd (cmd_help);
>>    grub_unregister_command (cmd_dump);
>>    grub_unregister_command (cmd_rmmod);
>>    grub_unregister_command (cmd_lsmod);
>> diff --git a/grub-core/normal/dyncmd.c b/grub-core/normal/dyncmd.c
>> index 719ebf477f2..1ef52cbe43b 100644
>> --- a/grub-core/normal/dyncmd.c
>> +++ b/grub-core/normal/dyncmd.c
>> @@ -78,11 +78,18 @@ grub_dyncmd_dispatcher (struct grub_extcmd_context *ctxt,
>>    cmd = grub_command_find (name);
>>    if (cmd)
>>      {
>> +      /* Temporarily enable paginated output for commands that asked for it 
>> */
>> +      if (cmd->flags & GRUB_COMMAND_FLAG_PAGINATED)
>> +        grub_enable_temp_more ();
>> +
>>        if (cmd->flags & GRUB_COMMAND_FLAG_BLOCKS &&
>>        cmd->flags & GRUB_COMMAND_FLAG_EXTCMD)
>>      ret = grub_extcmd_dispatcher (cmd, argc, args, ctxt->script);
>>        else
>>      ret = (cmd->func) (cmd, argc, args);
>> +
>> +      if (cmd->flags & GRUB_COMMAND_FLAG_PAGINATED)
>> +        grub_disable_temp_more ();
>>      }
>>    else
>>      ret = grub_errno;
>> diff --git a/grub-core/normal/term.c b/grub-core/normal/term.c
>> index a1e5c5a0daf..7d4021ff8be 100644
>> --- a/grub-core/normal/term.c
>> +++ b/grub-core/normal/term.c
>> @@ -57,6 +57,7 @@ static struct term_state *term_states = NULL;
>>
>>  /* If the more pager is active.  */
>>  static int grub_more;
>> +static int temp_more;
>>
>>  static void
>>  putcode_real (grub_uint32_t code, struct grub_term_output *term, int 
>> fixed_tab);
>> @@ -128,6 +129,22 @@ grub_set_more (int onoff)
>>    grub_normal_reset_more ();
>>  }
>>
>> +void
>> +grub_enable_temp_more (void)
>> +{
>> +  temp_more = grub_more;
>> +
>> +  if (!temp_more)
>> +    grub_set_more (1);
> 
> Does it change pager variable value? If yes I think
> that you should store its original value here and
> restore below.
>

Yes it does and that's why I added the temp_more variable.

It only calls to gru_set_more() if pager is disabled, since if is already
enabled, then there's no need to call grub_set_more(1).

>> +}
>> +
>> +void
>> +grub_disable_temp_more (void)
>> +{
>> +  if (!temp_more)
>> +    grub_set_more (0);
>

Same here, the pager only needs to be disabled if it wasn't enabled before.

> Daniel
> 

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat