From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gnu.wildebeest.org (gnu.wildebeest.org [45.83.234.184]) by sourceware.org (Postfix) with ESMTPS id B4B4A3858C5E for ; Wed, 5 Jul 2023 12:34:50 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org B4B4A3858C5E Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=klomp.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=klomp.org Received: from r6.localdomain (82-217-174-174.cable.dynamic.v4.ziggo.nl [82.217.174.174]) (using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by gnu.wildebeest.org (Postfix) with ESMTPSA id 4DF2B3000D0D; Wed, 5 Jul 2023 14:34:48 +0200 (CEST) Received: by r6.localdomain (Postfix, from userid 1000) id 2527734007A; Wed, 5 Jul 2023 14:34:48 +0200 (CEST) Message-ID: <8cc40db88fe82f506f03c8fab9f1d72175cef16f.camel@klomp.org> Subject: Re: PR29472: debuginfod metadata query From: Mark Wielaard To: "Frank Ch. Eigler" , elfutils-devel@sourceware.org Date: Wed, 05 Jul 2023 14:34:47 +0200 In-Reply-To: References: Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable User-Agent: Evolution 3.48.3 (3.48.3-1.fc38) MIME-Version: 1.0 X-Spam-Status: No, score=-3036.0 required=5.0 tests=BAYES_00,GIT_PATCH_0,JMQ_SPF_NEUTRAL,KAM_DMARC_STATUS,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: 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. >=20 > 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. > =20 > This file is free software; you can redistribute it and/or modify > @@ -31,6 +31,9 @@ > #include > #include > =20 > +#ifdef HAVE_JSON_C > + #include > +#endif Why? No json-c functions seem to be used. > /* Name and version of program. */ > ARGP_PROGRAM_VERSION_HOOK_DEF =3D print_version; > @@ -50,7 +53,11 @@ static const char args_doc[] =3D 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 =20 > + "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 the= re. */ > unsigned char* build_id =3D (unsigned char*) argv[remaining+1]; > int build_id_len =3D 0; /* assume text */ > + Elf* elf =3D NULL; > =20 > + /* Process optional buildid given via ELF file name, for some query ty= pes only. */ > + if (strcmp(argv[remaining], "debuginfo") =3D=3D 0 > + || strcmp(argv[remaining], "executable") =3D=3D 0 > + || strcmp(argv[remaining], "source") =3D=3D 0 > + || strcmp(argv[remaining], "section") =3D=3D 0) > + { > int any_non_hex =3D 0; > int i; > for (i =3D 0; build_id[i] !=3D '\0'; i++) > @@ -156,7 +170,6 @@ main(int argc, char** argv) > any_non_hex =3D 1; > =20 > int fd =3D -1; > - Elf* elf =3D NULL; > if (any_non_hex) /* raw build-id */ > { > fd =3D 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", bu= ild_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 =3D 0; > @@ -221,6 +235,19 @@ main(int argc, char** argv) > rc =3D debuginfod_find_section(client, build_id, build_id_len, > argv[remaining+2], &cache_name); > } > +#ifdef HAVE_JSON_C > + else if (strcmp(argv[remaining], "metadata") =3D=3D 0) /* no buildid! = */ > + { > + if (remaining+2 =3D=3D argc) > + { > + fprintf(stderr, "Require KEY and VALUE for \"metadata\"\n"); > + return 1; > + } > + =20 > + rc =3D debuginfod_find_metadata (client, argv[remaining+1], argv[r= emaining+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 >=3D 0) > - close (fd); > =20 > 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=3Dkey&value=3Dvalue The file name contains a & which should be escaped (in the shell). Could we use something else? Cheers, Mark