[v2] gdb: Print debuginfod first-use notification

Message ID 20210721042012.154980-1-amerey@redhat.com
State New
Headers show
Series
  • [v2] gdb: Print debuginfod first-use notification
Related show

Commit Message

will schmidt via Gdb-patches July 21, 2021, 4:20 a.m.
Hi Simon,

On Wed, Jul 14, 2021 at 9:02 PM Simon Marchi <simon.marchi@polymtl.ca> wrote:
> The use of "x ? : y" is a GNU extension, can we use that?  I don't

> remember.  Perhaps it's better to do: 

>

>   const char *cache_var = getenv (DEBUGINFOD_CACHE_PATH_ENV_VAR);

>   if (cache_var == nullptr)

>     return;

>

> Since it's useless to do the access call below with an empty string, we

> know it will fail.


Fixed.

> > + 

> > +  if (access (cache_var.c_str (), F_OK) == 0)

> > +    return;

> > + 

> > +  std::string xdg (getenv ("XDG_CACHE_HOME") ?: "");

> > + 

> > +  if (!xdg.empty ()) 

> > +    {

> > +      xdg.append ("/debuginfod_client");

>

> Does debuginfod use XDG_CACHE_HOME regardless of the platform to find a

> cache dir?  I think it would be much better if we could ask the 

> debuginfod library for the path to the local cache, so we don't bake-in

> the location where we think debuginfod puts it (which could change in

> the future).


To avoid this problem I changed the heuristic used to determine first use.
The presence of a 'debuginfod-used-p' file in the GDB config directory will
signal to GDB that the first use notice has already been printed. If the
file does not exist when the first query is about to happen then the file
is created and the notice is printed.

Aaron


From 0ee110b17624b187b6dfb55d4145756da582d8e3 Mon Sep 17 00:00:00 2001
From: Aaron Merey <amerey@redhat.com>

Date: Tue, 20 Jul 2021 23:36:59 -0400
Subject: gdb: Print debuginfod first-use notification

When querying debuginfod servers for the first time, notify the user
that GDB may automatically download debuginfo.  Helps ensure users are
aware that GDB may be relying on the correct operation of remote
debuginfod servers.

In order to determine whether debuginfod is being queried for the
first time, check for the presence of a 'debuginfod-used-p' file
in the gdb config directory.  If one cannot be found, the notification
will be displayed before performing the first query.
---
 gdb/debuginfod-support.c | 41 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 40 insertions(+), 1 deletion(-)

-- 
2.31.1

Comments

will schmidt via Gdb-patches July 21, 2021, 5:25 p.m. | #1
On Wed, Jul 21, 2021 at 12:20:12AM -0400, Aaron Merey via Gdb-patches wrote:
> Hi Simon,

> 

> On Wed, Jul 14, 2021 at 9:02 PM Simon Marchi <simon.marchi@polymtl.ca> wrote:

> > The use of "x ? : y" is a GNU extension, can we use that?  I don't

> > remember.  Perhaps it's better to do: 

> >

> >   const char *cache_var = getenv (DEBUGINFOD_CACHE_PATH_ENV_VAR);

> >   if (cache_var == nullptr)

> >     return;

> >

> > Since it's useless to do the access call below with an empty string, we

> > know it will fail.

> 

> Fixed.

> 

> > > + 

> > > +  if (access (cache_var.c_str (), F_OK) == 0)

> > > +    return;

> > > + 

> > > +  std::string xdg (getenv ("XDG_CACHE_HOME") ?: "");

> > > + 

> > > +  if (!xdg.empty ()) 

> > > +    {

> > > +      xdg.append ("/debuginfod_client");

> >

> > Does debuginfod use XDG_CACHE_HOME regardless of the platform to find a

> > cache dir?  I think it would be much better if we could ask the 

> > debuginfod library for the path to the local cache, so we don't bake-in

> > the location where we think debuginfod puts it (which could change in

> > the future).

> 

> To avoid this problem I changed the heuristic used to determine first use.

> The presence of a 'debuginfod-used-p' file in the GDB config directory will

> signal to GDB that the first use notice has already been printed. If the

> file does not exist when the first query is about to happen then the file

> is created and the notice is printed.

> 

> Aaron

> 

> 

> From 0ee110b17624b187b6dfb55d4145756da582d8e3 Mon Sep 17 00:00:00 2001

> From: Aaron Merey <amerey@redhat.com>

> Date: Tue, 20 Jul 2021 23:36:59 -0400

> Subject: gdb: Print debuginfod first-use notification

> 

> When querying debuginfod servers for the first time, notify the user

> that GDB may automatically download debuginfo.  Helps ensure users are

> aware that GDB may be relying on the correct operation of remote

> debuginfod servers.

> 

> In order to determine whether debuginfod is being queried for the

> first time, check for the presence of a 'debuginfod-used-p' file

> in the gdb config directory.  If one cannot be found, the notification

> will be displayed before performing the first query.

> ---

>  gdb/debuginfod-support.c | 41 +++++++++++++++++++++++++++++++++++++++-

>  1 file changed, 40 insertions(+), 1 deletion(-)

> 

> diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c

> index 2d626e335a0..2de703cc138 100644

> --- a/gdb/debuginfod-support.c

> +++ b/gdb/debuginfod-support.c

> @@ -22,6 +22,7 @@

>  #include "gdbsupport/scoped_fd.h"

>  #include "debuginfod-support.h"

>  #include "gdbsupport/gdb_optional.h"

> +#include "gdbsupport/pathstuff.h"

>  

>  #ifndef HAVE_LIBDEBUGINFOD

>  scoped_fd

> @@ -104,6 +105,41 @@ progressfn (debuginfod_client *c, long cur, long total)

>    return 0;

>  }

>  

> +/* If this is the first time that GDB is using debuginfod on this system, emit

> +   a warning message indicating that GDB may automatically download debuginfo.

> +

> +   To determine whether debuginfod is being used for the first time, check for

> +   the presence of a 'debuginfod-used-p' file in the GDB config directory.  If

> +   it is not found, then try to create the file and print the warning.  */

> +

> +static void

> +notify_first_use ()

> +{

> +  const char *urls = getenv (DEBUGINFOD_URLS_ENV_VAR);

> +  if (urls == nullptr)

> +    return;

> +

> +  std::string used_p = get_standard_config_filename ("debuginfod-used-p");

> +  if (access (used_p.c_str (), F_OK) == 0)

> +    return;

> +

> +  std::string cache = get_standard_config_dir ();

> +  if (cache.empty ())

> +    {

> +      warning (_("Cannot determine GDB config directory."));

> +    }

> +  else if (!mkdir_recursive (cache.c_str ())

> +	   || open (used_p.c_str (), O_CREAT, 0) < 0)

> +    {

> +      warning (_("Cannot create debuginfod config file: %s"),

> +	       safe_strerror (errno));

> +    }

> +

> +  printf_filtered ("\nThis GDB is configured to auto-download debuginfo from:\n%s\n\n",

> +		   urls);


Hi,

I am not entirely sure where GDB stands with regards to
internationalization, but since you use _() above in the patch, I guess this
string could also qualify for translation and should be wrapped with _().

Best,
Lancelot.

> +  return;

> +}

> +

>  static debuginfod_client *

>  get_debuginfod_client ()

>  {

> @@ -114,7 +150,10 @@ get_debuginfod_client ()

>        global_client.reset (debuginfod_begin ());

>  

>        if (global_client != nullptr)

> -	debuginfod_set_progressfn (global_client.get (), progressfn);

> +	{

> +	  notify_first_use ();

> +	  debuginfod_set_progressfn (global_client.get (), progressfn);

> +	}

>      }

>  

>    return global_client.get ();

> -- 

> 2.31.1

>
will schmidt via Gdb-patches July 22, 2021, 3:26 p.m. | #2
> @@ -104,6 +105,41 @@ progressfn (debuginfod_client *c, long cur, long total)

>    return 0;

>  }

