[PATCH] Enable pager by default

[PATCH] Enable pager by default

[Javier Martinez Canillas]

From: Peter Jones <pjones@redhat.com>

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.

Signed-off-by: Peter Jones <pjones@redhat.com>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

 util/grub.d/00_header.in | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/util/grub.d/00_header.in b/util/grub.d/00_header.in
index 93a90233ead..858b526c925 100644
--- a/util/grub.d/00_header.in
+++ b/util/grub.d/00_header.in
@@ -43,6 +43,8 @@ if [ "x${GRUB_DEFAULT_BUTTON}" = "xsaved" ] ; then GRUB_DEFAULT_BUTTON='${saved_
 if [ "x${GRUB_TIMEOUT_BUTTON}" = "x" ] ; then GRUB_TIMEOUT_BUTTON="$GRUB_TIMEOUT" ; fi
 
 cat << EOF
+set pager=1
+
 if [ -s \$prefix/grubenv ]; then
   load_env
 fi
-- 
2.21.0

Re: [PATCH] Enable pager by default

[Daniel Kiper]

On Fri, Oct 18, 2019 at 02:43:18PM +0200, Javier Martinez Canillas wrote:
> From: Peter Jones <pjones@redhat.com>
>
> 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.

Daniel

Re: [PATCH] Enable pager by default

[Javier Martinez Canillas]

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 <pjones@redhat.com>
>>
>> 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.
 
> Daniel
> 

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

Re: [PATCH] Enable pager by default

[Daniel Kiper]

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 <pjones@redhat.com>
> >>
> >> 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.

Daniel

Re: [PATCH] Enable pager by default

[Nicholas Vinson]

On 10/22/19 10:04, 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 <pjones@redhat.com>
>>>>
>>>> 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.

I think default paging behavior could be handled by an environment 
variable with paging disabled by default. GRUB's console could then 
print a very short message instructing people to set the variable to 
enable command paging.  I believe this would handle both concerns.

Nicholas Vinson

> 
> Daniel
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
> 

Re: [PATCH] Enable pager by default

[Daniel Kiper]

On Tue, Oct 22, 2019 at 09:50:10PM -0400, Nicholas Vinson wrote:
> On 10/22/19 10:04, 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 <pjones@redhat.com>
> > > > >
> > > > > 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.
>
> I think default paging behavior could be handled by an environment variable
> with paging disabled by default. GRUB's console could then print a very

Well, it is handled in that way. So?

> short message instructing people to set the variable to enable command
> paging.  I believe this would handle both concerns.

This is an option too. Though I am not convinced to change the global
variable to change the behavior of one command.

Daniel

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 <pjones@redhat.com>
>>>>
>>>> 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 <javierm@redhat.com>
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 <javierm@redhat.com>

---

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;
 \f
 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

Re: [PATCH] Enable pager by default

[Michael Chang]

On Tue, Oct 22, 2019 at 04:04:28PM +0200, 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 <pjones@redhat.com>
> > >>
> > >> 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.

I'd like to add option to the list, which is grub could provide the
information to have the commands able to tell they are executed in
shell's interactive (aka command-line) or batch mode. After they could
turn on/off paginated output according to the shell mode they are with.

Thanks,
Michael

> 
> Daniel
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] Enable pager by default

[Daniel Kiper]

On Thu, Oct 24, 2019 at 06:54:53AM +0000, Michael Chang wrote:
> On Tue, Oct 22, 2019 at 04:04:28PM +0200, 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 <pjones@redhat.com>
> > > >>
> > > >> 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.
>
> I'd like to add option to the list, which is grub could provide the
> information to have the commands able to tell they are executed in
> shell's interactive (aka command-line) or batch mode. After they could
> turn on/off paginated output according to the shell mode they are with.

Sounds interesting. However, I would go further. If pager == 1 and we are
in batch mode then paging is inactive. If pager == 2 and we are in batch
mode then paging is active. If we are in interactive mode then if
pager != 0 then paging is active.

Daniel

Re: [PATCH] Enable pager by default

[Daniel Kiper]

On Wed, Oct 23, 2019 at 01:26:54PM +0200, Javier Martinez Canillas wrote:
> 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 <pjones@redhat.com>
> >>>>
> >>>> 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 <javierm@redhat.com>
> 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 <javierm@redhat.com>

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

