Hi -

Thanks for giving it a look.

> > I had a bit of time to tweak Noah Sanci's PR28284 work, and I believe
> > it addresses Mark's last set of comments (from a while ago).  This
> > follow-up patch corrects things like case sensitivity, spacing, \r\n
> > processing, and tweaks documentation.
> 
> I hadn't thought about the \r\n at the end of the HTTP headers. Thanks.
> I assume \r\n at the end of HTTP headers required, since you are
> expecting in the code now, or could it also be \n on its own?

At the wire protocol level, yes, required.  But this code strips them
from the normal C/userspace processing (and libcurl adds them back as
needed).

> > The gist of it is to add a new client api function
> > debuginfod_get_headers(), documented to return x-debuginfod* headers
> > from current or previous fetch requests.
> 
> This looks good, but I think c->winning_headers needs to be
> freed/cleared at the start of debuginfod_query_server. Otherwise if you
> reuse the debuginfod_client and you hit the cache, the user gets the
> headers from the last use of debuginfod_client that did fetch something
> from a server. Which imho is confusing (the headers won't match the
> cached result returned).

Good point, we don't want an aborted new transfer to retain records from
a previous run, will fix that.

> >   debuginfod-find prints those
> > in -v verbose mode, and debuginfod relays them in federation.
> 
> This is the only thing I am not 100% happy about. It means you can only
> see the headers using debuginfod-find but no longer with any other
> client when DEBUGINFOD_VERBOSE is set. Is this really what we want?

Well, dunno, can't speak for we all. :-) For debugging purposes (which
is what DEBUGINFOD_VERBOSE is), we could print all headers that come
in, from any server we're connecting to.  For API / human-friendly
use, limiting to x-debuginfod* ones from the winning server seems more
useful.  (I hope we don't have to rethink again once the signature
contents start coming down that pipe too - hope they're not too long.)

Will add a commit to that effect shortly and run it through the
trybots.

- FChE

Reply via email to