>  

> +/* If this is the first time that GDB is using debuginfod on this system, emit

> +   a warning message indicating that GDB may automatically download debuginfo.

> +

> +   To determine whether debuginfod is being used for the first time, check for

> +   the presence of a 'debuginfod-used-p' file in the GDB config directory.  If

> +   it is not found, then try to create the file and print the warning.  */

> +

> +static void

> +notify_first_use ()

> +{

> +  const char *urls = getenv (DEBUGINFOD_URLS_ENV_VAR);

> +  if (urls == nullptr)

> +    return;

> +

> +  std::string used_p = get_standard_config_filename ("debuginfod-used-p");

> +  if (access (used_p.c_str (), F_OK) == 0)

> +    return;

> +

> +  std::string cache = get_standard_config_dir ();


cache -> config_dir (the cache dir is something else).

> +  if (cache.empty ())

> +    {

> +      warning (_("Cannot determine GDB config directory."));

> +    }


Remove braces for single statements.

> +  else if (!mkdir_recursive (cache.c_str ())

> +	   || open (used_p.c_str (), O_CREAT, 0) < 0)


This leaks the fd.  Not dramatic, but since open fds are a limited
resource, let's avoid leaving it open.

Use gdb_open_cloexec and store the result in a scoped_fd (not sure why
gdb_open_cloexec doesn't return a scoped_fd already, I'll look into it).

Simon

Patch

diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
index 2d626e335a0..2de703cc138 100644
--- a/gdb/debuginfod-support.c
+++ b/gdb/debuginfod-support.c
@@ -22,6 +22,7 @@ 
 #include "gdbsupport/scoped_fd.h"
 #include "debuginfod-support.h"
 #include "gdbsupport/gdb_optional.h"
+#include "gdbsupport/pathstuff.h"
 
 #ifndef HAVE_LIBDEBUGINFOD
 scoped_fd
@@ -104,6 +105,41 @@  progressfn (debuginfod_client *c, long cur, long total)
   return 0;
 }
 
