* [PATCH] Enable pager by default @ 2019-10-18 12:43 Javier Martinez Canillas 2019-10-21 14:56 ` Daniel Kiper 0 siblings, 1 reply; 18+ messages in thread From: Javier Martinez Canillas @ 2019-10-18 12:43 UTC (permalink / raw) To: grub-devel; +Cc: Peter Jones, 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] Enable pager by default 2019-10-18 12:43 [PATCH] Enable pager by default Javier Martinez Canillas @ 2019-10-21 14:56 ` Daniel Kiper 2019-10-22 8:30 ` Javier Martinez Canillas 0 siblings, 1 reply; 18+ messages in thread From: Daniel Kiper @ 2019-10-21 14:56 UTC (permalink / raw) To: Javier Martinez Canillas; +Cc: grub-devel, Peter Jones 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] Enable pager by default 2019-10-21 14:56 ` Daniel Kiper @ 2019-10-22 8:30 ` Javier Martinez Canillas 2019-10-22 14:04 ` Daniel Kiper 0 siblings, 1 reply; 18+ messages in thread From: Javier Martinez Canillas @ 2019-10-22 8:30 UTC (permalink / raw) To: Daniel Kiper; +Cc: grub-devel, Peter Jones 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] Enable pager by default 2019-10-22 8:30 ` Javier Martinez Canillas @ 2019-10-22 14:04 ` Daniel Kiper 2019-10-23 1:50 ` Nicholas Vinson ` (2 more replies) 0 siblings, 3 replies; 18+ messages in thread From: Daniel Kiper @ 2019-10-22 14:04 UTC (permalink / raw) To: Javier Martinez Canillas; +Cc: grub-devel, Peter Jones 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] Enable pager by default 2019-10-22 14:04 ` Daniel Kiper @ 2019-10-23 1:50 ` Nicholas Vinson 2019-10-23 9:13 ` Daniel Kiper 2019-10-23 11:26 ` Javier Martinez Canillas 2019-10-24 6:54 ` Michael Chang 2 siblings, 1 reply; 18+ messages in thread From: Nicholas Vinson @ 2019-10-23 1:50 UTC (permalink / raw) To: grub-devel 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 > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] Enable pager by default 2019-10-23 1:50 ` Nicholas Vinson @ 2019-10-23 9:13 ` Daniel Kiper 0 siblings, 0 replies; 18+ messages in thread From: Daniel Kiper @ 2019-10-23 9:13 UTC (permalink / raw) To: Nicholas Vinson; +Cc: grub-devel 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] Enable pager by default 2019-10-22 14:04 ` Daniel Kiper 2019-10-23 1:50 ` Nicholas Vinson @ 2019-10-23 11:26 ` Javier Martinez Canillas 2019-10-24 14:50 ` Daniel Kiper 2019-10-24 6:54 ` Michael Chang 2 siblings, 1 reply; 18+ messages in thread From: Javier Martinez Canillas @ 2019-10-23 11:26 UTC (permalink / raw) To: Daniel Kiper; +Cc: grub-devel, Peter Jones 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] Enable pager by default 2019-10-23 11:26 ` Javier Martinez Canillas @ 2019-10-24 14:50 ` Daniel Kiper 2019-10-24 16:24 ` Vladimir 'phcoder' Serbinenko 2019-10-25 9:02 ` Javier Martinez Canillas 0 siblings, 2 replies; 18+ messages in thread From: Daniel Kiper @ 2019-10-24 14:50 UTC (permalink / raw) To: Javier Martinez Canillas; +Cc: grub-devel, Peter Jones 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] Enable pager by default 2019-10-24 14:50 ` Daniel Kiper @ 2019-10-24 16:24 ` Vladimir 'phcoder' Serbinenko 2019-10-25 9:02 ` Javier Martinez Canillas 1 sibling, 0 replies; 18+ messages in thread From: Vladimir 'phcoder' Serbinenko @ 2019-10-24 16:24 UTC (permalink / raw) To: The development of GNU GRUB 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] Enable pager by default 2019-10-24 14:50 ` Daniel Kiper 2019-10-24 16:24 ` Vladimir 'phcoder' Serbinenko @ 2019-10-25 9:02 ` Javier Martinez Canillas 2019-10-30 12:12 ` Daniel Kiper 1 sibling, 1 reply; 18+ messages in thread From: Javier Martinez Canillas @ 2019-10-25 9:02 UTC (permalink / raw) To: Daniel Kiper; +Cc: grub-devel, Peter Jones 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] Enable pager by default 2019-10-25 9:02 ` Javier Martinez Canillas @ 2019-10-30 12:12 ` Daniel Kiper 2019-10-30 12:32 ` adrian15 adrian15 2019-10-30 12:43 ` Javier Martinez Canillas 0 siblings, 2 replies; 18+ messages in thread From: Daniel Kiper @ 2019-10-30 12:12 UTC (permalink / raw) To: Javier Martinez Canillas, phcoder; +Cc: grub-devel, Peter Jones 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] Enable pager by default 2019-10-30 12:12 ` Daniel Kiper @ 2019-10-30 12:32 ` adrian15 adrian15 2019-10-30 12:43 ` Javier Martinez Canillas 1 sibling, 0 replies; 18+ messages in thread From: adrian15 adrian15 @ 2019-10-30 12:32 UTC (permalink / raw) To: The development of GNU GRUB Cc: Javier Martinez Canillas, phcoder, Peter Jones [-- 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 --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] Enable pager by default 2019-10-30 12:12 ` Daniel Kiper 2019-10-30 12:32 ` adrian15 adrian15 @ 2019-10-30 12:43 ` Javier Martinez Canillas 1 sibling, 0 replies; 18+ messages in thread From: Javier Martinez Canillas @ 2019-10-30 12:43 UTC (permalink / raw) To: Daniel Kiper, phcoder; +Cc: grub-devel, Peter Jones 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] Enable pager by default 2019-10-22 14:04 ` Daniel Kiper 2019-10-23 1:50 ` Nicholas Vinson 2019-10-23 11:26 ` Javier Martinez Canillas @ 2019-10-24 6:54 ` Michael Chang 2019-10-24 14:39 ` Daniel Kiper 2 siblings, 1 reply; 18+ messages in thread From: Michael Chang @ 2019-10-24 6:54 UTC (permalink / raw) To: The development of GNU GRUB; +Cc: Javier Martinez Canillas, Peter Jones 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] Enable pager by default 2019-10-24 6:54 ` Michael Chang @ 2019-10-24 14:39 ` Daniel Kiper 2019-10-25 6:16 ` Michael Chang 0 siblings, 1 reply; 18+ messages in thread From: Daniel Kiper @ 2019-10-24 14:39 UTC (permalink / raw) To: Michael Chang Cc: The development of GNU GRUB, Javier Martinez Canillas, Peter Jones 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] Enable pager by default 2019-10-24 14:39 ` Daniel Kiper @ 2019-10-25 6:16 ` Michael Chang 2019-10-25 6:48 ` Vladimir 'phcoder' Serbinenko 0 siblings, 1 reply; 18+ messages in thread From: Michael Chang @ 2019-10-25 6:16 UTC (permalink / raw) To: Daniel Kiper Cc: The development of GNU GRUB, Javier Martinez Canillas, Peter Jones 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] Enable pager by default 2019-10-25 6:16 ` Michael Chang @ 2019-10-25 6:48 ` Vladimir 'phcoder' Serbinenko 2019-10-28 6:21 ` Michael Chang 0 siblings, 1 reply; 18+ messages in thread From: Vladimir 'phcoder' Serbinenko @ 2019-10-25 6:48 UTC (permalink / raw) To: The development of GNU GRUB [-- 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 --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] Enable pager by default 2019-10-25 6:48 ` Vladimir 'phcoder' Serbinenko @ 2019-10-28 6:21 ` Michael Chang 0 siblings, 0 replies; 18+ messages in thread From: Michael Chang @ 2019-10-28 6:21 UTC (permalink / raw) To: The development of GNU GRUB 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2019-10-30 12:44 UTC | newest] Thread overview: 18+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-10-18 12:43 [PATCH] Enable pager by default Javier Martinez Canillas 2019-10-21 14:56 ` Daniel Kiper 2019-10-22 8:30 ` Javier Martinez Canillas 2019-10-22 14:04 ` Daniel Kiper 2019-10-23 1:50 ` Nicholas Vinson 2019-10-23 9:13 ` Daniel Kiper 2019-10-23 11:26 ` Javier Martinez Canillas 2019-10-24 14:50 ` Daniel Kiper 2019-10-24 16:24 ` Vladimir 'phcoder' Serbinenko 2019-10-25 9:02 ` Javier Martinez Canillas 2019-10-30 12:12 ` Daniel Kiper 2019-10-30 12:32 ` adrian15 adrian15 2019-10-30 12:43 ` Javier Martinez Canillas 2019-10-24 6:54 ` Michael Chang 2019-10-24 14:39 ` Daniel Kiper 2019-10-25 6:16 ` Michael Chang 2019-10-25 6:48 ` Vladimir 'phcoder' Serbinenko 2019-10-28 6:21 ` Michael Chang
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.