Hello,

On Sun, Sep 12, 2021 at 3:08 PM Mark Wielaard <mark@klomp.org> wrote:
> > run-debuginfod-fd-prefetch-caches.sh was updated so that it doesn't
> > trip and fail as previously greping for a value that should yield zero
> > caused an error.
>
> I think this part should be in this patch.
Do you mean should or shouldn't? Removed for now.
> > Previously, target_handle was used to gather CURLE_OK reponses. Under
> > some conditions, target_handle was NULL when we wanted it to point to
> > the handle. This could cause some failuers. instead msg->easy_handle
> > is used, which ensures the correct handle is used.
>
> Thanks for including this explanation. What were the "some conditions"?
I removed this. For some time there was a some failures related to
target_handle being null, but msg->easy_handle being assigned.
My tests are passing like this, however

> I found https://curl.se/libcurl/c/CURLOPT_HEADERFUNCTION.html
> Maybe you could add that as comment for future readers.
Good idea, added.

> If the stuct handle_data had also a size field then most of the above
> recalculations of the size are unnecessary and since we then know the
> (old) end of response_data we can simply memcpy the new data to the
> end (of course we need to make sure to add a zero terminator, but that
> can be done with one byte wrote instead of doing a memset of the whole
> buffer).
Changed.

> >  #if LIBCURL_VERSION_NUM >= 0x072a00 /* 7.42.0 */
> >        curl_easy_setopt(data[i].handle, CURLOPT_PATH_AS_IS, (long) 1);
> >  #else
> > @@ -961,6 +994,7 @@ debuginfod_query_server (debuginfod_client *c,
> >    int committed_to = -1;
> >    bool verbose_reported = false;
> >    struct timespec start_time, cur_time;
> > +  c->winning_headers = NULL;
> >    if ( maxtime > 0 && clock_gettime(CLOCK_MONOTONIC_RAW, &start_time) == -1)
> >      {
> >        rc = errno;
> > @@ -995,8 +1029,18 @@ debuginfod_query_server (debuginfod_client *c,
> >           if (data[i].handle != target_handle)
> >             curl_multi_remove_handle(curlm, data[i].handle);
> >           else
> > -           committed_to = i;
> > -     }
> > +              {
> > +             committed_to = i;
> > +                if (c->winning_headers == NULL)
>
> The indenting here is off because of the mixing of spaces and tabs.
Fixed.


> > diff --git a/tests/ChangeLog b/tests/ChangeLog
> > index 1abe5456..23aeec4a 100644
> > --- a/tests/ChangeLog
> > +++ b/tests/ChangeLog
> > @@ -106,11 +106,12 @@
> >       run-debuginfod-query-retry.sh,
> >       run-debuginfod-regex.sh,
> >       run-debuginfod-sizetime.sh,
> > -     run-debuginfod-tmp-home.sh, and
> > -     run-debuginfod-writable.sh
> > -     run-debuginfod-x-forwarded-for.sh
> > -     * tests/Makefile.am: Added the above new files to the test suite
> > -     * tests/test-subr.sh: Added some general functions for above tests
> > +     run-debuginfod-tmp-home.sh,
> > +     run-debuginfod-writable.sh, and
> > +     run-debuginfod-x-forwarded-for.sh.
> > +     All of the above functions now use a 'base' variable to avoid races
> > +     * Makefile.am: Added the above new files to the test suite
> > +     * test-subr.sh: Added some general functions for above tests
>
> These changes seem unrelated to this patch.
Restored their states.

> > +2021-08-02  Noah Sanci  <nsanci@redhat.com>
> > +
> > +     PR27277
> > +     * run-debuginfod-response-headers.sh: Add a test to ensure that file descriptions
> > +     are accurate for files outside or within archives.
> > +     * Makefile.am: Add run-debuginfod-response-headers.sh to TESTS.
>
> It also needs to be added to EXTRA_DIST.
Added.

> > diff --git a/tests/run-debuginfod-fd-prefetch-caches.sh b/tests/run-debuginfod-fd-prefetch-caches.sh
> > index 61fee9e9..bee88c1e 100755
> > --- a/tests/run-debuginfod-fd-prefetch-caches.sh
> > +++ b/tests/run-debuginfod-fd-prefetch-caches.sh
> > @@ -51,9 +51,9 @@ grep 'prefetch fds ' vlog$PORT1 #$PREFETCH_FDS
> >  grep 'prefetch mbs ' vlog$PORT1 #$PREFETCH_MBS
> >  # search the vlog to find what metric counts should be and check the correct metrics
> >  # were incrimented
> > -wait_ready $PORT1 'fdcache_op_count{op="enqueue"}' $( grep -c 'interned.*front=1' vlog$PORT1 )
> > -wait_ready $PORT1 'fdcache_op_count{op="evict"}' $( grep -c 'evicted a=.*' vlog$PORT1 )
> > -wait_ready $PORT1 'fdcache_op_count{op="prefetch_enqueue"}' $( grep -c 'interned.*front=0' vlog$PORT1 )
> > +wait_ready $PORT1 'fdcache_op_count{op="enqueue"}' $( grep -c 'interned.*front=1' vlog$PORT1 || true)
> > +wait_ready $PORT1 'fdcache_op_count{op="evict"}' $( grep -c 'evicted a=.*' vlog$PORT1 || true )
> > +wait_ready $PORT1 'fdcache_op_count{op="prefetch_enqueue"}' $( grep -c 'interned.*front=0' vlog$PORT1 || true )
> >  wait_ready $PORT1 'fdcache_op_count{op="prefetch_evict"}' $( grep -c 'evicted from prefetch a=.*front=0' vlog$PORT1 || true )
> >
> >  kill $PID1
>
> This is an unrelated change.
Restored.

> > diff --git a/tests/run-debuginfod-response-headers.sh b/tests/run-debuginfod-response-headers.sh
> > new file mode 100755
> > index 00000000..a458ca1b
> > --- /dev/null
> > +++ b/tests/run-debuginfod-response-headers.sh
> > @@ -0,0 +1,118 @@
> > +#!/usr/bin/env bash
> > +#
> > +# Copyright (C) 2019-2021 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/>.
[...]
> > +# Wait till both files are in the index and scan/index fully finished
> > +wait_ready $PORT1 'thread_work_total{role="traverse"}' 2
> > +wait_ready $PORT1 'thread_work_pending{role="scan"}' 0
> > +wait_ready $PORT1 'thread_busy{role="scan"}' 0
> > +# All rpms need to be in the index, except the dummy permission-000 one
> > +rpms=$(find R -name \*rpm | grep -v nothing | wc -l)
> > +wait_ready $PORT1 'scanned_files_total{source=".rpm archive"}' $rpms
> > +kill -USR1 $PID1  # two hits of SIGUSR1 may be needed to resolve .debug->dwz->srefs
> > +# Wait till both files are in the index and scan/index fully finished
> > +wait_ready $PORT1 'thread_work_total{role="traverse"}' 3
> > +wait_ready $PORT1 'thread_work_pending{role="scan"}' 0
> > +wait_ready $PORT1 'thread_busy{role="scan"}' 0
>
> Is it really necessary to add all this if this is just a test to check
> the new headers are sent?
A lot of the setup is to check that both the archive and regular file
headers are added. In the attached
path I removed as much as I felt reasonable. Please get back to me on
if it is enough.

> > diff --git a/tests/test-subr.sh b/tests/test-subr.sh
> > index 41e95e31..95dcbb26 100644
> > --- a/tests/test-subr.sh
> > +++ b/tests/test-subr.sh
> > @@ -43,9 +43,9 @@ remove_files=
> >  exit_cleanup()
> >  {
> >    rm -rf ${PWD}/.client_cache*
> > -  rm -f $remove_files;
> > +  rm -f $remove_files;
> >    ls -a ${PWD}
> > -  cd ..;
> > +  cd ..;
> >    rmdir $test_dir
> >  }
[...]
> >    while true; do
> > -    PORT2=`expr '(' $RANDOM % 1000 ')' + 9000`
> > +    PORT2=`expr '(' $RANDOM % 100 ')' + $base`
> >      ss -atn | fgrep ":$PORT2" && $PORT1 -ne $PORT2 || break
> >    done
>
> These changes also seem unrelated.
Fixed.

Thanks,

Noah Sanci