+/* If this is the first time that GDB is using debuginfod on this system, emit
+   a warning message indicating that GDB may automatically download debuginfo.
+
+   To determine whether debuginfod is being used for the first time, check for
+   the presence of a 'debuginfod-used-p' file in the GDB config directory.  If
+   it is not found, then try to create the file and print the warning.  */
+
+static void
+notify_first_use ()
+{
+  const char *urls = getenv (DEBUGINFOD_URLS_ENV_VAR);
+  if (urls == nullptr)
+    return;
+
+  std::string used_p = get_standard_config_filename ("debuginfod-used-p");
+  if (access (used_p.c_str (), F_OK) == 0)
+    return;
+
+  std::string cache = get_standard_config_dir ();
+  if (cache.empty ())
+    {
+      warning (_("Cannot determine GDB config directory."));
+    }
+  else if (!mkdir_recursive (cache.c_str ())
+	   || open (used_p.c_str (), O_CREAT, 0) < 0)
+    {
+      warning (_("Cannot create debuginfod config file: %s"),
+	       safe_strerror (errno));
+    }
+
+  printf_filtered ("\nThis GDB is configured to auto-download debuginfo from:\n%s\n\n",
+		   urls);
+  return;
+}
+
 static debuginfod_client *
 get_debuginfod_client ()
 {
@@ -114,7 +150,10 @@  get_debuginfod_client ()
       global_client.reset (debuginfod_begin ());
 
       if (global_client != nullptr)
-	debuginfod_set_progressfn (global_client.get (), progressfn);
+	{
+	  notify_first_use ();
+	  debuginfod_set_progressfn (global_client.get (), progressfn);
+	}
     }
 
   return global_client.get ();