Re: [PATCH] debuginfod-support.c: Replace globals with user_data

[Aaron Merey <amerey@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2936 bytes --]

On Sat, Aug 1, 2020 at 10:45 AM Simon Marchi <simark@simark.ca> wrote:
>
> > From 1913317cb95d02c3244e5d91b7b59e33cfe991c5 Mon Sep 17 00:00:00 2001
> > From: Aaron Merey <amerey@redhat.com>
> > Date: Fri, 31 Jul 2020 18:25:48 -0400
> > Subject: [PATCH] debuginfod-support.c: Replace globals with user_data
> >
> > Store query information in user_data struct instead of global
> > variables. Also include DEBUGINFOD_CFLAGS in INTERNAL_CFLAG_BASE.
>
> The DEBUGINFOD_CFLAGS change looks good, but it's unrelated to the topic of this patch,
> please make it its own patch, with its own justification.
>
> > diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
> > index f4a227b040..9325bf8651 100644
> > --- a/gdb/debuginfod-support.c
> > +++ b/gdb/debuginfod-support.c
> > @@ -43,29 +43,39 @@ debuginfod_debuginfo_query (const unsigned char *build_id,
> >  #else
> >  #include <elfutils/debuginfod.h>
> >
> > -/* TODO: Use debuginfod API extensions instead of these globals.  */
> > -static std::string desc;
> > -static std::string fname;
> > -static bool has_printed;
> > +struct user_data
> > +{
> > +  std::string desc;
> > +  std::string fname;
> > +  bool has_printed;
> > +
> > +  user_data (std::string desc, std::string fname, bool has_printed)
> > +    : desc (desc), fname (fname), has_printed (has_printed)
> > +  { }
>
> Put the constructor before the fields.  Make the fields const if they are not intended
> to be changed once the object has been created.
>
> Using `std::string` causes unnecessary copying to happen.  Just use `const char *`
> (`const char * const` for the fields) or gdb::string_view, if you want to be more
> generic (but all callers are `const char *`, so it's not really necessary).  That's
> assuming that the lifetime of those strings will always be longer than the lifetime
> of the debuginfo client.  I think that's the case, since the debug info client is
> always created and destroyed in the same function.
>
> > +};
> >
> >  static int
> >  progressfn (debuginfod_client *c, long cur, long total)
> >  {
> > +  struct user_data *data = static_cast<user_data*> (debuginfod_get_user_data (c));
>
> This line is too long, just skip the `struct` keyword and you'll be fine.  Also, space
> before `*`.
>
> > +
> >    if (check_quit_flag ())
> >      {
> >        printf_filtered ("Cancelling download of %s %ps...\n",
> > -                    desc.c_str (),
> > -                    styled_string (file_name_style.style (), fname.c_str ()));
> > +                    data->desc.c_str (),
> > +                    styled_string (file_name_style.style (),
> > +                                   data->fname.c_str ()));
> >        return 1;
> >      }
> >
> > -  if (!has_printed && total != 0)
> > +  if (! data->has_printed && total != 0)
>
> Remove space after `!`.

Hi Simon,

Thanks for the review, I've updated the patch and moved the Makefile.in
change to a separate patch.

Aaron

[-- Attachment #2: 0001-debuginfod-support.c-Replace-globals-with-user_data.patch --]
[-- Type: text/x-patch, Size: 3422 bytes --]

From 922179058218a82f9b74e328249e67244adae3fc Mon Sep 17 00:00:00 2001
From: Aaron Merey <amerey@redhat.com>
Date: Wed, 12 Aug 2020 17:23:42 -0400
Subject: [PATCH 2/2] debuginfod-support.c: Replace globals with user_data

Store query information in user_data struct instead of global variables.

gdb/ChangeLog:

	* debuginfod-support.c: Replace global variables with user_data.
---
 gdb/ChangeLog            |  4 ++++
 gdb/debuginfod-support.c | 38 ++++++++++++++++++++++----------------
 2 files changed, 26 insertions(+), 16 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 0e035e233f..ad54c49864 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,7 @@
+2020-08-12  Aaron Merey  <amerey@redhat.com>
+
+	* debuginfod-support.c: Replace global variables with user_data.
+
 2020-08-12  Aaron Merey  <amerey@redhat.com>
 
 	* Makefile.in (DEBUGINFOD_CFLAGS, DEBUGINFOD_LIBS): New variables.
diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
index f4a227b040..3f51aaaf43 100644
--- a/gdb/debuginfod-support.c
+++ b/gdb/debuginfod-support.c
@@ -43,29 +43,37 @@ debuginfod_debuginfo_query (const unsigned char *build_id,
 #else
 #include <elfutils/debuginfod.h>
 
-/* TODO: Use debuginfod API extensions instead of these globals.  */
-static std::string desc;
-static std::string fname;
-static bool has_printed;
+struct user_data
+{
+  user_data (const char *desc, const char *fname, bool has_printed)
+    : desc (desc), fname (fname), has_printed (has_printed)
+  { }
+
+  const char * const desc;
+  const char * const fname;
+  bool has_printed;
+};
 
 static int
 progressfn (debuginfod_client *c, long cur, long total)
 {
+  user_data *data = static_cast<user_data *> (debuginfod_get_user_data (c));
+
   if (check_quit_flag ())
     {
       printf_filtered ("Cancelling download of %s %ps...\n",
-		       desc.c_str (),
-		       styled_string (file_name_style.style (), fname.c_str ()));
+		       data->desc,
+		       styled_string (file_name_style.style (), data->fname));
       return 1;
     }
 
-  if (!has_printed && total != 0)
+  if (!data->has_printed && total != 0)
     {
       /* Print this message only once.  */
-      has_printed = true;
+      data->has_printed = true;
       printf_filtered ("Downloading %s %ps...\n",
-		       desc.c_str (),
-		       styled_string (file_name_style.style (), fname.c_str ()));
+		       data->desc,
+		       styled_string (file_name_style.style (), data->fname));
     }
 
   return 0;
@@ -98,10 +106,9 @@ debuginfod_source_query (const unsigned char *build_id,
   if (c == nullptr)
     return scoped_fd (-ENOMEM);
 
-  desc = std::string ("source file");
-  fname = std::string (srcpath);
-  has_printed = false;
+  struct user_data data ("source file", srcpath, false);
 
+  debuginfod_set_user_data (c, &data);
   scoped_fd fd (debuginfod_find_source (c,
 					build_id,
 					build_id_len,
@@ -136,11 +143,10 @@ debuginfod_debuginfo_query (const unsigned char *build_id,
   if (c == nullptr)
     return scoped_fd (-ENOMEM);
 
-  desc = std::string ("separate debug info for");
-  fname = std::string (filename);
-  has_printed = false;
   char *dname = nullptr;
+  struct user_data data ("separate debug info for", filename, false);
 
+  debuginfod_set_user_data (c, &data);
   scoped_fd fd (debuginfod_find_debuginfo (c, build_id, build_id_len, &dname));
 
   if (fd.get () < 0 && fd.get () != -ENOENT)
-- 
2.25.4