[v2,gdb/debuginfod] Ask to cancel further downloads

Message ID 20221217001525.82647-1-amerey@redhat.com
State New
Headers
Series [v2,gdb/debuginfod] Ask to cancel further downloads |

Commit Message

Aaron Merey Dec. 17, 2022, 12:15 a.m. UTC
  v1 can be found here:
https://sourceware.org/pipermail/gdb-patches/2022-September/192188.html

v2 adds ^C pressed-twice functionality to debuginfod downloads based on
Pedro's feedback:
https://sourceware.org/pipermail/gdb-patches/2022-September/192225.html

Currently gdb only allows for debuginfod downloads to be cancelled one at
a time via ^C.  This can be a burden if one wishes to cancel a large series
of downloads.  Additionally there can also be ambiguity between whether
^C during a download was intended to cancel the download or interrupt the
inferior.

This patch addresses these issues by adding a new debuginfod setting and
changing the behavior of ^C during a download.

Add a new command "set debuginfod cancel one/all/ask", where:
- "one" means ^C cancels one download,
- "all" means ^C cancels all further downloads, and
- "ask" means ^C asks whether to cancel all further downloads.  A "yes" implies
  "set debuginfod cancel all", and a "no" implies "set debuginfod cancel one",
  so the question is only asked once.

Note that the behaviour as it was before this patch is equivalent to
"set debuginfod cancel one".  Instead, the new default is "set debuginfod
cancel ask".  Note that cancelling all further downloads implies "set
debuginfod enabled off".

Additionally, a single ^C during downloading now sets the quit_flag and
proceeds with all downloads.  If the inferior has the terminal, then a second
^C during downloading triggers a query asking whether to cancel the download
or interrupt the inferior.  If the user wishes to cancel the download then
the setting of 'set debuginfod cancel' determines whether one or all downloads
are cancelled.  In the case of "set debuginfod cancel ask", there will be
another query at this point asking whether to cancel one or all downloads.

If the inferior does not have the terminal, then a second ^C during
downloading simply cancels the download according to the setting of
"set debuginfod cancel".  In this case there is no query asking whether
to interrupt the inferior or cancel a download.