> ---
>
> 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;
>  \f
>  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.

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

Daniel

Re: [PATCH] Enable pager by default

[Vladimir 'phcoder' Serbinenko]

What is the advantage of having -p versus just using pager=1? We
should not duplicate functionality. It increases code size with no
benefit.
The easiest solution for informing the user is to add it to stating
prompt in GRUB console

On Thu, Oct 24, 2019 at 6:07 PM Daniel Kiper <dkiper@net-space.pl> wrote:
>
> On Wed, Oct 23, 2019 at 01:26:54PM +0200, Javier Martinez Canillas wrote:
> > 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 <pjones@redhat.com>
> > >>>>
> > >>>> 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 <javierm@redhat.com>
> > 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 <javierm@redhat.com>
>
> 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...
>
> > ---
> >
> > 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.
>
> > +}
> > +
> > +void
> > +grub_disable_temp_more (void)
> > +{
> > +  if (!temp_more)
> > +    grub_set_more (0);
>
> Daniel
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel



-- 
Regards
Vladimir 'phcoder' Serbinenko

Re: [PATCH] Enable pager by default

[Michael Chang]

On Thu, Oct 24, 2019 at 04:39:09PM +0200, Daniel Kiper wrote:
> On Thu, Oct 24, 2019 at 06:54:53AM +0000, Michael Chang wrote:
> > On Tue, Oct 22, 2019 at 04:04:28PM +0200, 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 <pjones@redhat.com>
> > > > >>
> > > > >> 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.
> >
> > I'd like to add option to the list, which is grub could provide the
> > information to have the commands able to tell they are executed in
> > shell's interactive (aka command-line) or batch mode. After they could
> > turn on/off paginated output according to the shell mode they are with.
> 
> Sounds interesting. However, I would go further. If pager == 1 and we are
> in batch mode then paging is inactive. If pager == 2 and we are in batch
> mode then paging is active. If we are in interactive mode then if
> pager != 0 then paging is active.

I agree completely. In this way we no longer have to worry about setting
page=1 would disrupt boot process as some command output could go overly
log and at the same time setting page=2 could enforce paginated output
everywhere, like what is working now. :)

Thanks,
Michael

> 
> Daniel

Re: [PATCH] Enable pager by default

[Vladimir 'phcoder' Serbinenko]

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

On Fri, 25 Oct 2019, 08:20 Michael Chang, <MChang@suse.com> wrote:

> On Thu, Oct 24, 2019 at 04:39:09PM +0200, Daniel Kiper wrote:
> > On Thu, Oct 24, 2019 at 06:54:53AM +0000, Michael Chang wrote:
> > > On Tue, Oct 22, 2019 at 04:04:28PM +0200, 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 <pjones@redhat.com>
> > > > > >>
> > > > > >> 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.
> > >
> > > I'd like to add option to the list, which is grub could provide the
> > > information to have the commands able to tell they are executed in
> > > shell's interactive (aka command-line) or batch mode. After they could
> > > turn on/off paginated output according to the shell mode they are with.
> >
> > Sounds interesting. However, I would go further. If pager == 1 and we are
> > in batch mode then paging is inactive. If pager == 2 and we are in batch
> > mode then paging is active. If we are in interactive mode then if
> > pager != 0 then paging is active.
>
> I agree completely. In this way we no longer have to worry about setting
> page=1 would disrupt boot process as some command output could go overly
> log and at the same time setting page=2 could enforce paginated output
> everywhere, like what is working now. :)
>
This increases complexity. Like what is batch mode. Is running sourcefile
batch or no? There will be edge cases like this. More complexity is more
risk. Moreover in this case the risk is not contained unlike in e.g.
filesystem modules that are not even loaded unless needed. Also it fails at
informing the user. In fact it does the opposite by making it more
confusing at first glance. Informing the user on welcome message is better

>
> Thanks,
> Michael
>
> >
> > Daniel
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

