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 <address@hidden> 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 <address@hidden>
> > >>>>
> > >>>> When user enters into the GRUB shell and tries to use help command, 
> > >>>> lot of
> > >>>> information is scrolled out of screen and the user doesn't have chance 
> > >>>> to
> > >>>> read it. Also, there isn't any information about 'set pager=1' at the 
> > >>>> end
> > >>>> of the help output, to tell the user how scrolling could be enabled.
> > >>>>
> > >>>> So just enable pager by default which leads to a much better 
> > >>>> experience.
> > >>>
> > >>> Hmmm... What will happen if a command produce tons of output during boot
> > >>> process? I am afraid that it will hang indefinitely waiting for an user
> > >>> input. This should not happen. So, I tend to agree that current help
> > >>> command behavior is annoying but I do not like the solution.
> > >>
> > >> Ok. I'll then explore having a paginated output only for the help command
> > >> instead of globally enabling it by default.
> > >
> > > Great! Though I would think about something which can be used also in
> > > other commands producing a lot of output. Maybe we should introduce "-p"
> > > (pause) command line option for such commands. And I am not against
> > > using existing code to do a pause. We just have to do it carefully.
> > >
> >
> > So I've tried different approaches to solves this. I'm not that sure about
> > introducing a -p option, since I think that we could have the same issue
> > of users not knowing that have to use this option to scroll the output.
> >
> > Unless we print for each command a message after its output, telling users
> > that have to use -p if they want to have paginated output for that command.
> >
> > What do you think about the following patch? That way we can specify if the
> > output for each command must be printed with the pager enabled or not.
> >
> > Probably should be broken in 2 patches, but just want to know your opinion.
> >
> > From 7c4da6295ebd3a034d1f7e32099eab33efa465d4 Mon Sep 17 00:00:00 2001
> > From: Javier Martinez Canillas <address@hidden>
> > Date: Tue, 22 Oct 2019 15:35:12 +0200
> > Subject: [PATCH v2] Add a GRUB_COMMAND_FLAG_PAGINATED to request paginated
> >  output for commands
> >
> > When user enters into the GRUB shell and tries to use help command, lot of
> > information is scrolled out of screen and the user doesn't have chance to
> > read it. Also, there isn't any information about 'set pager=1' at the end
> > of the help output, to tell the user how scrolling could be enabled.
> >
> > Since the out for some commands may not fit into a screen, add a new flag
> > GRUB_COMMAND_FLAG_PAGINATED that can be used when registering commands that
> > can be used to request the pager to be enabled while executing the handler.
> >
> > Signed-off-by: Javier Martinez Canillas <address@hidden>
>
> 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
> address@hidden
> https://lists.gnu.org/mailman/listinfo/grub-devel



-- 
Regards
Vladimir 'phcoder' Serbinenko