Re: [Bug debuginfod/27982] added DEBUGINFOD_MAXSIZE and DEBUGINFOD_MAXTIME
Hi Noah,
On Thu, 2021-07-29 at 16:29 -0400, Noah Sanci wrote:
> Why have MAXTIME default to LONG_MAX? Which is long, but different
> > on
> > different arches 32/64bit. If MAXSIZE == 0 means infinite, why not make
> > MAXTIME=0 the same for consistency?
>
> Fixed.
>
> > The bug suggests to also check the Content-Length header on reciept (in
> > case the server didn't support or respect the X-DEBUGINFOD-MAXSIZE
> > header. Did you try to implement that?
>
> Fixed.
>
> > I believe various error handling goto out1 should be goto out2 (after
> > your duplicate urls patch). Could you double check that?
>
> Fixed.
>
> > diff --git a/tests/run-debuginfod-find.sh b/tests/run-debuginfod-find.sh
> > index feec5ddf..8ed7743d 100755
> > --- a/tests/run-debuginfod-find.sh
> > +++ b/tests/run-debuginfod-find.sh
> > @@ -187,7 +187,7 @@ tempfiles find-vlog$PORT1
> > # wait for the server to fail the same number of times the query is
> > retried.
> > wait_ready $PORT1
> > 'http_responses_after_you_milliseconds_count{code="406"}' 1
> > # quickly ensure all reporting is functional
> > -grep 'serving file '${PWD}'/F/p+r%o\$g.debug' vlog$PORT1
> > +grep 'serving file '$(realpath ${PWD})'/F/p+r%o\$g.debug' vlog$PORT1
> > grep 'File too large' vlog$PORT1
> > grep 'using max size 1B' find-vlog$PORT1
> > if [ -f ${DEBUGINFOD_CACHE_PATH}/${BUILDID} ]; then
>
> Fixed.
Great. Thanks. Although you don't just have to do things the way I like
them. Feel free to push back and tell me you feel the solution you
chose is better than what I am suggesting. It really is a conversation.
> Find all the fixes in the attached patch.
Looks like you attached the wrong patch (url-duplicate)
Cheers,
Mark
Re: [PATCH] debuginfod-doc: PR27950 - Remove redanduncies in man page.
Hi Frank, On Thu, 2021-07-29 at 10:36 -0400, Frank Ch. Eigler via Elfutils-devel wrote: > Is debuginfod-client-config.7 intended to be installed? If so it should > > be added to notrans_dist_man7_MANS in doc/Makefile.am, [...] > > Yeah. > > > If it isn't intended to be installed, maybe give it a different suffix > > so it isn't confused to be a standalone man page? > > > > > +.so ./debuginfod-client-config.7 > > > > So this is intended to include the fragment into the man page? > > That also doesn't seem to work for me. How/when does it do the > > lookup/inclusion? > > man/nroff executes the .so directive during rendering, i.e., searches > for content in the installed $MANPATH at run time. It is used on > other fedora man pages e.g. for command aliases. aha, ok, I understand now. This makes testing in-tree a bit more difficult. So the .so ./debuginfod-client-config.7 does work if you got into the doc/ subdirectory, then man ./debuginfod-find.1 does show the included chunks. But for the installed tree it should be .so man7 debuginfod-client-config.7 ? > If we look at man1/builtins.1 and man1/bash.1, they show a bit of this > pattern. And actually bash.1 has some conditional inclusion tricks to > let the bash.1 page be includable as well as standalone. That same > trick could be done within the new debuginfod-client-config.7 file. > See the refs to ".ig zZ" and ".zZ", ".nr zZ 1" in the file that > contains the .so directive. That is interesting, then we could make debuginfod-client-config.7 into a real man page and include only the actual contents. I am not completely sure I understand how this works though. I hope there is some man/troff documentation that explains this trick? Thanks, Mark
Re: [PATCH] debuginfod-doc: PR27950 - Remove redanduncies in man page.
Hi - > aha, ok, I understand now. This makes testing in-tree a bit more > difficult. So the .so ./debuginfod-client-config.7 does work if you got > into the doc/ subdirectory, then man ./debuginfod-find.1 does show the > included chunks. But for the installed tree it should be .so man7 > debuginfod-client-config.7 ? It'd probably be ".so ../man7/debuginfod-client-config.7" or something. > > If we look at man1/builtins.1 and man1/bash.1, they show a bit of this > > pattern. And actually bash.1 has some conditional inclusion tricks to > > let the bash.1 page be includable as well as standalone. That same > > trick could be done within the new debuginfod-client-config.7 file. > > See the refs to ".ig zZ" and ".zZ", ".nr zZ 1" in the file that > > contains the .so directive. > > That is interesting, then we could make debuginfod-client-config.7 into > a real man page and include only the actual contents. I am not > completely sure I understand how this works though. I hope there is > some man/troff documentation that explains this trick? Documentation? Nah, let's reverse-engineer: NROFF CPP builtins.1: .nr zZ 1#define zZ 1 .so bash.1 #include bash.1: .if \n(zZ=1 .ig zZ #ifndef zZ [...][...] .zZ #else [...][...] - FChE
Re: [Bug debuginfod/27982] added DEBUGINFOD_MAXSIZE and DEBUGINFOD_MAXTIME
Hello,
Here is the real patch :).
Thanks,
Noah
On Fri, Jul 30, 2021 at 7:11 AM Mark Wielaard wrote:
>
> Hi Noah,
>
> On Thu, 2021-07-29 at 16:29 -0400, Noah Sanci wrote:
> > Why have MAXTIME default to LONG_MAX? Which is long, but different
> > > on
> > > different arches 32/64bit. If MAXSIZE == 0 means infinite, why not make
> > > MAXTIME=0 the same for consistency?
> >
> > Fixed.
> >
> > > The bug suggests to also check the Content-Length header on reciept (in
> > > case the server didn't support or respect the X-DEBUGINFOD-MAXSIZE
> > > header. Did you try to implement that?
> >
> > Fixed.
> >
> > > I believe various error handling goto out1 should be goto out2 (after
> > > your duplicate urls patch). Could you double check that?
> >
> > Fixed.
> >
> > > diff --git a/tests/run-debuginfod-find.sh b/tests/run-debuginfod-find.sh
> > > index feec5ddf..8ed7743d 100755
> > > --- a/tests/run-debuginfod-find.sh
> > > +++ b/tests/run-debuginfod-find.sh
> > > @@ -187,7 +187,7 @@ tempfiles find-vlog$PORT1
> > > # wait for the server to fail the same number of times the query is
> > > retried.
> > > wait_ready $PORT1
> > > 'http_responses_after_you_milliseconds_count{code="406"}' 1
> > > # quickly ensure all reporting is functional
> > > -grep 'serving file '${PWD}'/F/p+r%o\$g.debug' vlog$PORT1
> > > +grep 'serving file '$(realpath ${PWD})'/F/p+r%o\$g.debug' vlog$PORT1
> > > grep 'File too large' vlog$PORT1
> > > grep 'using max size 1B' find-vlog$PORT1
> > > if [ -f ${DEBUGINFOD_CACHE_PATH}/${BUILDID} ]; then
> >
> > Fixed.
>
> Great. Thanks. Although you don't just have to do things the way I like
> them. Feel free to push back and tell me you feel the solution you
> chose is better than what I am suggesting. It really is a conversation.
>
> > Find all the fixes in the attached patch.
>
> Looks like you attached the wrong patch (url-duplicate)
>
> Cheers,
>
> Mark
>
From b7e28a6a1c5e2664cdaeff7e7f5ac39aebfc12d0 Mon Sep 17 00:00:00 2001
From: Noah Sanci
Date: Mon, 26 Jul 2021 13:29:11 -0400
Subject: [PATCH] debuginfod: PR27982 - added DEBUGINFOD_MAXSIZE and
DEBUGINFOD_MAXTIME
DEBUGINFOD_TIMEOUT is a good way to catch servers that are too slow to
*start* transmitting a file. But we have no way of limiting total
download time or space. A user might prefer to have his debugger fetch
only quick & small files, and make do without the bigger ones. Some
transitive dependencies of e.g. gnome programs are huge: 3GB of LLVM
debuginfo, 1GB of webkitgtk, etc. etc.
DEBUGINFOD_MAXSIZE and DEBUGINFOD_MAXTIME were added to dictate the
max download size and time of a debuginfod client. DEBUGINFOD_MAXSIZE
is handled server-side and is sent using the http header:
X-DEBUGINFOD-MAXSIZE. The client side then checks to ensure this maxsize
has been respected.
https://sourceware.org/bugzilla/show_bug.cgi?id=27982
Signed-off-by: Noah Sanci
---
debuginfod/ChangeLog | 12 +
debuginfod/debuginfod-client.c | 99 --
debuginfod/debuginfod.cxx | 13 +
debuginfod/debuginfod.h.in | 2 +
doc/ChangeLog | 6 +++
doc/debuginfod-find.1 | 15 ++
tests/ChangeLog| 7 +++
tests/run-debuginfod-find.sh | 26 +
8 files changed, 177 insertions(+), 3 deletions(-)
diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
index 81eb56f9..9e82d78d 100644
--- a/debuginfod/ChangeLog
+++ b/debuginfod/ChangeLog
@@ -1,3 +1,15 @@
+2021-07-26 Noah Sanci
+
+ PR27982
+ * debuginfod-client.c (globals): added default_maxsize and
+ default_maxtime.
+ (debuginfod_query_server): Added DEBUGINFOD_MAXSIZE and
+ DEBUGINFOD_MAXTIME envvar processing.
+ * debuginfod.cxx (handler_cb): If the requested file exceeds
+ maxsize return code 406.
+ * debuginfod.h.in: Added DEBUGINFOD_MAXSIZE_ENV_VAR and
+ DEBUGINFOD_MAXTIME_ENV_VAR.
+
2021-07-16 Noah Sanci
PR28034
diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index a70f6d79..7d4b220f 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -555,6 +555,39 @@ debuginfod_query_server (debuginfod_client *c,
free (c->url);
c->url = NULL;
+ /* PR 27982: Add max size if DEBUGINFOD_MAXSIZE is set. */
+ long maxsize = 0;
+ const char *maxsize_envvar;
+ maxsize_envvar = getenv(DEBUGINFOD_MAXSIZE_ENV_VAR);
+ if (maxsize_envvar != NULL)
+maxsize = atol (maxsize_envvar);
+
+ /* PR 27982: Add max time if DEBUGINFOD_MAXTIME is set. */
+ long maxtime = 0;
+ const char *maxtime_envvar;
+ maxtime_envvar = getenv(DEBUGINFOD_MAXTIME_ENV_VAR);
+ if (maxtime_envvar != NULL)
+maxtime = atol (maxtime_envvar);
+ if (maxtime && vfd >= 0)
+dprintf(vfd, "using max time %lds\n", maxtime);
+
+ /* Maxsize is valid*/
+ if (maxsize > 0)
+{
+ if (vfd)
+dprintf (vfd, "using max size %ldB\n", maxsize);
+ char *size_header = NULL;
+ rc = asprintf (&size_header, "X-DEBUGINFOD-MAXSIZE: %ld", maxsize);
+ if (r
