[3/7] gdb/debuginfod: disable pagination during downloads

  Disable pagination during downloads in order to avoid inconvenient
continue prompts "--Type <RET> for more, q to quit...".

For more discussion on this issue see the following thread
https://sourceware.org/pipermail/gdb-patches/2023-February/196674.html
---
 gdb/debuginfod-support.c | 11 +++++++++++
 1 file changed, 11 insertions(+)
  

>>>>> "Aaron" == Aaron Merey via Gdb-patches <gdb-patches@sourceware.org> writes:

Aaron> Disable pagination during downloads in order to avoid inconvenient
Aaron> continue prompts "--Type <RET> for more, q to quit...".

I still don't get the rationale for this patch.

If the user has paging enabled, then barring some critical reason to
disable it, it seems like paging should happen.

Tom
  

Hi Tom,

On Fri, Mar 3, 2023 at 4:33 PM Tom Tromey <tom@tromey.com> wrote:
>
> >>>>> "Aaron" == Aaron Merey via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Aaron> Disable pagination during downloads in order to avoid inconvenient
> Aaron> continue prompts "--Type <RET> for more, q to quit...".
>
> I still don't get the rationale for this patch.
>
> If the user has paging enabled, then barring some critical reason to
> disable it, it seems like paging should happen.

During debuginfod progress updates, paging prompts sometimes happen
and sometimes don't happen due to conditions internal to gdb that aren't
obvious to the user.  This may be perplexing to the user. IMO it's better
if the prompt behavior is more uniform.

I've also heard from at least one person that they left gdb unsupervised
during a large series of downloads and were annoyed when they returned
to see that the prompt stopped downloading part way through waiting
for input.

However you are right that users already have the option to disable
pagination. They can separately disable debuginfod progress messages
too.  If you'd prefer that we keep the existing pagination behaviour, at
least users have multiple options if they don't like something.

Aaron
  

Aaron Merey via Gdb-patches <gdb-patches@sourceware.org> writes:

> Disable pagination during downloads in order to avoid inconvenient
> continue prompts "--Type <RET> for more, q to quit...".
>
> For more discussion on this issue see the following thread
> https://sourceware.org/pipermail/gdb-patches/2023-February/196674.html

Is this patch critical to the new functionality in this series?  If it's
not then you might be better spinning this patch into it's own thread.

I also echo Tom's query about why this change is needed.  I haven't read
the thread you reference above -- I want to review the rest of this
series first -- but if there's good justification for this change in
that thread then it would be nice to see that in the commit message, the
commit message is carried with the code, but the mail archive might
disappear in the future.

> ---
>  gdb/debuginfod-support.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
> index 12025fcf0c0..f4969e94b0a 100644
> --- a/gdb/debuginfod-support.c
> +++ b/gdb/debuginfod-support.c
> @@ -292,6 +292,9 @@ debuginfod_source_query (const unsigned char *build_id,
>  			 const char *srcpath,
>  			 gdb::unique_xmalloc_ptr<char> *destname)
>  {
> +  scoped_restore save_count_lines_printed
> +   = make_scoped_restore (&pagination_enabled, false);

Given you have several uses of the same pattern, it might be nice to add
a new 'scoped_restore_pagination' class, we already have lots of
scoped_restore_XXX specialisation classes.

Thanks,
Andrew

> +
>    if (!debuginfod_is_enabled ())
>      return scoped_fd (-ENOSYS);
>  
> @@ -333,6 +336,9 @@ debuginfod_debuginfo_query (const unsigned char *build_id,
>  			    const char *filename,
>  			    gdb::unique_xmalloc_ptr<char> *destname)
>  {
> +  scoped_restore save_count_lines_printed
> +   = make_scoped_restore (&pagination_enabled, false);
> +
>    if (!debuginfod_is_enabled ())
>      return scoped_fd (-ENOSYS);
>  
> @@ -371,6 +377,9 @@ debuginfod_exec_query (const unsigned char *build_id,
>  		       const char *filename,
>  		       gdb::unique_xmalloc_ptr<char> *destname)
>  {
> +  scoped_restore save_count_lines_printed
> +   = make_scoped_restore (&pagination_enabled, false);
> +
>    if (!debuginfod_is_enabled ())
>      return scoped_fd (-ENOSYS);
>  
> @@ -412,6 +421,8 @@ debuginfod_section_query (const unsigned char *build_id,
>  #if !defined (HAVE_DEBUGINFOD_FIND_SECTION)
>    return scoped_fd (-ENOSYS);
>  #else
> +  scoped_restore save_count_lines_printed
> +    = make_scoped_restore (&pagination_enabled, false);
>  
>   if (debuginfod_enabled != debuginfod_lazy || !debuginfod_is_enabled ())
>      return scoped_fd (-ENOSYS);
> -- 
> 2.39.2
  

Hi Andrew,

On Wed, May 24, 2023 at 5:38 AM Andrew Burgess <aburgess@redhat.com> wrote:
>
> Aaron Merey via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> > Disable pagination during downloads in order to avoid inconvenient
> > continue prompts "--Type <RET> for more, q to quit...".
> >
> > For more discussion on this issue see the following thread
> > https://sourceware.org/pipermail/gdb-patches/2023-February/196674.html
>
> Is this patch critical to the new functionality in this series?  If it's
> not then you might be better spinning this patch into it's own thread.

I think we can just drop this patch.  If the user doesn't want pagination
prompts they already have a way to disable it.

Aaron