Hi Frank,
A quick review of debuginfod-find.
On Wed, 2023-04-12 at 16:31 -0400, Frank Ch. Eigler via Elfutils-devel
wrote:
> + * debuginfod-find.c (main): Handle metadata queries.
>
> diff --git a/debuginfod/debuginfod-find.c b/debuginfod/debuginfod-find.c
> index 307310988c4c..2df6436d99a2 100644
> --- a/debuginfod/debuginfod-find.c
> +++ b/debuginfod/debuginfod-find.c
> @@ -1,6 +1,6 @@
> /* Command-line frontend for retrieving ELF / DWARF / source files
> from the debuginfod.
> - Copyright (C) 2019-2020 Red Hat, Inc.
> + Copyright (C) 2019-2023 Red Hat, Inc.
> This file is part of elfutils.
>
> This file is free software; you can redistribute it and/or modify
> @@ -31,6 +31,9 @@
> #include <gelf.h>
> #include <libdwelf.h>
>
> +#ifdef HAVE_JSON_C
> + #include <json-c/json.h>
> +#endif
Why? No json-c functions seem to be used.
> /* Name and version of program. */
> ARGP_PROGRAM_VERSION_HOOK_DEF = print_version;
> @@ -50,7 +53,11 @@ static const char args_doc[] = N_("debuginfo BUILDID\n"
> "source BUILDID /FILENAME\n"
> "source PATH /FILENAME\n"
> "section BUILDID SECTION-NAME\n"
> - "section PATH SECTION-NAME\n");
> + "section PATH SECTION-NAME\n"
> +#ifdef HAVE_JSON_C
> + "metadata KEY VALUE\n"
> +#endif
> + );
Could some example KEY/VALUE pairs be added to the help?
> /* Definitions of arguments for argp functions. */
> @@ -145,7 +152,14 @@ main(int argc, char** argv)
> /* If we were passed an ELF file name in the BUILDID slot, look in there.
> */
> unsigned char* build_id = (unsigned char*) argv[remaining+1];
> int build_id_len = 0; /* assume text */
> + Elf* elf = NULL;
>
> + /* Process optional buildid given via ELF file name, for some query types
> only. */
> + if (strcmp(argv[remaining], "debuginfo") == 0
> + || strcmp(argv[remaining], "executable") == 0
> + || strcmp(argv[remaining], "source") == 0
> + || strcmp(argv[remaining], "section") == 0)
> + {
> int any_non_hex = 0;
> int i;
> for (i = 0; build_id[i] != '\0'; i++)
> @@ -156,7 +170,6 @@ main(int argc, char** argv)
> any_non_hex = 1;
>
> int fd = -1;
> - Elf* elf = NULL;
> if (any_non_hex) /* raw build-id */
> {
> fd = open ((char*) build_id, O_RDONLY);
> @@ -184,6 +197,7 @@ main(int argc, char** argv)
> else
> fprintf (stderr, "Cannot extract build-id from %s: %s\n",
> build_id, elf_errmsg(-1));
> }
> + }
OK, but shouldn't we have some kind of check for valid KEYs in the
metadata case?
> char *cache_name;
> int rc = 0;
> @@ -221,6 +235,19 @@ main(int argc, char** argv)
> rc = debuginfod_find_section(client, build_id, build_id_len,
> argv[remaining+2], &cache_name);
> }
> +#ifdef HAVE_JSON_C
> + else if (strcmp(argv[remaining], "metadata") == 0) /* no buildid! */
> + {
> + if (remaining+2 == argc)
> + {
> + fprintf(stderr, "Require KEY and VALUE for \"metadata\"\n");
> + return 1;
> + }
> +
> + rc = debuginfod_find_metadata (client, argv[remaining+1],
> argv[remaining+2],
> + &cache_name);
> + }
> +#endif
Why the HAVE_JSON_C guard? debuginfod_find_metadata will return -ENOSYS
if there is no support doesn't it?
> else
> {
> argp_help (&argp, stderr, ARGP_HELP_USAGE, argv[0]);
> @@ -240,8 +267,6 @@ main(int argc, char** argv)
> debuginfod_end (client);
> if (elf)
> elf_end(elf);
> - if (fd >= 0)
> - close (fd);
>
> if (rc < 0)
> {
Why remove the close?
Also should the output of an metadata query really be the
debuginfod_client cache file? I would at least expect an indication
whether the query got any results (does the file contain just an empty
[ ] array?).
All the results now look like:
/home/mark/.cache/debuginfod_client/metadata/key=key&value=value
The file name contains a & which should be escaped (in the shell).
Could we use something else?
Cheers,
Mark
