Re: patch 2/2 debuginfod server etc.

["Frank Ch. Eigler" <fche@redhat.com>]

Hi -

> 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.

Well, one did not have to fetch them from anywhere, they were already
there.  But yes, working on a smaller synthetic set of RPMs.

> > +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)

OK.


> > +set -x
> 
> Is this really necessary?
> It makes the log files very verbose.
> Or is that the intention?

I've found it tricky to debug failing tests without this.


> > +# 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?

iproute-5.2.0-1.fc30.x86_64

> Make sure it is listed as a BuildRequires.

OK, because of %check in the RPM?

> Also I am slightly worried about it not finding anything and looping
> forever. Probably unlikely, but ss might misfunction? 

There is no 100%-probable way of assigning a port number ahead of
time.  Would you like an iteration limit / abort for that unlikely
situation?

> What happens if ss isn't installed?

The set -e should nuke the test run.


> 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

OK, will try that.


> > +mkdir F R
> > +tempfiles F R
> 
> O, they are directories, then yeah, tempfiles isn't enough.
> Maybe don't mark them as tempfiles then.

OK.

> > +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?

This one may not be needed.


> > +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 :{

Well, here's the problem.  We want to assert that the scan was
successful.  We can't just reject a 404 response because the scan may
not have finished yet.  So we'd have to race/loop/poll.  But then we'd
need a timeout (how long?).  It turns out to be the same thing, just
more complicated.


> > +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?

To give extra time for scanning RPMs.

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

Yes, again, same reasons as above.  You can either have a timeout-poll
loop, or a time sleep and a single judgement poll.


> > +# 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".

I see the man page doesn't specifically disclose the interpretation of
zero.  A "no retention of prior results" purpose is useful, and is
consistent with 0 as per the text.  A "retain forever" setting would
have to be a different value.

- FChE