* [patch git] PR28284 - debuginfod x-debuginfod* header processing
@ 2022-09-03 0:13 Frank Ch. Eigler
2022-09-06 15:49 ` Mark Wielaard
0 siblings, 1 reply; 5+ messages in thread
From: Frank Ch. Eigler @ 2022-09-03 0:13 UTC (permalink / raw)
To: elfutils-devel
Hi -
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.
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. debuginfod-find prints those
in -v verbose mode, and debuginfod relays them in federation.
This stuff is an enabler for rgoldber's subsequent
signature-passing/checking code, to which I plan to turn my attention
next.
Please see users/fche/try-pr28284d for this draft of the code. I'd
like to keep it as two separate commits to preserve Noah's id in the
git history, even though that makes it a bit harder to give a final
review.
git diff --stat origin/master...origin/users/fche/try-pr28284d
debuginfod/ChangeLog | 22 ++++++++++++++++++++++
debuginfod/debuginfod-client.c | 24 ++++++++++++++++++------
debuginfod/debuginfod-find.c | 3 +++
debuginfod/debuginfod.cxx | 22 ++++++++++++++++++++++
debuginfod/debuginfod.h.in | 4 ++++
debuginfod/libdebuginfod.map | 3 +++
doc/ChangeLog | 10 ++++++++++
doc/debuginfod_find_debuginfo.3 | 14 ++++++++++++++
doc/debuginfod_get_headers.3 | 2 ++
tests/ChangeLog | 12 ++++++++++++
tests/run-debuginfod-response-headers.sh | 53 +++++++++++++++++++++++++++++++++++++++++++++--------
11 files changed, 155 insertions(+), 14 deletions(-)
- FChE
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [patch git] PR28284 - debuginfod x-debuginfod* header processing
2022-09-03 0:13 [patch git] PR28284 - debuginfod x-debuginfod* header processing Frank Ch. Eigler
@ 2022-09-06 15:49 ` Mark Wielaard
2022-09-06 16:05 ` Frank Ch. Eigler
0 siblings, 1 reply; 5+ messages in thread
From: Mark Wielaard @ 2022-09-06 15:49 UTC (permalink / raw)
To: Frank Ch. Eigler, elfutils-devel
Hi Frank,
On Fri, 2022-09-02 at 20:13 -0400, Frank Ch. Eigler via Elfutils-devel
wrote:
> 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?
> 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).
> 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?
> This stuff is an enabler for rgoldber's subsequent
> signature-passing/checking code, to which I plan to turn my attention
> next.
>
> Please see users/fche/try-pr28284d for this draft of the code. I'd
> like to keep it as two separate commits to preserve Noah's id in the
> git history, even though that makes it a bit harder to give a final
> review.
Thanks. I like this version except for those two nitpicks above. What
do you think?
Cheers,
Mark
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [patch git] PR28284 - debuginfod x-debuginfod* header processing
2022-09-06 15:49 ` Mark Wielaard
@ 2022-09-06 16:05 ` Frank Ch. Eigler
2022-09-08 11:42 ` Mark Wielaard
0 siblings, 1 reply; 5+ messages in thread
From: Frank Ch. Eigler @ 2022-09-06 16:05 UTC (permalink / raw)
To: Mark Wielaard; +Cc: elfutils-devel
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [patch git] PR28284 - debuginfod x-debuginfod* header processing
2022-09-06 16:05 ` Frank Ch. Eigler
@ 2022-09-08 11:42 ` Mark Wielaard
2022-09-08 13:45 ` Frank Ch. Eigler
0 siblings, 1 reply; 5+ messages in thread
From: Mark Wielaard @ 2022-09-08 11:42 UTC (permalink / raw)
To: Frank Ch. Eigler; +Cc: elfutils-devel
Hi Frank,
On Tue, 2022-09-06 at 12:05 -0400, Frank Ch. Eigler via Elfutils-devel
wrote:
> > 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.
Not just a new transfer, but also when we hit the cache before doing a
new transfer. Currently when we hit the cache and don't do any transfer
the winning_headers will point to the last http transfer which will
have nothing to do with the returned (cached) result. Just like we
clear client->url early.
Thanks,
Mark
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [patch git] PR28284 - debuginfod x-debuginfod* header processing
2022-09-08 11:42 ` Mark Wielaard
@ 2022-09-08 13:45 ` Frank Ch. Eigler
0 siblings, 0 replies; 5+ messages in thread
From: Frank Ch. Eigler @ 2022-09-08 13:45 UTC (permalink / raw)
To: Mark Wielaard; +Cc: elfutils-devel
Hi -
> > Good point, we don't want an aborted new transfer to retain records
> > from a previous run, will fix that.
>
> Not just a new transfer, but also when we hit the cache before doing a
> new transfer. Currently when we hit the cache and don't do any transfer
> the winning_headers will point to the last http transfer which will
> have nothing to do with the returned (cached) result. Just like we
> clear client->url early.
Got it, pushing this:
diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index 272a6a7a007f..5e5c140ab205 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -588,9 +588,11 @@ debuginfod_query_server (debuginfod_client *c,
goto out;
}
- /* Clear the obsolete URL from a previous _find operation. */
+ /* Clear the obsolete data from a previous _find operation. */
free (c->url);
c->url = NULL;
+ free (c->winning_headers);
+ c->winning_headers = NULL;
/* PR 27982: Add max size if DEBUGINFOD_MAXSIZE is set. */
long maxsize = 0;
- FChE
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-09-08 13:45 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-03 0:13 [patch git] PR28284 - debuginfod x-debuginfod* header processing Frank Ch. Eigler
2022-09-06 15:49 ` Mark Wielaard
2022-09-06 16:05 ` Frank Ch. Eigler
2022-09-08 11:42 ` Mark Wielaard
2022-09-08 13:45 ` Frank Ch. Eigler
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).