Re: [Bug debuginfod/27277] Describe retrieved files when verbose

[Mark Wielaard <mark@klomp.org>]

Hi Noah,

On Fri, Aug 05, 2022 at 03:21:48PM -0400, Noah Sanci wrote:
> On Thu, Aug 4, 2022 at 9:12 AM Mark Wielaard <mark@klomp.org> wrote:
> >
> > Hi Noah,
> >
> > On Thu, 2022-07-14 at 11:32 -0400, Noah Sanci via Elfutils-devel wrote:
> > > Please find the patch for pr28284 attached
> > >
> > >     Debuginfod and debuginfod clients are now equipped to send
> > >     and receive http headers prefixed with X-DEBUGINFOD and
> > >     print them in verbose mode for more context
> >
> > OK, so this patch does 3 things:
> >
> > - Introduce a debuginfod_get_headers public api for libdebuginfod
> >   that provides any X-DEBUGINFOD prefixed headers for the last
> >   retrieved file if the http(s) protocol was used (but not for the file
> >   protocol nor when the file was found in the cache).
> >
> > - It changes debuginfod-find to use this new api to show those headers
> >   in verbose mode.
> >
> > - Changes the debuginfod server to use the api to add any
> >   X-DEBUGINFOD headers to the response if the request was
> >   delegated.
> >
> > So the last two are obvious once we have the first. I am slightly
> > worried about the "inconsistency" in when these headers are available.
> > Should we maybe cache them? Or do they only really make sense during
> > download/in the progress callback?
>
> Not quite sure what you mean here, they're available from the time a server
> is chosen and the file downloaded until when debuginfod_end is called. Are
> you referring to caching outside the lifetime of the debuginfod_client
> or within its lifetime?

I am trying to figure out what the use case is. If the use case is
simply providing information during the download then having/promising
it is just available during the progress callback function might make
things easier.

There are two quirks making the winning headers available after the
download has concluded. If there was an error during download the
headers are erased. Likewise when the handle is reused.

This might be fine, but then has to be documented clearly.

And if there isn't really a usecase then to have the headers available
after the download has succeeded then it can also simply be documented
as only being valid during/from a progress function callback.

> > Looking at the headers the headers are cleaned up when a new request is
> > made using the same client object (not just when the client object is
> > released).
>
> If I understand what's being said, the implication is that there is
> potential for a single client to make multiple requests which would
> unintentionally overwrite previous winning_headers. If the concern
> is overwriting, winning_headers can be expanded into an array of all
> winning headers and a function created to search for headers based
> on header data such as X-DEBUGINFOD-FILE.

It isn't bad when that happens as long it is clear to the user. It
also means that it might be simpler to just guarantee access to the
headers during the progress function callback and not promise anything
about what the call returns after the (possible) download.

> > Also I am not getting any headers because they seem to be turned into
> > lower-case by curl. e.g. from debuginfod.fedoraproject.org:
> >
> > < date: Thu, 04 Aug 2022 12:56:20 GMT
> > < content-type: application/octet-stream
> > < x-debuginfod-size: 4896240
> > < x-debuginfod-archive: /mnt/fedora_koji_prod/koji/packages/bash/5.1.16/2.fc36/x86_64/bash-debuginfo-5.1.16-2.fc36.x86_64.rpm
> > < x-debuginfod-file: /usr/lib/debug/usr/bin/bash-5.1.16-2.fc36.x86_64.debug
> > < last-modified: Wed, 19 Jan 2022 22:15:52 GMT
> >
> > And according to the RFC "Each header field consists of a name
> > followed by a colon (":") and the field value. Field names are
> > case-insensitive." So at least in the code we always need to
> > compare case-insensitive.
>
> Both solutions implemented in the below patch will fix the issue
> assuming that the case is consistent (i.e the prefix isn't
> X-dEbUgInfod),

That is unlikely, but isn't really an invalid header.  I can imagine
someone using X-debuginfod or X-Debuginfod for example. So why not
simply use strncasecmp.

> however,
> for the debuginfod.cxx header cleaning the case of the original
> headers are forced to upper case and not preserved. I hope that is not
> an issue.

That shouldn't be a problem.

> @@ -1111,8 +1118,6 @@ debuginfod_query_server (debuginfod_client *c,
>                  if (c->winning_headers == NULL)
>                    {
>                      c->winning_headers = data[committed_to].response_data;
> -                    if (vfd >= 0 && c->winning_headers != NULL)
> -                      dprintf(vfd, "\n%s", c->winning_headers);
>                      data[committed_to].response_data = NULL;
>                      data[committed_to].response_data_size = 0;
>                    }

Note that previously this provided the full http headers in verbose
mode before/during the actualy download. While now debuginfod-find
--verbose will only provide the X-Debuginfod- headers after the
download.

I can see how only the X-debuginfod headers are useful after the call,
but during the progress callback function maybe the full headers are
useful to determine whether or not to abort the download early?

Cheers,

Mark