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

[Aaron Merey <amerey@redhat.com>]

On Wed, Aug 12, 2020 at 5:45 PM Simon Marchi <simark@simark.ca> wrote:
> Hi Aaron,
>
> Thanks, just a few minor things, the patch LGTM with those fixed.
>
> > 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)
>
> There's no need for the `has_printed` parameter.  Just initialize the `has_printed` field to false below.
>
> > +  { }
> > +
> > +  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);
>
> Omit the `struct` keyword when referring to the new type.
>
> >
> > +  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);
>
> Same here.

Thanks Simon, I pushed the patch with these changes.

Aaron