From: Keith Seitz <keiths@redhat.com>
To: Aaron Merey <amerey@redhat.com>, gdb-patches@sourceware.org
Subject: Re: [PATCH 1/2] gdb: add set/show commands for managing debuginfod
Date: Wed, 20 Oct 2021 13:34:32 -0700 [thread overview]
Message-ID: <7eeed275-d442-fee2-4d5e-352c8ed34ec3@redhat.com> (raw)
In-Reply-To: <20211018230133.265619-2-amerey@redhat.com>
Hi,
Thank you for your perseverance with this patch. This is really
shaping up as a nice, user-friendly addition. It's also very
"gdb-ish."
I have just a few things. Nothing major.
On 10/18/21 16:01, Aaron Merey wrote:
> This GDB supports auto-downloading debuginfo from the following URLs:
> <URL1> <URL2> ...
> Enable debuginfod? (y or [n]) y
> Debuginfod has been enabled.
> To make this setting permanent, add 'set debuginfod on' to .gdbinit.
Very nice!
> Also add 'set/show debuginfod-urls' command. Accepts a string of
> space-separated debuginfod server URLs to be queried. The default
> value is copied from $DEBUGINFOD_URLS.
At first I thought this space-separated list as a bit odd, but I see that
this is exactly the same format as the environment variable DEBUGINFOD_URLS.
I certainly appreciate the consistency.
> Finally add 'set debug debuginfod" command to control whether
> debuginfod-related debugging output is displayed. This debugging
> output is displayed by default.
>
> (gdb) run
> Starting program: /bin/sleep 5
> Download failed: No route to host. Continuing without debug info for /lib64/libc.so.6.
Here I have a question. From the patch, it appears that the debugging output
consists entirely of error messages? Am I alone thinking we should always print these
for the user?
> diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
> index 2d626e335a0..1a4a6e1dde0 100644
> --- a/gdb/debuginfod-support.c
> +++ b/gdb/debuginfod-support.c
> @@ -68,6 +69,63 @@ struct debuginfod_client_deleter
> using debuginfod_client_up
> = std::unique_ptr<debuginfod_client, debuginfod_client_deleter>;
>
> +static const char debuginfod_on[] = "on";
> +static const char debuginfod_off[] = "off";
> +static const char debuginfod_ask[] = "ask";
> +static const char *const debuginfod_cmd_names[] =
> +{
> + debuginfod_on,
> + debuginfod_off,
> + debuginfod_ask,
> + NULL,
> +};
> +
> +static const char *debuginfod_enable = debuginfod_ask;
> +static std::string debuginfod_urls;
> +static bool debuginfod_debug = true;
> +
> +/* Show whether debuginfod is enabled. */
> +
> +static void
> +show_debuginfod_command (struct ui_file *file, int from_tty,
> + struct cmd_list_element *cmd, const char *value)
> +{
> + fprintf_unfiltered (file, _("Debuginfod functionality is currently set " \
> + "to \"%s\".\n"), debuginfod_enable);
> +}
> +
> +/* Set the URLs that debuginfod will query. */
> +
> +static void
> +set_debuginfod_urls_command (const char *args, int from_tty,
> + struct cmd_list_element *c)
> +{
> + if (setenv ("DEBUGINFOD_URLS", debuginfod_urls.c_str (), 1) != 0)
> + warning (_("Unable to set debuginfod URLs: %s"), strerror (errno));
>
I believe we prefer to use safe_strerror (as used elsewhere in this file).
> +
> +/* Show the URLs that debuginfod will query. */
> +
> +static void
> +show_debuginfod_urls_command (struct ui_file *file, int from_tty,
> + struct cmd_list_element *cmd, const char *value)
> +{
> + if (debuginfod_urls.empty ())
> + fprintf_unfiltered (file, _("You have not set debuginfod URLs.\n"));
> + else
> + fprintf_filtered (file, _("Debuginfod URLs are currently set to:\n%s\n"),
> + value);
> +}
> +
> +/* Show whether to display debuginfod debugging output. */
> +
> +static void
> +show_debuginfod_debug_command (struct ui_file *file, int from_tty,
> + struct cmd_list_element *cmd, const char *value)
> +{
> + fprintf_filtered (file, _("Debuginfod debugging output is %s.\n"), value);
> +}
> +
> static int
> progressfn (debuginfod_client *c, long cur, long total)
> {
> @@ -195,3 +283,50 @@ debuginfod_debuginfo_query (const unsigned char *build_id,
> return fd;
> }
> #endif
> +
> +/* Register debuginfod commands. */
> +
> +void _initialize_debuginfod ();
> +void
> +_initialize_debuginfod ()
> +{
> +#ifdef HAVE_LIBDEBUGINFOD
> + char *urls = getenv (DEBUGINFOD_URLS_ENV_VAR);
> +
> + if (urls != nullptr)
> + debuginfod_urls = urls;
> +
> + add_setshow_enum_cmd ("debuginfod", class_run, debuginfod_cmd_names,
> + &debuginfod_enable, _("\
> +Set whether debuginfod is enabled."), _("\
> +Show whether debuginfod is enabled."), _("\
> +Automatically query debuginfod servers for missing debuginfo, executables \
> +or source files. This command accepts the following arguments:\n\
> + on - debuginfod will be enabled\n\
> + off - debuginfod will be disabled\n\
> + ask - an interactive prompt will ask whether to enable debuginfod\n\
> +For interactive sessions, \"ask\" is the default. For non-interactive \
> +sessions, \"off\" is the default."),
> + NULL, show_debuginfod_command, &setlist, &showlist);
> +
> + add_setshow_string_noescape_cmd ("debuginfod-urls", class_run,
> + &debuginfod_urls, _("\
> +Set the list of debuginfod server URLs."), _("\
> +Show the list of debuginfod server URLs."), _("\
> +Manage the space-separated list of debuginfod server URLs that GDB will query \
> +when missing debuginfo, executables or source files.\nThe default value is \
> +copied from $DEBUGINFOD_URLS."),
> + set_debuginfod_urls_command,
> + show_debuginfod_urls_command,
> + &setlist, &showlist);
> +
> + add_setshow_boolean_cmd ("debuginfod", class_maintenance,
> + &debuginfod_debug, _("\
> +Set debuginfod debugging."), _("\
> +Show debuginfod debugging."), _("\
> +When set, display debugging output for debuginfod. \
> +Displayed by default"),
> + NULL, show_debuginfod_debug_command,
> + &setdebuglist, &showdebuglist);
> +#endif
> +}
Just a question for others: Is this the normal style used in GDB? E.g., the
python command is always available:
(gdb) python print(1)
Python scripting is not supported in this copy of GDB.
Like command/command-line options, I'd like us to be vigilant of consistency.
[Maybe that's just my personal thing, though.]
It's just a question -- I am not asking for any changes.
With the safe_strerror change, though, I recommend a maintainer review
this patch for final approval(s).
Keith
next prev parent reply other threads:[~2021-10-20 20:34 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-10-18 23:01 [PATCH 0/2 v3] gdb: Add debuginfod first-use notification Aaron Merey
2021-10-18 23:01 ` [PATCH 1/2] gdb: add set/show commands for managing debuginfod Aaron Merey
2021-10-20 20:34 ` Keith Seitz [this message]
2021-10-21 22:23 ` Lancelot SIX
2021-10-25 22:30 ` Aaron Merey
2021-10-21 22:02 ` Lancelot SIX
2021-10-26 16:08 ` Simon Marchi
2021-10-28 22:18 ` Aaron Merey
2021-10-29 1:47 ` Simon Marchi
2021-10-30 1:09 ` Aaron Merey
2021-10-30 1:54 ` Simon Marchi
2021-10-31 2:43 ` Simon Marchi
2021-11-01 15:52 ` Simon Marchi
2021-11-01 17:39 ` Aaron Merey
2021-11-01 18:00 ` Simon Marchi
2021-11-02 16:51 ` Simon Marchi
2021-11-02 20:35 ` Aaron Merey
2021-10-18 23:01 ` [PATCH 2/2] gdb.texinfo: Expand documentation for debuginfod Aaron Merey
2021-10-19 11:17 ` Eli Zaretskii
2021-10-19 22:35 ` Aaron Merey
2021-10-20 11:38 ` Eli Zaretskii
2021-10-30 1:18 ` Aaron Merey
2021-10-30 6:57 ` Tom de Vries
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7eeed275-d442-fee2-4d5e-352c8ed34ec3@redhat.com \
--to=keiths@redhat.com \
--cc=amerey@redhat.com \
--cc=gdb-patches@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).