Re: [Bug debuginfod/27983] ignore duplicate urls
Hi Noah, On Wed, 2021-07-28 at 12:23 -0400, Noah Sanci via Elfutils-devel wrote: > This patch fixes a memory leak and slightly alters the PR27983 test, > isolating where its DEBUGINFO_URLS's duplicates are accessible, which > fixes a case of test failure on some systems. Great. I pushed my patch for reallocarray support on older systems and this one (with two small whitespace fixes). Thanks, Mark
[Bug general/28136] update Japanese translation
https://sourceware.org/bugzilla/show_bug.cgi?id=28136 Mark Wielaard changed: What|Removed |Added Resolution|--- |FIXED CC||mark at klomp dot org Status|UNCONFIRMED |RESOLVED --- Comment #1 from Mark Wielaard --- Thanks for your signed-off-by on this patch. Applied and pushed as: commit 216996bc7b343e84997af364bbe135f02b14fa59 Author: Hayatsu Shunsuke Date: Sun Jul 25 15:50:30 2021 +0900 update Japanese translation Signed-off-by: Hayatsu Shunsuke -- You are receiving this mail because: You are on the CC list for the bug.
Re: [Bug debuginfod/27982] added DEBUGINFOD_MAXSIZE and DEBUGINFOD_MAXTIME
Hi Noah,
On Mon, 2021-07-26 at 14:56 -0400, Noah Sanci via Elfutils-devel wrote:
> Please find the attached patch for pr 27982. DEBUGINFOD_MAXSIZE and
> MAXTIME were added in this patch to allow users to use more
> constraints when downloading debuginfo.
This looks good. Some high level comments (because I don't actually
know much about http requests).
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?
An alternative of passing around the X-DEBUGINFOD-MAXSIZE header is
doing it all at the client side if we supported HEAD. See PR27277. Did
you consider that route?
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?
I believe various error handling goto out1 should be goto out2 (after
your duplicate urls patch). Could you double check that?
I had to make this little tweak to make the testcase pass on my setup
(because PWD contains symlinks):
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
Cheers,
Mark
Re: [Bug debuginfod/27982] added DEBUGINFOD_MAXSIZE and DEBUGINFOD_MAXTIME
Hi - > [...] An alternative of passing around the X-DEBUGINFOD-MAXSIZE > header is doing it all at the client side if we supported HEAD. See > PR27277. Did you consider that route? [...] That would require -two- http round-trips (one HEAD, one actual GET). This way, the requested limit goes out in the initial GET request. - FChE
Re: [PATCH] debuginfod-doc: PR27950 - Remove redanduncies in man page.
Hi Alice, On Wed, 2021-07-28 at 15:42 -0400, Alice Zhang via Elfutils-devel wrote: > Create a new file, debuginfod-client-config.7, that holds all environment > variables and cache control files related info. Get rid of repetitive > definitions in three other files, instead, those files will include the > content of new file. Any future modification related to environment > variables and cache files will only require changes in one file. > > +2021-07-28 Alice Zhang > + > + PR27950 > + * debuginfod-client-config.7: New file to store all cache config > + infos. > + * debuginfod-find.1: Removed redundant occurrences of environment > + variables & cache control files. > + * debuginfod.8: Likewise. > + * debuginfod_find_debuginfo.3: Likewise. I like the idea, but I am unclear how this works. That might just be my limited knowledge of man/troff. Is debuginfod-client-config.7 intended to be installed? If so it should be added to notrans_dist_man7_MANS in doc/Makefile.am, otherwise it should be added to EXTRA_DIST to indicate it is a file part of the (source) distribution. I assume it should not be installed because it looks like a fragment of a man page. At least it doesn't seem to render correctly when displayed by man for me. 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? Please feel free to point me to the automake/man documentation if that explains it all. Thanks, Mark
Re: [PATCH] debuginfod-doc: PR27950 - Remove redanduncies in man page.
Hi - > 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. 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. - FChE
Re: [Bug debuginfod/27982] added DEBUGINFOD_MAXSIZE and DEBUGINFOD_MAXTIME
Hello,
> 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.
Find all the fixes in the attached patch.
Thanks,
Noah
From b9d36857b6b7cf4f2889280119ba01628afe2420 Mon Sep 17 00:00:00 2001
From: Noah Sanci
Date: Fri, 9 Jul 2021 14:53:10 -0400
Subject: [PATCH] debuginfod: PR27983 - ignore duplicate urls
Gazing at server logs, one sees a minority of clients who appear to have
duplicate query traffic coming in: the same URL, milliseconds apart.
Chances are the user accidentally doubled her $DEBUGINFOD_URLS somehow,
and the client library is dutifully asking the servers TWICE. Bug #27863
reduces the pain on the servers' CPU, but dupe network traffic is still
being paid. We should reject sending outright duplicate concurrent
traffic.
The urls are now simply removed upon finding a duplicate after url
construction.
https://sourceware.org/bugzilla/show_bug.cgi?id=27983
Signed-off-by: Noah Sanci
---
debuginfod/ChangeLog | 9 +++
debuginfod/debuginfod-client.c | 104 +++--
tests/ChangeLog| 6 ++
tests/run-debuginfod-find.sh | 11
4 files changed, 99 insertions(+), 31 deletions(-)
diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
index 7709c24d..81eb56f9 100644
--- a/debuginfod/ChangeLog
+++ b/debuginfod/ChangeLog
@@ -34,6 +34,15 @@
(parse_opt): Handle 'r' option by setting regex_groom to true.
(groom): Introduce and use reg_include and reg_exclude.
+2021-07-09 Noah Sanci
+
+ PR27983
+ * debuginfod-client.c (debuginfod_query_server): As full-length
+ urls are generated with standardized formats, ignore duplicates.
+ Created out1 and changed out2 error gotos. Updated url creation print
+ statements.
+ (globals): Removed url_delim_char, as it was no longer used.
+
2021-06-18 Mark Wielaard
* debuginfod-client.c (debuginfod_begin): Don't use client if
diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index 26ba1891..5afefbfa 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -152,7 +152,6 @@ static const char *cache_xdg_name = "debuginfod_client";
/* URLs of debuginfods, separated by url_delim. */
static const char *url_delim = " ";
-static const char url_delim_char = ' ';
/* Timeout for debuginfods, in seconds (to get at least 100K). */
static const long default_timeout = 90;
@@ -765,13 +764,60 @@ debuginfod_query_server (debuginfod_client *c,
goto out0;
}
+ /* Initialize the memory to zero */
+ char *strtok_saveptr;
+ char **server_url_list = NULL;
+ char *server_url = strtok_r(server_urls, url_delim, &strtok_saveptr);
/* Count number of URLs. */
int num_urls = 0;
- for (int i = 0; server_urls[i] != '\0'; i++)
-if (server_urls[i] != url_delim_char
-&& (i == 0 || server_urls[i - 1] == url_delim_char))
- num_urls++;
-
+
+ while (server_url != NULL)
+{
+ /* PR 27983: If the url is already set to be used use, skip it */
+ char *slashbuildid;
+ if (strlen(server_url) > 1 && server_url[strlen(server_url)-1] == '/')
+slashbuildid = "buildid";
+ else
+slashbuildid = "/buildid";
+
+ char *tmp_url;
+ if (asprintf(&tmp_url, "%s%s", server_url, slashbuildid) == -1)
+{
+ rc = -ENOMEM;
+ goto out1;
+}
+ int url_index;
+ for (url_index = 0; url_index < num_urls; ++url_index)
+{
+ if(strcmp(tmp_url, server_url_list[url_index]) == 0)
+{
+ url_index = -1;
+ break;
+}
+}
+ if (url_index == -1)
+{
+ if (vfd >= 0)
+dprintf(vfd, "duplicate url: %s, skipping\n", tmp_url);
+ free(tmp_url);
+}
+ else
+{
+ num_urls++;
+ char
