Re: PR29472: debuginfod metadata query

[Mark Wielaard <mark@klomp.org>]

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