Hi Frank,
Sorry, I missed the earlier, the mailinglist setup changed and
suppressed the email to the list because you had CCed me... oops.
I think I fixed it for me personally, but others might want to get a
password reminder for this list and check that "Avoid duplicate copies
of messages?" hasn't been accidentally turned to "Yes".
On Thu, 2020-03-19 at 20:31 -0400, Frank Ch. Eigler wrote:
> commit d307089fa8621edf09d66665d0a1ffc123b904b2
> Author: Frank Ch. Eigler <f...@redhat.com>
> Date: Thu Mar 19 20:27:11 2020 -0400
>
> debuginfod client API: add get/set user_data functions
>
> Add a pair of functions to associate a void* parameter with a client
> object. Requested by GDB team as a way to pass file names and such
> user-interface data through to a progressfn callback.
Looks good, but missing a ChangeLog entry, so it is slightly harder to
see which changes were intended. In general looks good. I had rather
seen a separate testcase than reusing debuginfod-find for that. Not
that there is that much to test.
One suggestion, it be a good idea IMHO to initialize user_data to NULL.
Just so a client knows whether or not it has been set or it is a random
value.
> diff --git a/debuginfod/debuginfod-find.c b/debuginfod/debuginfod-find.c
> index 8bd3a3dba7a0..0b1ca9cf1d76 100644
> --- a/debuginfod/debuginfod-find.c
> +++ b/debuginfod/debuginfod-find.c
> [...]
> @@ -130,6 +133,8 @@ main(int argc, char** argv)
> return 1;
> }
>
> + debuginfod_end (client);
> +
> if (rc < 0)
> {
> fprintf(stderr, "Server query failed: %s\n", strerror(-rc));
> @@ -137,9 +142,7 @@ main(int argc, char** argv)
> }
>
> printf("%s\n", cache_name);
> -
> free (cache_name);
> - debuginfod_end (client);
>
> return 0;
> }
Why is the debuginfo_end () call moved?
It is hard to see in this diff whether there are any uses of the client
handle in between the old and new location. But it looks there is none,
so this should be fine.
> diff --git a/doc/Makefile.am b/doc/Makefile.am
> index b5db01ff75d5..87d1fee03302 100644
> --- a/doc/Makefile.am
> +++ b/doc/Makefile.am
> [...]
> @@ -24,7 +24,14 @@ notrans_dist_man1_MANS=
>
> if DEBUGINFOD
> notrans_dist_man8_MANS += debuginfod.8
> -notrans_dist_man3_MANS += debuginfod_find_debuginfo.3
> debuginfod_find_source.3 debuginfod_find_executable.3
> debuginfod_set_progressfn.3
> +notrans_dist_man3_MANS += debuginfod_begin.3
> +notrans_dist_man3_MANS += debuginfod_end.3
> +notrans_dist_man3_MANS += debuginfod_find_debuginfo.3
> +notrans_dist_man3_MANS += debuginfod_find_executable.3
> +notrans_dist_man3_MANS += debuginfod_find_source.3
> +notrans_dist_man3_MANS += debuginfod_get_user_data.3
> +notrans_dist_man3_MANS += debuginfod_set_progressfn.3
> +notrans_dist_man3_MANS += debuginfod_set_user_data.3
> notrans_dist_man1_MANS += debuginfod-find.1
> endif
What exactly is going on here?
Did we forget some, so they didn't get distributed?
> diff --git a/doc/debuginfod_find_debuginfo.3
> b/doc/debuginfod_find_debuginfo.3
> index f9e770b0c530..d975927f757b 100644
> --- a/doc/debuginfod_find_debuginfo.3
> +++ b/doc/debuginfod_find_debuginfo.3
> @@ -21,9 +21,15 @@ debuginfod_find_debuginfo \- request debuginfo
> from debuginfod
> .nf
> .B #include <elfutils/debuginfod.h>
> .PP
> +Link with \fB-ldebuginfod\fP.
Yes, this looks like a better location for that note.
> +.SS "USER DATA POINTER"
> +
> +A single \fIvoid *\fP pointer associated with the connection handle
> +may be set any time via
> +.BR \%debuginfod_set_user_data () ,
> +and retrieved via
> +.BR \%debuginfod_get_user_data () .
> +The value may be NULL.
So, if we decide that the initial value is NULL then this would be
where we would document that or say that the value is undefined unless
debuginfod_set_user_data () has never been called.
Thanks,
Mark
