Re: [PATCH] Enable pager by default

[Javier Martinez Canillas]

Hello Daniel,

On 10/22/19 4:04 PM, Daniel Kiper wrote:
> On Tue, Oct 22, 2019 at 10:30:20AM +0200, Javier Martinez Canillas wrote:
>> Hello Daniel,
>>
>> On 10/21/19 4:56 PM, Daniel Kiper wrote:
>>> On Fri, Oct 18, 2019 at 02:43:18PM +0200, Javier Martinez Canillas wrote:
>>>> From: Peter Jones <address@hidden>
>>>>
>>>> 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.
>>>>
>>>> So just enable pager by default which leads to a much better experience.
>>>
>>> Hmmm... What will happen if a command produce tons of output during boot
>>> process? I am afraid that it will hang indefinitely waiting for an user
>>> input. This should not happen. So, I tend to agree that current help
>>> command behavior is annoying but I do not like the solution.
>>
>> Ok. I'll then explore having a paginated output only for the help command
>> instead of globally enabling it by default.
> 
> Great! Though I would think about something which can be used also in
> other commands producing a lot of output. Maybe we should introduce "-p"
> (pause) command line option for such commands. And I am not against
> using existing code to do a pause. We just have to do it carefully.
>

So I've tried different approaches to solves this. I'm not that sure about
introducing a -p option, since I think that we could have the same issue
of users not knowing that have to use this option to scroll the output.

Unless we print for each command a message after its output, telling users
that have to use -p if they want to have paginated output for that command.

What do you think about the following patch? That way we can specify if the
output for each command must be printed with the pager enabled or not.

Probably should be broken in 2 patches, but just want to know your opinion.

>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>

---

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);
+}
+
+void
+grub_disable_temp_more (void)
+{
+  if (!temp_more)
+    grub_set_more (0);
+}
+
 enum
   {
     GRUB_CP437_UPDOWNARROW     = 0x12,
diff --git a/grub-core/script/execute.c b/grub-core/script/execute.c
index ee299fd0ea6..8e837cf922b 100644
--- a/grub-core/script/execute.c
+++ b/grub-core/script/execute.c
@@ -1017,6 +1017,10 @@ grub_script_execute_cmdline (struct grub_script_cmd *cmd)
   /* Execute the GRUB command or function.  */
   if (grubcmd)
     {
+     /* Temporarily enable paginated output for commands that asked for it */
+      if (grubcmd->flags & GRUB_COMMAND_FLAG_PAGINATED)
+        grub_enable_temp_more ();
+
       if (grub_extractor_level && !(grubcmd->flags
                                    & GRUB_COMMAND_FLAG_EXTRACTOR))
        ret = grub_error (GRUB_ERR_EXTRACTOR,
@@ -1027,6 +1031,9 @@ grub_script_execute_cmdline (struct grub_script_cmd *cmd)
        ret = grub_extcmd_dispatcher (grubcmd, argc, args, argv.script);
       else
        ret = (grubcmd->func) (grubcmd, argc, args);
+
+      if (grubcmd->flags & GRUB_COMMAND_FLAG_PAGINATED)
+        grub_disable_temp_more ();
     }
   else
     ret = grub_script_function_call (func, argc, args);
diff --git a/include/grub/command.h b/include/grub/command.h
index eee4e847ee4..713445ac761 100644
--- a/include/grub/command.h
+++ b/include/grub/command.h
@@ -37,7 +37,9 @@ typedef enum grub_command_flags
     /* This command accepts only options preceding direct arguments.  */
     GRUB_COMMAND_OPTIONS_AT_START = 0x100,
     /* Can be executed in an entries extractor.  */
-    GRUB_COMMAND_FLAG_EXTRACTOR = 0x200
+    GRUB_COMMAND_FLAG_EXTRACTOR = 0x200,
+    /* This command output must be paginated. */
+    GRUB_COMMAND_FLAG_PAGINATED = 0x400
   } grub_command_flags_t;
 
 struct grub_command;
diff --git a/include/grub/normal.h b/include/grub/normal.h
index 218cbabccaf..26d6b47d263 100644
--- a/include/grub/normal.h
+++ b/include/grub/normal.h
@@ -134,6 +134,9 @@ void read_terminal_list (const char *prefix);
 
 void grub_set_more (int onoff);
 
+void grub_enable_temp_more (void);
+void grub_disable_temp_more (void);
+
 void grub_normal_reset_more (void);
 
 void grub_xputs_normal (const char *str);
-- 
2.21.0

-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat