patch 2/2 debuginfod server etc.

Fri Nov 15 14:40:00 GMT 2019

Hi,

On Mon, 2019-10-28 at 15:07 -0400, Frank Ch. Eigler wrote:
> Subject: [PATCH 2/2] debuginfod 2/2: server side
> 
> Add the server to the debuginfod/ subdirectory.  This is a highly
> multithreaded c++11 program (still buildable on rhel7's gcc 4.8,
> which is only partly c++11 compliant).  Includes an initial suite
> of tests, man pages, and a sample systemd service.

We already talked a bit on irc about the tests.

I had a problem with the testcase because at the end of the test (when
trapping exit 0) the working directory isn't empty. Seems sqlite leaves
some working files around that are cleaned up too late.

And I don't like the pretty big testfiles, which aren't self-contained. 
You have to fetch them from a remote koji server. It would be much
better to have a (small) self-contained .spec file that can be used to
generate the rpms. Frank is already working on that.

> diff --git a/tests/ChangeLog b/tests/ChangeLog
> index 97b8dedb03f5..ef5f2f0f1211 100644
> --- a/tests/ChangeLog
> +++ b/tests/ChangeLog
> @@ -1,3 +1,10 @@
> +2019-10-28  Aaron Merey  <amerey@redhat.com>
> +           Frank Ch. Eigler  <fche@redhat.com>
> +
> +       * run-debuginfod-find.sh, debuginfod_build_id_find.c: New test.
> +       * testfile-debuginfod-*.rpm.bz2: New data files for test.
> +       * Makefile.am: Run it.
> +
>  2019-09-02  Mark Wielaard  <mark@klomp.org>
>  
>         * run-readelf-s.sh: Add --dyn-syms case.
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index ad0855dec441..a1794493fda5 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -1,6 +1,6 @@
>  ## Process this file with automake to create Makefile.in
>  ##
> -## Copyright (C) 1996-2018 Red Hat, Inc.
> +## Copyright (C) 1996-2019 Red Hat, Inc.
>  ## This file is part of elfutils.
>  ##
>  ## This file is free software; you can redistribute it and/or modify
> @@ -190,6 +190,11 @@ check_PROGRAMS += $(asm_TESTS)
>  TESTS += $(asm_TESTS) run-disasm-bpf.sh
>  endif
>  
> +if DEBUGINFOD
> +check_PROGRAMS += debuginfod_build_id_find
> +TESTS += run-debuginfod-find.sh
> +endif

OK

>  EXTRA_DIST = run-arextract.sh run-arsymtest.sh run-ar.sh \
>              run-show-die-info.sh run-get-files.sh run-get-lines.sh \
>              run-next-files.sh run-next-lines.sh testfile-only-debug-line.bz2 \
> @@ -440,7 +445,10 @@ EXTRA_DIST = run-arextract.sh run-arsymtest.sh run-ar.sh \
>              run-dwelf_elf_e_machine_string.sh \
>              run-elfclassify.sh run-elfclassify-self.sh \
>              run-disasm-riscv64.sh \
> -            testfile-riscv64-dis1.o.bz2 testfile-riscv64-dis1.expect.bz2
> +            testfile-riscv64-dis1.o.bz2 testfile-riscv64-dis1.expect.bz2 \
> +            testfile-debuginfod-0.rpm.bz2 testfile-debuginfod-1.rpm.bz2 \
> +             testfile-debuginfod-2.rpm.bz2 run-debuginfod-find.sh
> +

OK. Odd indentation though.

>  if USE_VALGRIND
>  valgrind_cmd='valgrind -q --leak-check=full --error-exitcode=1'
> @@ -474,7 +482,7 @@ TESTS_ENVIRONMENT = LC_ALL=C; LANG=C; VALGRIND_CMD=$(valgrind_cmd); \
>                     export LC_ALL; export LANG; export VALGRIND_CMD; \
>                     NM=$(NM); export NM;
>  LOG_COMPILER = $(abs_srcdir)/test-wrapper.sh \
> -              $(abs_top_builddir)/libdw:$(abs_top_builddir)/backends:$(abs_top_builddir)/libelf:$(abs_top_builddir)/libasm
> +              $(abs_top_builddir)/libdw:$(abs_top_builddir)/backends:$(abs_top_builddir)/libelf:$(abs_top_builddir)/libasm:$(abs_top_builddir)/debuginfod

OK.

>  installcheck-local:
>         $(MAKE) $(AM_MAKEFLAGS) \
> @@ -610,6 +618,7 @@ unit_info_LDADD = $(libdw)
>  next_cfi_LDADD = $(libelf) $(libdw)
>  elfcopy_LDADD = $(libelf)
>  addsections_LDADD = $(libelf)
> +debuginfod_build_id_find_LDADD = $(libdw)

You also use libelf functions, they'll be pulled in through libdw, but
I think it is better to be explicit
debuginfod_build_id_find_LDADD = $(libelf) $(libdw)

>  xlate_notes_LDADD = $(libelf)
>  elfrdwrnop_LDADD = $(libelf)
>  dwelf_elf_e_machine_string_LDADD = $(libelf) $(libdw)
> diff --git a/tests/debuginfod_build_id_find.c b/tests/debuginfod_build_id_find.c
> new file mode 100644
> index 000000000000..8e302c8e2116
> --- /dev/null
> +++ b/tests/debuginfod_build_id_find.c
> @@ -0,0 +1,60 @@
> +/* Test program for fetching debuginfo with debuginfo-server.
> +   Copyright (C) 2019 Red Hat, Inc.
> +   This file is part of elfutils.
> +
> +   This file is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   elfutils is distributed in the hope that it will be useful, but
> +   WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +
> +#ifdef HAVE_CONFIG_H
> +# include <config.h>
> +#endif
> +#include <stdio.h>
> +#include ELFUTILS_HEADER(dwfl)
> +#include <elf.h>
> +#include <dwarf.h>
> +#include <argp.h>
> +#include <assert.h>
> +#include <string.h>
> +
> +static const char *debuginfo_path = "";
> +static const Dwfl_Callbacks cb  =
> +  {
> +    NULL,
> +    dwfl_standard_find_debuginfo,
> +    NULL,
> +    (char **)&debuginfo_path,
> +  };

Is this done so that the standard_find_debuginfo doesn't do any
path/file system based lookups?

> +int
> +main (int argc __attribute__ ((unused)), char **argv)
> +{
> +  int expect_pass = strcmp(argv[3], "0");
> +  Dwarf_Addr bias = 0;
> +  Dwfl *dwfl = dwfl_begin(&cb);
> +  dwfl_report_begin(dwfl);
> +
> +  /* Open an executable.  */
> +  Dwfl_Module *mod = dwfl_report_offline(dwfl, argv[2], argv[2], -1);
> +
> +  /* The corresponding debuginfo will not be found in debuginfo_path
> +     (since it's empty), causing the server to be queried.  */

Aha, it is... :) OK.

> +  Dwarf *res = dwfl_module_getdwarf(mod, &bias);
> +  if (expect_pass)
> +    assert(res);
> +  else
> +    assert(!res);
> +
> +  return 0;
> +}
> diff --git a/tests/run-debuginfod-find.sh b/tests/run-debuginfod-find.sh
> new file mode 100755
> index 000000000000..2facef2cbaad
> --- /dev/null
> +++ b/tests/run-debuginfod-find.sh
> @@ -0,0 +1,220 @@
> +# Copyright (C) 2019 Red Hat, Inc.
> +# This file is part of elfutils.
> +#
> +# This file is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# elfutils is distributed in the hope that it will be useful, but
> +# WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +set -x

Is this really necessary?
It makes the log files very verbose.
Or is that the intention?

> +. $srcdir/test-subr.sh  # includes set -e
> +
> +# These are released Fedora 30 i686 main,-debuginfo,-debugsource rpms from koji
> +# https://koji.fedoraproject.org/koji/buildinfo?buildID=1355903
> +testfiles testfile-debuginfod-0.rpm testfile-debuginfod-1.rpm testfile-debuginfod-2.rpm

As mentioned earlier I would really, really, really like it if we could
create them "by hand" through a self-contained .spec file. You can
still check in the, hopefully much smaller, binary rpms. But I feel
somewhat strongly about providing the sources and instructions on how
to recreate them, instead of relying on some remote server.

> +DB=${PWD}/.debuginfod_tmp.sqlite
> +export DEBUGINFOD_CACHE_PATH=${PWD}/.client_cache
> +
> +# clean up trash if we were aborted early
> +trap 'set +e; kill $PID1 $PID2; rm -rf F R ${PWD}/.client_cache*; exit_cleanup; exit 0' 0 1 2 3 5 9 15

So on my machine this causes exit_cleanup to fail because the working
directory isn't fully empty yet. Specifically there seem to
be .debuginfod_tmp.sqlite-shm .debuginfod_tmp.sqlite-wal files left.

You don't have to remove F and R since you already mark them as
tempfiles.

> +# find an unused port number
> +while true; do
> +    PORT1=`expr '(' $RANDOM % 1000 ')' + 9000`
> +    ss -atn | fgrep ":$PORT1" || break
> +done    

Which package does ss come from?
Make sure it is listed as a BuildRequires.
Also I am slightly worried about it not finding anything and looping
forever. Probably unlikely, but ss might misfunction? What happens if
ss isn't installed?

If we assume bash (which I believe we already do in some places) you
could use the bash builtin special /dev/tcp redirection to test whether
a localhost port is open with echo >/dev/tcp/localhost/$PORT

> +# We want to run debuginfod in the background.  We also want to start
> +# it with the same check/installcheck-sensitive LD_LIBRARY_PATH stuff
> +# that the testrun alias sets.  But: we if we just use
> +#    testrun .../debuginfod
> +# it runs in a subshell, with different pid, so not helpful.
> +#
> +# So we gather the LD_LIBRARY_PATH with this cunning trick:
> +ldpath=`testrun sh -c 'echo $LD_LIBRARY_PATH'`
> +
> +mkdir F R
> +tempfiles F R

O, they are directories, then yeah, tempfiles isn't enough.
Maybe don't mark them as tempfiles then.

> +env LD_LIBRARY_PATH=$ldpath DEBUGINFOD_URLS= ${abs_builddir}/../debuginfod/debuginfod -vvvv -d $DB \
> +-p $PORT1 -t0 -g0 R F &
> +PID1=$!
> +sleep 3

Is there no better way to test the server has started?

> +export DEBUGINFOD_URLS=http://localhost:$PORT1/   # or without trailing /
> +
> +# We use -t0 and -g0 here to turn off time-based scanning & grooming.
> +# For testing purposes, we just sic SIGUSR1 / SIGUSR2 at the process.
> +
> +########################################################################
> +
> +# Compile a simple program, strip its debuginfo and save the build-id.
> +# Also move the debuginfo into another directory so that elfutils
> +# cannot find it without debuginfod.
> +echo "int main() { return 0; }" > ${PWD}/prog.c
> +tempfiles prog.c
> +gcc -g -o prog ${PWD}/prog.c
> + ${abs_top_builddir}/src/strip -g -f prog.debug ${PWD}/prog
> +BUILDID=`env LD_LIBRARY_PATH=$ldpath ${abs_builddir}/../src/readelf \
> +          -a prog | grep 'Build ID' | cut -d ' ' -f 7`
> +
> +mv prog F
> +mv prog.debug F
> +kill -USR1 $PID1
> +sleep 3 # give enough time for scanning pass 

Hmm that hardcoded sleep 3 again :{

> +########################################################################
> +
> +# Test whether elfutils, via the debuginfod client library dlopen hooks,
> +# is able to fetch debuginfo from the local debuginfod.
> +testrun ${abs_builddir}/debuginfod_build_id_find -e F/prog 1
> +
> +########################################################################
> +
> +# Test whether debuginfod-find is able to fetch those files.
> +rm -rf $DEBUGINFOD_CACHE_PATH # clean it from previous tests
> +filename=`testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID`
> +cmp $filename F/prog.debug
> +
> +filename=`testrun ${abs_top_builddir}/debuginfod/debuginfod-find executable $BUILDID`
> +cmp $filename F/prog
> +
> +filename=`testrun ${abs_top_builddir}/debuginfod/debuginfod-find source $BUILDID ${PWD}/prog.c`
> +cmp $filename  ${PWD}/prog.c

OK. Cute.

> +########################################################################
> +
> +# Add artifacts to the search paths and test whether debuginfod finds them while already running.
> +
> +# Build another, non-stripped binary
> +echo "int main() { return 0; }" > ${PWD}/prog2.c
> +tempfiles prog2.c
> +gcc -g -o prog2 ${PWD}/prog2.c
> +BUILDID2=`env LD_LIBRARY_PATH=$ldpath ${abs_builddir}/../src/readelf \
> +          -a prog2 | grep 'Build ID' | cut -d ' ' -f 7`
> +
> +mv prog2 F
> +kill -USR1 $PID1
> +sleep 3
> +
> +# Rerun same tests for the prog2 binary
> +filename=`testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID2`
> +cmp $filename F/prog2
> +filename=`testrun ${abs_top_builddir}/debuginfod/debuginfod-find executable $BUILDID2`
> +cmp $filename F/prog2
> +filename=`testrun ${abs_top_builddir}/debuginfod/debuginfod-find source $BUILDID2 ${PWD}/prog2.c`
> +cmp $filename ${PWD}/prog2.c
> +
> +mv testfile-debuginfod-0.rpm R
> +mv testfile-debuginfod-1.rpm R
> +mv testfile-debuginfod-2.rpm R
> +kill -USR1 $PID1
> +sleep 10

Why 10?

> +kill -USR1 $PID1  # two hits of SIGUSR1 may be needed to resolve .debug->dwz->srefs
> +sleep 10

And another :{

> +RPM_BUILDID=5cae7f84186d4ff6c462c32324a764f7a38c3b80                # ./usr/bin/eu-readelf
> +RPM_SOURCE_PATH=/usr/src/debug/elfutils-0.177-1.fc30.i386/src/readelf.c
> +RPM_EXECUTABLE_SHA1SUM=9e4c79dd91a4646d95dfbf091b133e1a21ab2d4c
> +RPM_DEBUGINFO_SHA1SUM=6b638fa2abc5ff0d4d6c438d904092d20cc71827
> +RPM_SOURCE_SHA1SUM=a5bde2a096f6d8f8221456c9380d3532235d7980
> +
> +# Run similar tests against contents of the test RPMs ... except we can't (don't want to)
> +# compare the returned binary to the one in the RPM(s), so we cheat a bit, just use a
> +# sha1sum comparison
> +filename=`testrun ${abs_top_builddir}/debuginfod/debuginfod-find executable $RPM_BUILDID`
> +hash=`cat $filename | sha1sum | awk '{print $1}'`
> +test $hash = $RPM_EXECUTABLE_SHA1SUM
> +
> +filename=`testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $RPM_BUILDID`
> +hash=`cat $filename | sha1sum | awk '{print $1}'`
> +test $hash = $RPM_DEBUGINFO_SHA1SUM
> +
> +filename=`testrun ${abs_top_builddir}/debuginfod/debuginfod-find source $RPM_BUILDID $RPM_SOURCE_PATH`
> +hash=`cat $filename | sha1sum | awk '{print $1}'`
> +test $hash = $RPM_SOURCE_SHA1SUM

OK, because otherwise you would need rpm2cpio and friends to unpack.

> +########################################################################
> +
> +# Drop some of the artifacts, run a groom cycle; confirm that
> +# debuginfod has forgotten them, but remembers others
> +
> +rm R/testfile-*
> +kill -USR2 $PID1  # groom cycle
> +sleep 3
> +rm -rf $DEBUGINFOD_CACHE_PATH # clean it from previous tests
> +
> +testrun ${abs_top_builddir}/debuginfod/debuginfod-find executable $RPM_BUILDID && false || true
> +
> +testrun ${abs_top_builddir}/debuginfod/debuginfod-find executable $BUILDID2

OK

> +########################################################################
> +
> +# Federation mode
> +
> +# find another unused port
> +while true; do
> +    PORT2=`expr '(' $RANDOM % 1000 ')' + 9000`
> +    ss -atn | fgrep ":$PORT2" || break
> +done

Same comment as above.

> +export DEBUGINFOD_CACHE_PATH=${PWD}/.client_cache2
> +mkdir -p $DEBUGINFOD_CACHE_PATH
> +# NB: inherits the DEBUGINFOD_URLS to the first server
> +env LD_LIBRARY_PATH=$ldpath ${abs_builddir}/../debuginfod/debuginfod -d ${DB}_2 -p $PORT2 &
> +PID2=$!
> +sleep 3
> +
> +# have clients contact the new server
> +export DEBUGINFOD_URLS=http://localhost:$PORT2
> +testrun ${abs_builddir}/debuginfod_build_id_find -e F/prog 1
> +
> +# test parallel queries in client
> +export DEBUGINFOD_CACHE_PATH=${PWD}/.client_cache3
> +mkdir -p $DEBUGINFOD_CACHE_PATH
> +export DEBUGINFOD_URLS="BAD http://localhost:$PORT1 localhost:$PORT1 http://localhost:$PORT22 DNE"
> +
> +testrun ${abs_builddir}/debuginfod_build_id_find -e F/prog2 1

OK.

> +########################################################################
> +
> +# Run the tests again without the servers running. The target file should
> +# be found in the cache.
> +
> +kill -INT $PID1 $PID2
> +sleep 5
> +tempfiles .debuginfod_*
> +
> +testrun ${abs_builddir}/debuginfod_build_id_find -e F/prog2 1

OK. Cute.

> +########################################################################
> +
> +# Trigger a cache clean and run the tests again. The clients should be unable to
> +# find the target.
> +echo 0 > $DEBUGINFOD_CACHE_PATH/cache_clean_interval_s
> +echo 0 > $DEBUGINFOD_CACHE_PATH/max_unused_age_s
> +
> +testrun ${abs_builddir}/debuginfod_build_id_find -e F/prog 1
> +
> +testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID2 && false || true

OK. But that means zero means never cache/always clean?
I would have expected 0 to mean "forever".

> +########################################################################
> +
> +# Ensure debuginfod-find can be safely called with no arguments.
> +# Use a relative path to prevent automatic line breaks in the output
> +# due to excessive characters.
> +testrun_compare ../../debuginfod/debuginfod-find <<EOF
> +Usage: ../../debuginfod/debuginfod-find debuginfo BUILDID
> +  or:  ../../debuginfod/debuginfod-find executable BUILDID
> +  or:  ../../debuginfod/debuginfod-find source BUILDID /FILENAME
> +EOF
> +
> +exit 0

OK.