[PATCH] debuginfod-support.c: Use long-lived debuginfod_client

[Aaron Merey <amerey@redhat.com>]

On Thu, May 6, 2021 at 3:12 PM Simon Marchi <simon.marchi@polymtl.ca> wrote:
>
> On 2021-05-06 2:47 p.m., Frank Ch. Eigler wrote:> Hi -
> >
> >> I think it would still be a good idea to properly close the client
> >> at exit.  Not because the memory leak is a concern, but because we
> >> shouldn't assume what closing a debuginfod client (current or
> >> future) does or does not do, so we shouldn't assume that we can get 
> >> away without calling debuginfo_end.
> >
> > If it's not hard to do, sure .... but wouldn't that necessitate
> > exposing the pointer from function static scope again?
>
> I don't think so, just make the static variable a unique pointer (the
> type that you are removing in this patch).  The destructor of the 
> unique_ptr will be called at program exit, closing the client cleanly.
> That variable can be declared inside debuginfod_init, like Tom 
> suggested.  Something like:
>
> static debuginfo_client *
> debuginfod_init ()
> {
>   static debuginfod_client_up global_client;
>
>   if (global_client == nullptr)
>     global_client.reset (debuginfod_begin ());
>
>   return global_client.get (); 
> }
>
> In that case, the debuginfod_init function should probably be renamed to
> something like get_debuginfod_client, something like that.
>
> >> A debuginfod client could eventually maintain some temporary files
> >> that need to be deleted when closing in order not to litter /tmp,
> >> some cache files need to be flushed, etc.
> >
> > I suspect that if we were to do any of those things in the future,
> > we'd keep things clean by techniques like holding onto fd's to
> > unlinked files, so process exit is enough.  (We could make a
> > commitment in the docs.)
>
> That sounds like a good idea, this way even if the user of the client
> crashes, there won't be any leftovers.  However, committing to that in
> the documentation just sounds like an unnecessary way to paint yourself
> in a corner.

Even if we ensure that debuginfod_end() properly closes libdebuginfod
internals, I'd still like to use the debuginfod_client unique_ptr the 
way Simon described.  This design is more extensible if in the future we
need to safely close other debuginfod-related resources on the gdb-side
at program exit. 

I've included the updated patch below.

Aaron

---

Instead of initializing a new debuginfod_client for each query, store
the first initialized client for the remainder of the GDB session and
use it for every debuginfod query.

In conjunction with upcoming changes to libdebuginfod, using one client
for all queries will avoid latency caused by unneccesarily setting up
TCP connections multiple times.

Tested on Fedora 33 x86_64.

gdb/ChangeLog:

	* debuginfod-support.c (debuginfod_init): Remove.
	(get_debuginfod_client): New function.
---
 gdb/debuginfod-support.c | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
index 9778e2e4cfe..2d626e335a0 100644
--- a/gdb/debuginfod-support.c
+++ b/gdb/debuginfod-support.c
@@ -72,6 +72,7 @@ static int
 progressfn (debuginfod_client *c, long cur, long total)
 {
   user_data *data = static_cast<user_data *> (debuginfod_get_user_data (c));
+  gdb_assert (data != nullptr);
 
   if (check_quit_flag ())
     {
@@ -103,15 +104,20 @@ progressfn (debuginfod_client *c, long cur, long total)
   return 0;
 }
 
-static debuginfod_client_up
-debuginfod_init ()
+static debuginfod_client *
+get_debuginfod_client ()
 {
-  debuginfod_client_up c (debuginfod_begin ());
+  static debuginfod_client_up global_client;
 
-  if (c != nullptr)
-    debuginfod_set_progressfn (c.get (), progressfn);
+  if (global_client == nullptr)
+    {
+      global_client.reset (debuginfod_begin ());
+
+      if (global_client != nullptr)
+	debuginfod_set_progressfn (global_client.get (), progressfn);
+    }
 
-  return c;
+  return global_client.get ();
 }
 
 /* See debuginfod-support.h  */
@@ -126,19 +132,20 @@ debuginfod_source_query (const unsigned char *build_id,
   if (urls_env_var == NULL || urls_env_var[0] == '\0')
     return scoped_fd (-ENOSYS);
 
-  debuginfod_client_up c = debuginfod_init ();
+  debuginfod_client *c = get_debuginfod_client ();
 
   if (c == nullptr)
     return scoped_fd (-ENOMEM);
 
   user_data data ("source file", srcpath);
 
-  debuginfod_set_user_data (c.get (), &data);
-  scoped_fd fd (debuginfod_find_source (c.get (),
+  debuginfod_set_user_data (c, &data);
+  scoped_fd fd (debuginfod_find_source (c,
 					build_id,
 					build_id_len,
 					srcpath,
 					nullptr));
+  debuginfod_set_user_data (c, nullptr);
 
   /* TODO: Add 'set debug debuginfod' command to control when error messages are shown.  */
   if (fd.get () < 0 && fd.get () != -ENOENT)
@@ -164,7 +171,7 @@ debuginfod_debuginfo_query (const unsigned char *build_id,
   if (urls_env_var == NULL || urls_env_var[0] == '\0')
     return scoped_fd (-ENOSYS);
 
-  debuginfod_client_up c = debuginfod_init ();
+  debuginfod_client *c = get_debuginfod_client ();
 
   if (c == nullptr)
     return scoped_fd (-ENOMEM);
@@ -172,9 +179,10 @@ debuginfod_debuginfo_query (const unsigned char *build_id,
   char *dname = nullptr;
   user_data data ("separate debug info for", filename);
 
-  debuginfod_set_user_data (c.get (), &data);
-  scoped_fd fd (debuginfod_find_debuginfo (c.get (), build_id, build_id_len,
+  debuginfod_set_user_data (c, &data);
+  scoped_fd fd (debuginfod_find_debuginfo (c, build_id, build_id_len,
 					   &dname));
+  debuginfod_set_user_data (c, nullptr);
 
   if (fd.get () < 0 && fd.get () != -ENOENT)
     printf_filtered (_("Download failed: %s.  Continuing without debug info for %ps.\n"),
-- 
2.30.2