Example session where inferior has terminal:

  (gdb) run
  [...]
  Downloading separate debug info for /lib64/libxyz.so
  [###                                                            ]^C^C
  Cancel the current download?
  If no, then ^C will be sent to the target process. ([y] or n) y
  Cancelling download of separate debug info for /lib64/libxyz.so...
  Cancel further downloading for this session? (y or [n]) n
  Downloading separate debug info for /lib64/libabcd.so

Example session where inferior does not have terminal:

  (gdb) run
  [...]
  Downloading separate debug info for /lib64/libxyz.so
  [###                                                            ]^C^C
  Cancelling download of separate debug info for /lib64/libxyz.so...
  Cancel further downloading for this session? (y or [n]) y
  Debuginfod has been disabled.
  To re-enable use the 'set debuginfod enabled on' command.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29582
Suggested-By: Martin Liška <mliska@suse.cz>
Co-Authored-By: Tom de Vries <tdevries@suse.de>
---
 gdb/debuginfod-support.c | 131 +++++++++++++++++++++++++++++++++++++--
 gdb/doc/gdb.texinfo      |  23 +++++++
 2 files changed, 150 insertions(+), 4 deletions(-)
  

Comments

Eli Zaretskii Dec. 17, 2022, 8:23 a.m. UTC | #1
> Cc: pedro@palves.net, tdevries@suse.de, Aaron Merey <amerey@redhat.com>,
>  Martin Liška <mliska@suse.cz>
> Date: Fri, 16 Dec 2022 19:15:25 -0500
> From: Aaron Merey via Gdb-patches <gdb-patches@sourceware.org>
> 
> +  add_setshow_enum_cmd ("cancel", class_run, debuginfod_cancel_enum,
> +			_("Set cancellation mode for debuginfod."),

I suggest a more specific wording here:

  Set cancellation mode for debuginfod downloads.

> +			_("Show cancellation mode for debuginfod."),

And similarly here.

> +The cancellation mode modifies the behavior of ^C while a file is downloading\
> + from debuginfod.\nWhen set to one, two ^C cancels a single download.\n\
> +When set to all, two ^C cancels all further downloads.\n\
> +When set to ask, two ^C asks what to do.\nA single ^C during downloading is\
> + passed to the target process being debugged.\nA second ^C during downloading\
> + may raise a prompt asking whether to cancel the download or send ^C to the\
> + target.\nIf the download is to be cancelled, the cancellation mode takes\
> + effect and no ^C is sent to the target."),

This should use "cancel" and "ask", in plural, not "cancels" and
"asks" in singular, because it talks about "two ^C".

However, it might be better to say

  ..., pressing ^C twice cancels a single download

and similarly for the other instances, because "two ^C" is somewhat
awkward.  That is the wording you used for the manual, and it is
better.

> +@kindex set debuginfod cancel
> +@anchor{set debuginfod cancel}
> +@item set debuginfod cancel
> +@itemx set debuginfod cancel one
> +@cindex debuginfod cancellation mode
> +Pressing @code{^C} twice during downloading will cancel the current

I suggest to use @kbd{Ctrl-C} instead @code{^C}, here and elsewhere in
the patch.

> +@item set debuginfod cancel all
> +Pressing @code{^C} twice during downloading will cancel all further
> +downloads.

I think "this and all further downloads" is more accurate.

> +@item set debuginfod cancel ask
> +Pressing @code{^C} twice during a download will prompt the user to

Instead of "the user", please use "you", or even nothing.  The manual
is written from the POV of "the user", so we should not use "the user"
as some third party.

Also, "will prompt whether to cancel", since this is the essence of
the question.  The user is not asked to cancel, the user is asked
whether GDB should cancel.

> +cancel all further downloads before attempting to perform the next query.
                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
I'm not sure I understand the emphasized part.  What do you mean by
"before attempting"?

> +@kindex show debuginfod cancel
> +@item show debuginfod cancel
> +Display whether @code{debuginfod cancel} is set to @code{one}, @code{all} or
> +@code{ask}.

I suggest to reword this

  Display the current setting of @code{debuginfod cancel}.

Thanks.
  

Patch

diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
index f1249203da0..da4af4a144b 100644
--- a/gdb/debuginfod-support.c
+++ b/gdb/debuginfod-support.c
@@ -34,6 +34,8 @@  static cmd_list_element *show_debuginfod_prefix_list;
 static const char debuginfod_on[] = "on";
 static const char debuginfod_off[] = "off";
 static const char debuginfod_ask[] = "ask";
+static const char debuginfod_one[] = "one";
+static const char debuginfod_all[] = "all";
 
 static const char *debuginfod_enabled_enum[] =
 {
@@ -43,6 +45,20 @@  static const char *debuginfod_enabled_enum[] =
   nullptr
 };
 
+/* Valid values for set debuginfod cancel command.  */
+
+static const char *debuginfod_cancel_enum[] =
+{
+  debuginfod_one,
+  debuginfod_all,
+  debuginfod_ask,
+  nullptr
+};
+
+/* Value of debuginfod cancellation mode.  */
+
+static const char *debuginfod_cancel = debuginfod_ask;
+
 static const char *debuginfod_enabled =
 #if defined(HAVE_LIBDEBUGINFOD)
   debuginfod_ask;
@@ -88,9 +104,11 @@  debuginfod_exec_query (const unsigned char *build_id,
 struct user_data
 {
   user_data (const char *desc, const char *fname)
-    : desc (desc), fname (fname)
+    : pass_quit_flag (false), inf_had_term (false), desc (desc), fname (fname)
   { }
 
+  bool pass_quit_flag;
+  bool inf_had_term;
   const char * const desc;
   const char * const fname;
   ui_out::progress_update progress;
@@ -148,9 +166,45 @@  progressfn (debuginfod_client *c, long cur, long total)
 
   if (check_quit_flag ())
     {
-      gdb_printf ("Cancelling download of %s %s...\n",
-		  data->desc, styled_fname.c_str ());
-      return 1;
+      /* If a single ^C occurs during downloading, let it propagate to the
+	 target.  If more than one ^C occurs, ask whether to cancel the
+	 current download or interrupt the target.  If the download is
+	 cancelled, the setting of debuginfod_cancel will determine whether
+	 the current download is cancelled or debuginfod is disabled.  */
+      if (!data->pass_quit_flag)
+	data->pass_quit_flag = true;
+      else
+	{
+	  int resp = 1;
+
+	  if (data->inf_had_term)
+	    resp = yquery (_("Cancel the current download?\nIf no, then ^C "
+			     "will be sent to the target process. "));
+	  if (resp)
+	    {
+	      gdb_printf (_("Cancelling download of %s %s...\n"),
+			  data->desc, styled_fname.c_str ());
+	      if (debuginfod_cancel == debuginfod_ask)
+		{
+		  resp = nquery
+		    (_("Cancel further downloading for this session? "));
+		  if (resp)
+		    debuginfod_cancel = debuginfod_all;
+		  else
+		    debuginfod_cancel = debuginfod_one;
+		}
+	      if (debuginfod_cancel == debuginfod_all)
+		{
+		  gdb_printf (_("Debuginfod has been disabled.\nTo re-enable "
+				"use the 'set debuginfod enabled on' "
+				"command.\n"));
+		  debuginfod_enabled = debuginfod_off;
+		}
+
+	      data->pass_quit_flag = false;
+	      return 1;
+	    }
+	}
     }
 
   if (debuginfod_verbose == 0)
@@ -293,6 +347,10 @@  debuginfod_source_query (const unsigned char *build_id,
   user_data data ("source file", srcpath);
 
   debuginfod_set_user_data (c, &data);
+
+  if (!target_terminal::is_ours ())
+    data.inf_had_term = true;
+
   gdb::optional<target_terminal::scoped_restore_terminal_state> term_state;
   if (target_supports_terminal_ours ())
     {
@@ -308,6 +366,10 @@  debuginfod_source_query (const unsigned char *build_id,
   debuginfod_set_user_data (c, nullptr);
   print_outcome (data, fd.get ());
 
+  if (data.pass_quit_flag)
+    set_quit_flag ();
+  if (data.inf_had_term && term_state.has_value ())
+    target_terminal::inferior ();
   if (fd.get () >= 0)
     destname->reset (dname);
 
@@ -334,6 +396,10 @@  debuginfod_debuginfo_query (const unsigned char *build_id,
   user_data data ("separate debug info for", filename);
 
   debuginfod_set_user_data (c, &data);
+
+  if (!target_terminal::is_ours ())
+    data.inf_had_term = true;
+
   gdb::optional<target_terminal::scoped_restore_terminal_state> term_state;
   if (target_supports_terminal_ours ())
     {
@@ -346,6 +412,10 @@  debuginfod_debuginfo_query (const unsigned char *build_id,
   debuginfod_set_user_data (c, nullptr);
   print_outcome (data, fd.get ());
 
+  if (data.pass_quit_flag)
+    set_quit_flag ();
+  if (data.inf_had_term && term_state.has_value ())
+    target_terminal::inferior ();
   if (fd.get () >= 0)
     destname->reset (dname);
 
@@ -372,6 +442,10 @@  debuginfod_exec_query (const unsigned char *build_id,
   user_data data ("executable for", filename);
 
   debuginfod_set_user_data (c, &data);
+
+  if (!target_terminal::is_ours ())
+    data.inf_had_term = true;
+
   gdb::optional<target_terminal::scoped_restore_terminal_state> term_state;
   if (target_supports_terminal_ours ())
     {
@@ -383,6 +457,10 @@  debuginfod_exec_query (const unsigned char *build_id,
   debuginfod_set_user_data (c, nullptr);
   print_outcome (data, fd.get ());
 
+  if (data.pass_quit_flag)
+    set_quit_flag ();
+  if (data.inf_had_term && term_state.has_value ())
+    target_terminal::inferior ();
   if (fd.get () >= 0)
     destname->reset (dname);
 
@@ -423,6 +501,33 @@  show_debuginfod_enabled (ui_file *file, int from_tty, cmd_list_element *cmd,
 		"\"%s\".\n"), debuginfod_enabled);
 }
 
+/* Set callback for "set debuginfod cancel".  */
+
+static void
+set_debuginfod_cancel (const char *value)
+{
+  debuginfod_cancel = value;
+}
+
+/* Get callback for "set debuginfod cancel".  */
+
+static const char *
+get_debuginfod_cancel ()
+{
+  return debuginfod_cancel;
+}
+
+/* Show callback for "set debuginfod cancel".  */
+
+static void
+show_debuginfod_cancel (ui_file *file, int from_tty, cmd_list_element *cmd,
+			const char *value)
+{
+  gdb_printf (file,
+	      _("Debuginfod cancellation mode is currently set to "
+		"\"%s\".\n"), debuginfod_cancel);
+}
+
 /* Set callback for "set debuginfod urls".  */
 
 static void
@@ -503,6 +608,24 @@  source files."),
 			&set_debuginfod_prefix_list,
 			&show_debuginfod_prefix_list);
 
+  add_setshow_enum_cmd ("cancel", class_run, debuginfod_cancel_enum,
+			_("Set cancellation mode for debuginfod."),
+			_("Show cancellation mode for debuginfod."),
+			_("\
+The cancellation mode modifies the behavior of ^C while a file is downloading\
+ from debuginfod.\nWhen set to one, two ^C cancels a single download.\n\
+When set to all, two ^C cancels all further downloads.\n\
+When set to ask, two ^C asks what to do.\nA single ^C during downloading is\
+ passed to the target process being debugged.\nA second ^C during downloading\
+ may raise a prompt asking whether to cancel the download or send ^C to the\
+ target.\nIf the download is to be cancelled, the cancellation mode takes\
+ effect and no ^C is sent to the target."),
+			set_debuginfod_cancel,
+			get_debuginfod_cancel,
+			show_debuginfod_cancel,
+			&set_debuginfod_prefix_list,
+			&show_debuginfod_prefix_list);
+
   /* set/show debuginfod urls */
   add_setshow_string_noescape_cmd ("urls", class_run, _("\
 Set the list of debuginfod server URLs."), _("\
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index bababf3c7ff..246854d229a 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -48270,6 +48270,29 @@  is set to @code{ask} for interactive sessions.
 Display whether @code{debuginfod enabled} is set to @code{on}, @code{off} or
 @code{ask}.
 
+@kindex set debuginfod cancel
+@anchor{set debuginfod cancel}
+@item set debuginfod cancel
+@itemx set debuginfod cancel one
+@cindex debuginfod cancellation mode
+Pressing @code{^C} twice during downloading will cancel the current
+download.
+
+@item set debuginfod cancel all
+Pressing @code{^C} twice during downloading will cancel all further
+downloads.
+
+@item set debuginfod cancel ask
+Pressing @code{^C} twice during a download will prompt the user to
+cancel all further downloads before attempting to perform the next query.
+By default, @code{debuginfod cancel} is set to @code{ask} for interactive
+sessions.
+
+@kindex show debuginfod cancel
+@item show debuginfod cancel
+Display whether @code{debuginfod cancel} is set to @code{one}, @code{all} or
+@code{ask}.
+
 @kindex set debuginfod urls
 @cindex configure debuginfod URLs
 @item set debuginfod urls