[-- Attachment #2: Type: text/html, Size: 4647 bytes --]

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 <javierm@redhat.com>
>> 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 <javierm@redhat.com>
> 
> 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;
>>  \f
>>  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

Re: [PATCH] Enable pager by default

[Michael Chang]

On Fri, Oct 25, 2019 at 08:48:05AM +0200, Vladimir 'phcoder' Serbinenko wrote:
> On Fri, 25 Oct 2019, 08:20 Michael Chang, <MChang@suse.com> wrote:
> 
> > On Thu, Oct 24, 2019 at 04:39:09PM +0200, Daniel Kiper wrote:
> > > On Thu, Oct 24, 2019 at 06:54:53AM +0000, Michael Chang wrote:
> > > > On Tue, Oct 22, 2019 at 04:04:28PM +0200, 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 <pjones@redhat.com>
> > > > > > >>
> > > > > > >> 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.
> > > >
> > > > I'd like to add option to the list, which is grub could provide the
> > > > information to have the commands able to tell they are executed in
> > > > shell's interactive (aka command-line) or batch mode. After they could
> > > > turn on/off paginated output according to the shell mode they are with.
> > >
> > > Sounds interesting. However, I would go further. If pager == 1 and we are
> > > in batch mode then paging is inactive. If pager == 2 and we are in batch
> > > mode then paging is active. If we are in interactive mode then if
> > > pager != 0 then paging is active.
> >
> > I agree completely. In this way we no longer have to worry about setting
> > page=1 would disrupt boot process as some command output could go overly
> > log and at the same time setting page=2 could enforce paginated output
> > everywhere, like what is working now. :)
> >
> This increases complexity. Like what is batch mode. Is running sourcefile
> batch or no?

Yes, any running script (via source or similar) from interactive shell
is also batch mode, only the command directly read from user's input in
grub's command shell is considered interactive.

> There will be edge cases like this. More complexity is more
> risk. Moreover in this case the risk is not contained unlike in e.g.
> filesystem modules that are not even loaded unless needed. Also it fails at
> informing the user. In fact it does the opposite by making it more
> confusing at first glance. Informing the user on welcome message is better

OK. Adimttedly new behavior is more complex and more difficult to tell
would inadvertantly enable interactive output. I am fine going with
either way being discussed in this thread. :)

Thanks,
Michael

> 
> >
> > Thanks,
> > Michael
> >
> > >
> > > Daniel
> >
> > _______________________________________________
> > Grub-devel mailing list
> > Grub-devel@gnu.org
> > https://lists.gnu.org/mailman/listinfo/grub-devel
> >

> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] Enable pager by default

[Daniel Kiper]

On Fri, Oct 25, 2019 at 11:02:04AM +0200, Javier Martinez Canillas wrote:
> 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 <javierm@redhat.com>
> >> 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 <javierm@redhat.com>
> >
> > 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.

After some thinking it seems to me that Valdimir is right and we should
not over-complicate the solution. Let's print the message on interactive
screen how to enable paging.

Daniel

Re: [PATCH] Enable pager by default

[adrian15 adrian15]

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

I agree.

One of the Super Grub2 Disk options consists on showing from its menu the
COPYRIGHT page... the pager variable is set to true so that the output is
paged when the cat command is run.
This usecase might not work anymore if something else than prompting a
message on the interactive screen was used.

El mié., 30 oct. 2019 a las 13:12, Daniel Kiper (<dkiper@net-space.pl>)
escribió:

> On Fri, Oct 25, 2019 at 11:02:04AM +0200, Javier Martinez Canillas wrote:
> > 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 <javierm@redhat.com>
> > >> 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 <javierm@redhat.com>
> > >
> > > 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.
>
> After some thinking it seems to me that Valdimir is right and we should
> not over-complicate the solution. Let's print the message on interactive
> screen how to enable paging.
>
> Daniel
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>


-- 
Support free software. Donate to Super Grub Disk. Apoya el software libre.
Dona a Super Grub Disk. https://www.supergrubdisk.org/donate/

[-- Attachment #2: Type: text/html, Size: 4397 bytes --]

Re: [PATCH] Enable pager by default

[Javier Martinez Canillas]

On 10/30/19 1:12 PM, Daniel Kiper wrote:
> On Fri, Oct 25, 2019 at 11:02:04AM +0200, Javier Martinez Canillas wrote:

[snip]

>>
>> But I'll wait for the discussion to settle and a solution to be agreed upon,
>> before posting a new patch for this.
> 
> After some thinking it seems to me that Valdimir is right and we should
> not over-complicate the solution. Let's print the message on interactive
> screen how to enable paging.
>

Ok, sounds good to me.
 
> Daniel
> 

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