Hi Frank,
LGTM, please go ahead and merge the patch with these changes.
Aaron
On Sat, Jun 1, 2024 at 11:26 AM Frank Ch. Eigler <f...@redhat.com> wrote:
>
> Hi -
>
> > Thanks for the patch, some suggestions below.
>
> Thanks, adding the following 'git diff -w' to the patch you reviewed:
>
> > [...]
> > I was experimenting with metadata queries to a local server that indexed
> > a single rpm (binutils-2.41-8.fc40.x86_64.rpm). The following commands
> > produced JSON with empty "results":
> >
> > debuginfod-find metadata glob '*'
> > debuginfod-find metadata glob '/usr*'
> > debuginfod-find metadata glob '/usr/bin*'
> >
> > Using the glob '/usr/bin/*' did produce results with complete metadata.
> >
> > I haven't looked into the cause but this seems like a bug. I'd expect
> > the first 3 globs to return at least as many results as the 4th. If this is
> > intentional behaviour then I think it should be documented.
>
> This is intentional. When you use glob patterns like '*' or '/usr*'
> in the shell, you don't get recursive path name lists either.
> Addressed in the documentation via reference to shell /
> fnmatch(FNM_PATHNAME).
>
>
> diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
> index c75abadf7dce..3d6f8d8c4bea 100644
> --- a/debuginfod/debuginfod-client.c
> +++ b/debuginfod/debuginfod-client.c
> @@ -72,7 +72,7 @@ int debuginfod_find_section (debuginfod_client *c, const
> unsigned char *b,
> int s, const char *scn, char **p)
> { return -ENOSYS; }
> int debuginfod_find_metadata (debuginfod_client *c,
> - const char *k, char *v, char **p) { return
> -ENOSYS; }
> + const char *k, const char *v, char **p) {
> return -ENOSYS; }
> void debuginfod_set_progressfn(debuginfod_client *c,
> debuginfod_progressfn_t fn) { }
> void debuginfod_set_verbose_fd(debuginfod_client *c, int fd) { }
> @@ -858,22 +858,12 @@ init_server_urls(char* url_subdir, const char* type,
> char *server_url = strtok_r(server_urls, url_delim, &strtok_saveptr);
> /* Count number of URLs. */
> int n = 0;
> - assert (0 == strcmp(url_subdir, "buildid") || 0 == strcmp(url_subdir,
> "metadata"));
>
> while (server_url != NULL)
> {
> - int r;
> - char *tmp_url;
> - if (strlen(server_url) > 1 && server_url[strlen(server_url)-1] == '/')
> - r = asprintf(&tmp_url, "%s%s", server_url, url_subdir);
> - else
> - r = asprintf(&tmp_url, "%s/%s", server_url, url_subdir);
> -
> - if (r == -1)
> - return -ENOMEM;
> -
> - // When we encounted a (well-formed) token off the form ima:foo, we
> update the policy
> - // under which results from that server will be ima verified
> + // When we encountered a (well-formed) token off the form
> + // ima:foo, we update the policy under which results from that
> + // server will be ima verified
> if (startswith(server_url, "ima:"))
> {
> #ifdef ENABLE_IMA_VERIFICATION
> @@ -898,6 +888,17 @@ init_server_urls(char* url_subdir, const char* type,
> goto continue_next_url;
> }
>
> + // Construct actual URL for libcurl
> + int r;
> + char *tmp_url;
> + if (strlen(server_url) > 1 && server_url[strlen(server_url)-1] == '/')
> + r = asprintf(&tmp_url, "%s%s", server_url, url_subdir);
> + else
> + r = asprintf(&tmp_url, "%s/%s", server_url, url_subdir);
> +
> + if (r == -1)
> + return -ENOMEM;
> +
> /* PR 27983: If the url is duplicate, skip it */
> int url_index;
> for (url_index = 0; url_index < n; ++url_index)
> @@ -1025,7 +1026,7 @@ init_handle(debuginfod_client *client,
>
> /*
> * This function busy-waits on one or more curl queries to complete. This can
> - * be controled via only_one, which, if true, will find the first winner and
> exit
> + * be controlled via only_one, which, if true, will find the first winner
> and exit
> * once found. If positive maxtime and maxsize dictate the maximum allowed
> wait times
> * and download sizes respectively. Returns 0 on success and -Posix error on
> failure.
> */
> @@ -1044,7 +1045,7 @@ perform_queries(CURLM *curlm, CURL **target_handle,
> struct handle_data *data, de
> c->winning_headers = NULL;
> }
> if (maxtime > 0 && clock_gettime(CLOCK_MONOTONIC_RAW, &start_time) == -1)
> - return errno;
> + return -errno;
> long delta = 0;
> do
> {
> @@ -1052,7 +1053,7 @@ perform_queries(CURLM *curlm, CURL **target_handle,
> struct handle_data *data, de
> if (maxtime > 0)
> {
> if (clock_gettime(CLOCK_MONOTONIC_RAW, &cur_time) == -1)
> - return errno;
> + return -errno;
> delta = cur_time.tv_sec - start_time.tv_sec;
> if ( delta > maxtime)
> {
> @@ -1108,9 +1109,7 @@ perform_queries(CURLM *curlm, CURL **target_handle,
> struct handle_data *data, de
> }
>
> long dl_size = -1;
> - if (only_one && target_handle)
> - { // Only bother with progress functions if we're retrieving exactly
> 1 file
> - if (*target_handle && (c->progressfn || maxsize > 0))
> + if (target_handle && *target_handle && (c->progressfn || maxsize > 0))
> {
> /* Get size of file being downloaded. NB: If going through
> deflate-compressing proxies, this number is likely to be
> @@ -1148,7 +1147,7 @@ perform_queries(CURLM *curlm, CURL **target_handle,
> struct handle_data *data, de
> {
> loops ++;
> long pa = loops; /* default param for progress callback */
> - if (*target_handle) /* we've committed to a server; report its
> download progress */
> + if (target_handle && *target_handle) /* we've committed to a
> server; report its download progress */
> {
> /* PR30809: Check actual size of cached file. This same
> fd is shared by all the multi-curl handles (but only
> @@ -1186,7 +1185,6 @@ perform_queries(CURLM *curlm, CURL **target_handle,
> struct handle_data *data, de
> break;
> }
> }
> - }
> /* Check to see if we are downloading something which exceeds maxsize,
> if set.*/
> if (target_handle && *target_handle && dl_size > maxsize && maxsize >
> 0)
> {
> @@ -2050,7 +2048,6 @@ debuginfod_query_server_by_buildid (debuginfod_client
> *c,
> retry_limit = atoi (retry_limit_envvar);
>
> CURLM *curlm = c->server_mhandle;
> - assert (curlm != NULL);
>
> /* Tracks which handle should write to fd. Set to the first
> handle that is ready to write the target file to the cache. */
> @@ -2637,13 +2634,8 @@ debuginfod_find_section (debuginfod_client *client,
>
>
> int debuginfod_find_metadata (debuginfod_client *client,
> - const char* key, char* value, char **path)
> + const char* key, const char* value, char
> **path)
> {
> - (void) client;
> - (void) key;
> - (void) value;
> - (void) path;
> -
> char *server_urls = NULL;
> char *urls_envvar = NULL;
> char *cache_path = NULL;
> @@ -2718,7 +2710,10 @@ int debuginfod_find_metadata (debuginfod_client
> *client,
> /* Check if we have a recent result already in the cache. */
> cache_path = make_cache_path();
> if (! cache_path)
> + {
> + rc = -ENOMEM;
> goto out;
> + }
> xalloc_str (target_cache_dir, "%s/metadata", cache_path);
> (void) mkdir (target_cache_dir, 0700);
> xalloc_str (target_cache_path, "%s/%s", target_cache_dir,
> target_file_name);
> @@ -2823,7 +2818,6 @@ int debuginfod_find_metadata (debuginfod_client *client,
> }
>
> CURLM *curlm = client->server_mhandle;
> - assert (curlm != NULL);
>
> CURL *target_handle = NULL;
> data = malloc(sizeof(struct handle_data) * num_urls);
> diff --git a/debuginfod/debuginfod-find.c b/debuginfod/debuginfod-find.c
> index b0a7c2360dd8..0ef80377a81b 100644
> --- a/debuginfod/debuginfod-find.c
> +++ b/debuginfod/debuginfod-find.c
> @@ -243,6 +243,8 @@ main(int argc, char** argv)
>
> rc = debuginfod_find_metadata (client, argv[remaining+1],
> argv[remaining+2],
> &cache_name);
> + if (rc >= 0)
> + {
> /* We output a pprinted JSON object, not the regular
> debuginfod-find cached file path */
> print_cached_file = false;
> json_object *metadata = json_object_from_file(cache_name);
> @@ -262,6 +264,7 @@ main(int argc, char** argv)
> return 1;
> }
> }
> + }
> else
> {
> argp_help (&argp, stderr, ARGP_HELP_USAGE, argv[0]);
> diff --git a/debuginfod/debuginfod.h.in b/debuginfod/debuginfod.h.in
> index 3936b17b97cf..0a6a4a22efd9 100644
> --- a/debuginfod/debuginfod.h.in
> +++ b/debuginfod/debuginfod.h.in
> @@ -97,17 +97,12 @@ int debuginfod_find_section (debuginfod_client *client,
> successful, set *path to a strdup'd copy of the name of the same
> file in the cache. Caller must free() it later.
>
> - key can be one of 'glob' or 'file' corresponding to querying for value
> - by exact name or using a pattern matching approach.
> -
> - The JSON document will be of the form {results: [{...}, ...], complete:
> <bool>},
> - where the results are JSON objects containing metadata and complete is
> true iff
> - all of the federation of servers responded with complete results (as
> opposed to 1+
> - failing to return or having an issue)
> + See the debuginfod-find(1) man page for examples of the supported types
> + of key/value queries and their JSON results.
> */
> int debuginfod_find_metadata (debuginfod_client *client,
> const char *key,
> - char* value,
> + const char* value,
> char **path);
>
> typedef int (*debuginfod_progressfn_t)(debuginfod_client *c, long a, long b);
> diff --git a/doc/Makefile.am b/doc/Makefile.am
> index 87de4f0beb7f..0c094af2289b 100644
> --- a/doc/Makefile.am
> +++ b/doc/Makefile.am
> @@ -39,6 +39,7 @@ 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_find_section.3
> +notrans_dist_man3_MANS += debuginfod_find_metadata.3
> notrans_dist_man3_MANS += debuginfod_get_user_data.3
> notrans_dist_man3_MANS += debuginfod_get_url.3
> notrans_dist_man3_MANS += debuginfod_set_progressfn.3
> diff --git a/doc/debuginfod-find.1 b/doc/debuginfod-find.1
> index 8c63b2c5a5e0..38b5b0184dfe 100644
> --- a/doc/debuginfod-find.1
> +++ b/doc/debuginfod-find.1
> @@ -123,16 +123,17 @@ l l.
>
> .SS metadata \fIKEY\fP \fIVALUE\fP
>
> -All designated debuginfod servers are queried for metadata about files
> -in their index. Different search keys may be supported by different
> -servers.
> +All designated debuginfod servers are queried for metadata about all
> +files that match a given key/value query in their index. The results
> +include names and buildids, which may be used in future queries to
> +fetch actual files.
>
> .TS
> l l l .
> KEY VALUE DESCRIPTION
>
> -\fBfile\fP path exact match \fIpath\fP, including in archives
> -\fBglob\fP pattern glob match \fIpattern\fP, including in archives
> +\fBfile\fP \fIpath\fP exact match \fIpath\fP, including in archives
> +\fBglob\fP \fIpattern\fP shell-style glob match \fIpattern\fP,
> including in archives, as in fnmatch(FNM_PATHNAME)
> .TE
>
> The resulting output will look something like the following
> diff --git a/doc/debuginfod_find_debuginfo.3 b/doc/debuginfod_find_debuginfo.3
> index 4e359c8c4bd4..589a2c2b63b4 100644
> --- a/doc/debuginfod_find_debuginfo.3
> +++ b/doc/debuginfod_find_debuginfo.3
> @@ -48,6 +48,10 @@ LOOKUP FUNCTIONS
> .BI " int " build_id_len ","
> .BI " const char * " section ","
> .BI " char ** " path ");"
> +.BI "int debuginfod_find_metadata(debuginfod_client *" client ","
> +.BI " const char *" key ","
> +.BI " const char *" value ","
> +.BI " char ** " path ");"
>
>
> OPTIONAL FUNCTIONS
> @@ -114,6 +118,14 @@ section queries, debuginfod_find_section may query the
> server for the
> debuginfo and/or executable with \fIbuild_id\fP in order to retrieve
> and extract the section.
>
> +.BR debuginfod_find_metadata ()
> +queries all debuginfod server URLs contained in
> +.BR $DEBUGINFOD_URLS
> +for metadata for all matches of a given key/value query against files
> +in their indexes. The resulting file is a JSON document. See the
> +\fIdebuginfod-find(1)\fP man page for examples of the supported types
> +of key/value queries and their JSON results.
> +
> If \fIpath\fP is not NULL and the query is successful, \fIpath\fP is set
> to the path of the file in the cache. The caller must \fBfree\fP() this
> value.
>
> diff --git a/doc/debuginfod_find_metadata.3 b/doc/debuginfod_find_metadata.3
> new file mode 100644
> index 000000000000..16279936e2ea
> --- /dev/null
> +++ b/doc/debuginfod_find_metadata.3
> @@ -0,0 +1 @@
> +.so man3/debuginfod_find_debuginfo.3
> diff --git a/tests/run-debuginfod-find-metadata.sh
> b/tests/run-debuginfod-find-metadata.sh
> index f19c5a6e6942..78a34f09490f 100755
> --- a/tests/run-debuginfod-find-metadata.sh
> +++ b/tests/run-debuginfod-find-metadata.sh
> @@ -1,6 +1,6 @@
> #!/usr/bin/env bash
> #
> -# Copyright (C) 2022 Red Hat, Inc.
> +# Copyright (C) 2022-2024 Red Hat, Inc.
> # This file is part of elfutils.
> #
> # This file is free software; you can redistribute it and/or modify
>
>
>
>
> - FChE
>
