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

Aaron Merey
2020-08-13T21:55:05Z

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