Worker threads and MHD_USE_EPOLL
The debuginfod man page states that if the server is run without the --connection-pool option, a new thread will be cloned for every request. However this is not always the case. Since commit e646e363e debuginfod attempts to configure the libmicrohttpd daemon to use epoll for the daemon's internal event loop. libmicrohttpd requires that if epoll support is enabled, it will not create a new thread to handle each request. So when the --connection-pool option is not given, the daemon ends up configured to use a just a single worker thread that can only respond to one request at a time. There are a few ways we can address this. My preferred approach is to use MHD_USE_THREAD_PER_CONNECTION instead of MHD_USE_EPOLL when --connection-pool is not given. This preserves the behaviour stated in the man page and if users want the performance benefits of epoll they can always use --connection-pool to enable it. If we insist on always using epoll when it's available then we might want to change the default number of worker threads to something other than 1, just like when --connection-pool is given with no argument. We could simply modify the man page to state that if --connection-pool isn't given then only 1 worker thread will be used. However I don't like this approach because it unnecessarily constrains the server's default behavior and ideally we should try to preserve the behavior that we advertise in the man page. WDYT? Aaron
[PATCH] debuginfod: Use auto-sized connection pool when -C is not given with arg
Since commit 4b42d9ad, libmicrohttpd's epoll event loop is used when available in which case we must disable its setting for spawning a thread per request. This contradicts the debuginfod doc's description of '-C', which indicates that if this command line option is not given then the thread pool size is unbounded. Fix this by using an auto-sized thread pool when '-C' is not given, just as we do when it's given with no argument. Update the doc's description of '-C'. Also use a fixed-size pool even if epoll is not supported. The unbounded pool config cannot be considered entirely reliable as it appears to cause random fails in the run-debuginfod-webapi-concurrency test. Signed-off-by: Aaron Merey --- debuginfod/ChangeLog | 7 +++ debuginfod/debuginfod.cxx | 22 ++ doc/ChangeLog | 4 doc/debuginfod.8 | 10 -- 4 files changed, 25 insertions(+), 18 deletions(-) diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog index da58e9a8..c692a389 100644 --- a/debuginfod/ChangeLog +++ b/debuginfod/ChangeLog @@ -1,3 +1,10 @@ +2022-09-02 Aaron Merey + + * debuginfod.cxx (parse_opt): If '-C' is given with no arg, do not + update connection_pool since it will be done at a later point. + (main): Use auto-sized connection_pool if '-C' isn't given with an + arg. Do not use MHD_USE_THREAD_PER_CONNECTION. + 2022-08-17 Martin Liska * debuginfod.cxx (handle_buildid): Update HTTP statistics only if diff --git a/debuginfod/debuginfod.cxx b/debuginfod/debuginfod.cxx index 3e2dd9ef..8680c048 100644 --- a/debuginfod/debuginfod.cxx +++ b/debuginfod/debuginfod.cxx @@ -575,8 +575,6 @@ parse_opt (int key, char *arg, if (connection_pool < 2) argp_failure(state, 1, EINVAL, "-C NUM minimum 2"); } - else // arg not given -connection_pool = std::thread::hardware_concurrency() * 2 ?: 2; break; case 'I': // NB: no problem with unconditional free here - an earlier failed regcomp would exit program @@ -3937,6 +3935,11 @@ main (int argc, char *argv[]) } } + /* If '-C' wasn't given or was given with no arg, pick a reasonable default + for the number of worker threads. */ + if (connection_pool == 0) +connection_pool = std::thread::hardware_concurrency() * 2 ?: 2; + /* Note that MHD_USE_EPOLL and MHD_USE_THREAD_PER_CONNECTION don't work together. */ unsigned int use_epoll = 0; @@ -3944,12 +3947,11 @@ main (int argc, char *argv[]) use_epoll = MHD_USE_EPOLL; #endif - unsigned int mhd_flags = ((connection_pool || use_epoll -? 0 : MHD_USE_THREAD_PER_CONNECTION) + unsigned int mhd_flags = ( #if MHD_VERSION >= 0x00095300 - | MHD_USE_INTERNAL_POLLING_THREAD + MHD_USE_INTERNAL_POLLING_THREAD #else - | MHD_USE_SELECT_INTERNALLY + MHD_USE_SELECT_INTERNALLY #endif | MHD_USE_DUAL_STACK | use_epoll @@ -3964,12 +3966,8 @@ main (int argc, char *argv[]) handler_cb, NULL, /* handler callback */ MHD_OPTION_EXTERNAL_LOGGER, error_cb, NULL, - (connection_pool - ? MHD_OPTION_THREAD_POOL_SIZE - : MHD_OPTION_END), - (connection_pool - ? (int)connection_pool - : MHD_OPTION_END), + MHD_OPTION_THREAD_POOL_SIZE, + (int)connection_pool, MHD_OPTION_END); MHD_Daemon *d4 = NULL; diff --git a/doc/ChangeLog b/doc/ChangeLog index ceec2467..c446e99e 100644 --- a/doc/ChangeLog +++ b/doc/ChangeLog @@ -1,3 +1,7 @@ +2022-09-02 Aaron Merey + + * debuginfod.8 (-C): Update description. + 2022-06-03 Michael Trapp * debuginfod.8 (--disable-source-scan): Document. diff --git a/doc/debuginfod.8 b/doc/debuginfod.8 index 50fce7f5..7c1dc3dd 100644 --- a/doc/debuginfod.8 +++ b/doc/debuginfod.8 @@ -212,15 +212,13 @@ following table summarizes the interpretaton of this option and its optional NUM parameter. .TS l l. -no option clone new thread for every request, no fixed pool -\-Cuse a fixed thread pool sized automatically +no option, \-C use a fixed thread pool sized automatically \-C=NUMuse a fixed thread pool sized NUM, minimum 2 .TE -The first mode is useful for friendly bursty traffic. The second mode -is a simple and safe configuration based on the number of processors. -The third mode is suitable for tuned load-limiting configurations -facing unruly traffic. +The first mode is a simple and safe
[PATCH] debuginfod: Support queries for ELF/DWARF sections.
This patch adds a new function debuginfod_find_section which queries
debuginfod servers for the binary contents of the specified ELF/DWARF
section in a file matching the given build-id.
In order to distinguish between debuginfo and executable files with the
same build-id, this function includes a bool parameter use_debuginfo.
If true, attempt to retrieve the section from the debuginfo file with
the given build-id. If false, use the executable instead.
If the server can find the section in the parent file, the section's
binary contents are written to a temporary file. This file is interned
by the server's fdcache as if it was extracted from an archive.
Modify the webapi to include "debuginfo-section" and
"executable-section" artifacttypes. This helps prevent debuginfod
servers built without _find_section() support from responding with the
entire debuginfo/executable when a section was requested. The section
name is included in the url like source paths are for _find_source()
queries.
One of the primary use cases for this feature is so that tools can
aquire indecies such as .gdb_index without having to download entire
files. Although this patch does not implement it, we could generate
.gdb_index on-the-fly if the target file does not contain it. However
for large debuginfo files generating the index can take a non-trivial
amount of time (ex. about 25 seconds for a 2.5GB debuginfo file).
---
ChangeLog | 4 +
NEWS| 2 +
debuginfod/ChangeLog| 19 ++
debuginfod/debuginfod-client.c | 41 +++-
debuginfod/debuginfod-find.c| 39 +++-
debuginfod/debuginfod.cxx | 388 ++--
debuginfod/debuginfod.h.in | 9 +
debuginfod/libdebuginfod.map| 1 +
doc/ChangeLog | 6 +
doc/Makefile.am | 1 +
doc/debuginfod_find_debuginfo.3 | 30 ++-
doc/debuginfod_find_section.3 | 1 +
tests/ChangeLog | 5 +
tests/Makefile.am | 4 +-
tests/run-debuginfod-section.sh | 124 ++
15 files changed, 590 insertions(+), 84 deletions(-)
create mode 100644 doc/debuginfod_find_section.3
create mode 100755 tests/run-debuginfod-section.sh
diff --git a/ChangeLog b/ChangeLog
index 1f449d60..a1e37f88 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,7 @@
+2022-09-27 Aaron Merey
+
+ * NEWS: Add debuginfod_find_section.
+
2022-09-27 Taketo Kabe
* debuginfod/debuginfod-client.c: Correctly get timestamp when
diff --git a/NEWS b/NEWS
index 156f78df..974ce920 100644
--- a/NEWS
+++ b/NEWS
@@ -4,6 +4,8 @@ readelf: Add -D, --use-dynamic option.
debuginfod: Add --disable-source-scan option.
+debuginfod-client: Add new function debuginfod_find_section.
+
libdwfl: Add new function dwfl_get_debuginfod_client.
Add new function dwfl_frame_reg.
diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
index 680720ff..49bdf250 100644
--- a/debuginfod/ChangeLog
+++ b/debuginfod/ChangeLog
@@ -1,3 +1,22 @@
+2022-09-27 Aaron Merey
+
+ * debuginfod-client.c (debuginfod_find_section): New function.
+ (debuginfod_query_server): Add support for section queries.
+ * debuginfod-find.c (main): Add support for section queries.
+ * debuginfod.cxx (extract_section_to_fd): New function.
+ (handle_buildid_f_match): Add section parameter. When non-empty,
+ try to create response from section contents.
+ (handle_buildid_r_match): Add section parameter. When non-empty,
+ try to create response from section contents.
+ (handle_buildid_match): Add section parameter. Pass to
+ handle_buildid_{f,r}_match.
+ (handle_buildid): Handle section name when artifacttype is set to
+ "debuginfo-section" or "executable-section". Query upstream servers
+ via debuginfod_find_section when necessary.
+ (debuginfod.h.in): Include stdbool.h. Add declaration for
+ debuginfod_find_section.
+ (libdebuginfod.map): Add debuginfod_find_section.
+
2022-09-08 Frank Ch. Eigler
* debuginfod-client.c (debuginfod_query_server): Clear
diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index 28ad04c0..9795f6b1 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -55,6 +55,9 @@ int debuginfod_find_executable (debuginfod_client *c, const
unsigned char *b,
int s, char **p) { return -ENOSYS; }
int debuginfod_find_source (debuginfod_client *c, const unsigned char *b,
int s, const char *f, char **p) { return -ENOSYS;
}
+int debuginfod_find_section (debuginfod_client *c, const unsigned char *b,
+int s, const char *scn, bool u,
+char **p) { return -ENOSYS; }
void debuginfod_set_progressfn(debuginfod_client *c,
debuginfod_progressfn_t fn) { }
void debuginfod_se
[OB PATCH] debuginfod-client: Ensure only negative error codes returned.
Committing as obvious:
Switch a couple error codes from positive to negative so they aren't
interpreted as file descriptors by the caller.
Signed-off-by: Aaron Merey
---
debuginfod/ChangeLog | 5 +
debuginfod/debuginfod-client.c | 4 ++--
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
index 680720ff..8fb65133 100644
--- a/debuginfod/ChangeLog
+++ b/debuginfod/ChangeLog
@@ -1,3 +1,8 @@
+2022-09-28 Aaron Merey
+
+ * debuginfod-client.c (debuginfod_query_server): Switch sign of some
+ error codes from positive to negative.
+
2022-09-08 Frank Ch. Eigler
* debuginfod-client.c (debuginfod_query_server): Clear
diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index 28ad04c0..2a14d9d9 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -1085,7 +1085,7 @@ debuginfod_query_server (debuginfod_client *c,
c->winning_headers = NULL;
if ( maxtime > 0 && clock_gettime(CLOCK_MONOTONIC_RAW, &start_time) == -1)
{
- rc = errno;
+ rc = -errno;
goto out2;
}
long delta = 0;
@@ -1096,7 +1096,7 @@ debuginfod_query_server (debuginfod_client *c,
{
if (clock_gettime(CLOCK_MONOTONIC_RAW, &cur_time) == -1)
{
- rc = errno;
+ rc = -errno;
goto out2;
}
delta = cur_time.tv_sec - start_time.tv_sec;
--
2.37.3
Re: [PATCH] debuginfod: Support queries for ELF/DWARF sections.
Hi Frank, On Wed, Sep 28, 2022 at 10:28 AM Frank Ch. Eigler wrote: > On Tue, Sep 27, 2022 at 10:10:52PM -0400, Aaron Merey via Elfutils-devel > wrote: > > > [...] In order to distinguish between debuginfo and executable > > files with the same build-id, this function includes a bool > > parameter use_debuginfo. If true, attempt to retrieve the section > > from the debuginfo file with the given build-id. If false, use the > > executable instead. [...] > > How would a client know which one to use? Does it provide power or > benefit to force them to choose (or iterate?). Is there a scenario > where the content could be different between the two (if both > existed)? > > If that decisionmaking is not warranted to put upon the shoulders of > the client, the server could just be asked for a section name "as if > from an unstripped executable", and let it find that in the executable > or debuginfo, whereever. Good point, the server/client should figure this out internally. On IRC we also discussed the possible usefulness of client-side emulation of section queries in case a server isn't built with _find_section support. Will update the patch to include these details. > > [...] Although this patch does not implement it, we could generate > > .gdb_index on-the-fly if the target file does not contain it. > > However for large debuginfo files generating the index can take a > > non-trivial amount of time (ex. about 25 seconds for a 2.5GB > > debuginfo file). [...] > > Even that is not too bad, considering that the alternative would be > having to download that 2.5GB file. I recall you saying that on some > distros, gdb-index sections are always there anyway, so we wouldn't > have to rush to implement this feature. I did a quick experiment checking the debuginfo for the libraries used by gdb, firefox and qemu-kvm on F36. Out of the 265 files I checked only 1 (libicudata.so.69.1 debuginfo) didn't contain a .gdb_index because it strangely does not contain any .debug_* sections at all. Aaron
[PATCH v2] debuginfod: Support queries for ELF/DWARF sections
v1 can be found here:
https://sourceware.org/pipermail/elfutils-devel/2022q3/005402.html
v2 simplifies debuginfod_find_section by automatically checking both
debuginfo and executables to extract the target section from. Previously
this function included a parameter for specifying whether the section should
come from a debuginfo or executable file. For sections that are normally
stored in debuginfo or for sections that might differ between debuginfo and
executable (ex. .shstrtab, .gnu.build.attributes), debuginfo files are
searched before executables.
Artifacttype "debuginfo-section" and "executable-section" from v1 have
been replaced with artifacttype "section". An example URL for a
section query is: /buildid/0123456789abcdef/section/.gdb_index
Section files in the client cache are named "section" followed by
the section name. Ex: "section.gdb_index".
debuginfod_query_server now attempts to extract the desired section
from a debuginfo or executable in the client cache before querying
any servers.
If debuginfod servers are built without _find_section support, they will
respond to a section query with response code 503. If the client recieves
this code in response to a section query, it will attempt to query servers
for the full debuginfo or the executable (possibly both) in order to extract
the section on the client side. For sections that are normally stored in
debuginfo or for sections that might differ between debuginfo and executable,
debuginfo files are downloaded/searched before executables.
---
ChangeLog | 4 +
NEWS| 2 +
debuginfod/ChangeLog| 22 ++
debuginfod/Makefile.am | 2 +-
debuginfod/debuginfod-client.c | 273 +++-
debuginfod/debuginfod-find.c| 21 +-
debuginfod/debuginfod.cxx | 433 +++-
debuginfod/debuginfod.h.in | 6 +
debuginfod/libdebuginfod.map| 1 +
doc/ChangeLog | 6 +
doc/Makefile.am | 1 +
doc/debuginfod_find_debuginfo.3 | 26 +-
doc/debuginfod_find_section.3 | 1 +
tests/ChangeLog | 5 +
tests/Makefile.am | 4 +-
tests/run-debuginfod-section.sh | 141 +++
16 files changed, 865 insertions(+), 83 deletions(-)
create mode 100644 doc/debuginfod_find_section.3
create mode 100755 tests/run-debuginfod-section.sh
diff --git a/ChangeLog b/ChangeLog
index 60624183..29b2108e 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,7 @@
+2022-10-20 Aaron Merey
+
+ * NEWS: Add debuginfod_find_section.
+
2022-09-13 Aleksei Vetrov
* NEWS (libdwfl): Add dwfl_report_offline_memory.
diff --git a/NEWS b/NEWS
index 6ebd172c..3e290ff7 100644
--- a/NEWS
+++ b/NEWS
@@ -4,6 +4,8 @@ readelf: Add -D, --use-dynamic option.
debuginfod: Add --disable-source-scan option.
+debuginfod-client: Add new function debuginfod_find_section.
+
libdwfl: Add new function dwfl_get_debuginfod_client.
Add new function dwfl_frame_reg.
Add new function dwfl_report_offline_memory.
diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
index 59d50df1..aacf655b 100644
--- a/debuginfod/ChangeLog
+++ b/debuginfod/ChangeLog
@@ -1,3 +1,25 @@
+2022-10-20 Aaron Merey
+
+ * Makefile.am (libdebuginfod_so_LDLIBS): Add libelf.
+ * debuginfod-client.c (debuginfod_find_section): New function.
+ (extract_section): New function.
+ (maybe_debuginfo_section): New function.
+ (cache_find_section): New function.
+ (debuginfod_query_server): Add support for section queries.
+ * debuginfod-find.c (main): Add support for section queries.
+ * debuginfod.cxx (extract_section): New function.
+ (handle_buildid_f_match): Add section parameter. When non-empty,
+ try to create response from section contents.
+ (handle_buildid_r_match): Add section parameter. When non-empty,
+ try to create response from section contents.
+ (handle_buildid_match): Add section parameter. Pass to
+ handle_buildid_{f,r}_match.
+ (handle_buildid): Handle section name when artifacttype is set to
+ "section". Query upstream servers via debuginfod_find_section
+ when necessary.
+ (debuginfod.h.in): Add declaration for debuginfod_find_section.
+ (libdebuginfod.map): Add debuginfod_find_section.
+
2022-10-17 Frank Ch. Eigler
* debuginfod.cxx (main): Report libmicrohttpd version.
diff --git a/debuginfod/Makefile.am b/debuginfod/Makefile.am
index 435cb8a6..f27d6e2e 100644
--- a/debuginfod/Makefile.am
+++ b/debuginfod/Makefile.am
@@ -97,7 +97,7 @@ libdebuginfod_so_LIBS = libdebuginfod_pic.a
if DUMMY_LIBDEBUGINFOD
libdebuginfod_so_LDLIBS =
else
-libdebuginfod_so_LDLIBS = -lpthread $(libcurl_LIBS) $(fts_LIBS)
+libdebuginfod_so_LDLIBS = -lpthread $(libcurl_LIBS) $(fts_LIBS) $(libelf)
endif
$(LIBDEBUGINFOD_SONAME): $(srcdir)/libdebuginfod.map $(libdebuginfod_so_LIBS)
[PATCH] debuginfod: Support queries for ELF/DWARF sections
I'm resending this patch with a small modification. I added a new
field "progressfn_cancel" to debuginfod_client that indicates whether
the most recent query was cancelled due to progressfn returning 1.
If a server doesn't support section queries and the client begins
downloading a debuginfo or executable in an attempt to extract the
section, progressfn_cancel is used to indicate that if the first
query was cancelled by the progressfn then the second query should
also be skipped.
---
ChangeLog | 4 +
NEWS| 2 +
debuginfod/ChangeLog| 22 ++
debuginfod/Makefile.am | 2 +-
debuginfod/debuginfod-client.c | 295 +-
debuginfod/debuginfod-find.c| 21 +-
debuginfod/debuginfod.cxx | 433 +++-
debuginfod/debuginfod.h.in | 6 +
debuginfod/libdebuginfod.map| 1 +
doc/ChangeLog | 6 +
doc/Makefile.am | 1 +
doc/debuginfod_find_debuginfo.3 | 26 +-
doc/debuginfod_find_section.3 | 1 +
tests/ChangeLog | 5 +
tests/Makefile.am | 4 +-
tests/run-debuginfod-section.sh | 141 +++
16 files changed, 886 insertions(+), 84 deletions(-)
create mode 100644 doc/debuginfod_find_section.3
create mode 100755 tests/run-debuginfod-section.sh
diff --git a/ChangeLog b/ChangeLog
index 60624183..29b2108e 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,7 @@
+2022-10-20 Aaron Merey
+
+ * NEWS: Add debuginfod_find_section.
+
2022-09-13 Aleksei Vetrov
* NEWS (libdwfl): Add dwfl_report_offline_memory.
diff --git a/NEWS b/NEWS
index 6ebd172c..3e290ff7 100644
--- a/NEWS
+++ b/NEWS
@@ -4,6 +4,8 @@ readelf: Add -D, --use-dynamic option.
debuginfod: Add --disable-source-scan option.
+debuginfod-client: Add new function debuginfod_find_section.
+
libdwfl: Add new function dwfl_get_debuginfod_client.
Add new function dwfl_frame_reg.
Add new function dwfl_report_offline_memory.
diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
index 59d50df1..aacf655b 100644
--- a/debuginfod/ChangeLog
+++ b/debuginfod/ChangeLog
@@ -1,3 +1,25 @@
+2022-10-20 Aaron Merey
+
+ * Makefile.am (libdebuginfod_so_LDLIBS): Add libelf.
+ * debuginfod-client.c (debuginfod_find_section): New function.
+ (extract_section): New function.
+ (maybe_debuginfo_section): New function.
+ (cache_find_section): New function.
+ (debuginfod_query_server): Add support for section queries.
+ * debuginfod-find.c (main): Add support for section queries.
+ * debuginfod.cxx (extract_section): New function.
+ (handle_buildid_f_match): Add section parameter. When non-empty,
+ try to create response from section contents.
+ (handle_buildid_r_match): Add section parameter. When non-empty,
+ try to create response from section contents.
+ (handle_buildid_match): Add section parameter. Pass to
+ handle_buildid_{f,r}_match.
+ (handle_buildid): Handle section name when artifacttype is set to
+ "section". Query upstream servers via debuginfod_find_section
+ when necessary.
+ (debuginfod.h.in): Add declaration for debuginfod_find_section.
+ (libdebuginfod.map): Add debuginfod_find_section.
+
2022-10-17 Frank Ch. Eigler
* debuginfod.cxx (main): Report libmicrohttpd version.
diff --git a/debuginfod/Makefile.am b/debuginfod/Makefile.am
index 435cb8a6..f27d6e2e 100644
--- a/debuginfod/Makefile.am
+++ b/debuginfod/Makefile.am
@@ -97,7 +97,7 @@ libdebuginfod_so_LIBS = libdebuginfod_pic.a
if DUMMY_LIBDEBUGINFOD
libdebuginfod_so_LDLIBS =
else
-libdebuginfod_so_LDLIBS = -lpthread $(libcurl_LIBS) $(fts_LIBS)
+libdebuginfod_so_LDLIBS = -lpthread $(libcurl_LIBS) $(fts_LIBS) $(libelf)
endif
$(LIBDEBUGINFOD_SONAME): $(srcdir)/libdebuginfod.map $(libdebuginfod_so_LIBS)
$(AM_V_CCLD)$(LINK) $(dso_LDFLAGS) -o $@ \
diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index 2a14d9d9..c50110b7 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -44,6 +44,7 @@
#include "system.h"
#include
#include
+#include
/* We might be building a bootstrap dummy library, which is really simple. */
#ifdef DUMMY_LIBDEBUGINFOD
@@ -55,6 +56,9 @@ int debuginfod_find_executable (debuginfod_client *c, const
unsigned char *b,
int s, char **p) { return -ENOSYS; }
int debuginfod_find_source (debuginfod_client *c, const unsigned char *b,
int s, const char *f, char **p) { return -ENOSYS;
}
+int debuginfod_find_section (debuginfod_client *c, const unsigned char *b,
+int s, const char *scn, char **p)
+ { return -ENOSYS; }
void debuginfod_set_progressfn(debuginfod_client *c,
debuginfod_progressfn_t fn) { }
Re: [PATCH] debuginfod: Support queries for ELF/DWARF sections
Hi Frank, On Mon, Oct 24, 2022 at 2:38 PM Frank Ch. Eigler wrote: > - use of write(2) to put files onto disk is not quite right; write(2) can > be partial, so you need a loop (or a macro wrapping a loop) Fixed. > - not sure I understand why the code worries about dots in or not in > section names. Why not just pass them verbatim throughout the code > base, and not worry about whether or not there's a dot? Does the > ELF standard even require a dot? Fair point. The motivation for providing a general section query API as opposed to an API for specific indices was to be flexible and accommodate a wide range of use cases. Insisting on the dot is contrary to this goal. > - not sure whether/why the I queries require a new _query_i view, as > opposed to running the _query_d & _query_e views union'd together. > I see an ORDER BY that's different here but not sure why bother; > if anything, the server could prefer one or the other type, based > on the same section-name heuristic as the client The idea here was to prevent cases where a section would be extracted from the executable on one server and from the debuginfo on another server. Granted this is only relevant when the file mtimes differ between servers and for sections that differ between debuginfo and executable. This situation seems unlikely and these particular sections aren't of much interest (ex. .shstrtab). I will remove this view and just use the existing ones. > - don't really see a need for the X-DEBUGINFOD-SECTION response > header, which simply echoes back the very same parameter the client > just requested; the other X-DEBUGINFOD-* headers are novel metadata > > - re. verbose logging in the section vs non-section case, suggest > just keeping the code simple (even if it makes the logs more verbose), > i.e., not duplicating if (...) clog << STUFF else clog << STUFF; > no biggie tho > > - the webapi docs in debuginfod.8 should document the new query type Fixed. Aaron
Re: [PATCH] debuginfod: Support queries for ELF/DWARF sections
Hi Mark, On Wed, Oct 26, 2022 at 11:06 AM Mark Wielaard wrote: > On Mon, 2022-10-24 at 14:38 -0400, Frank Ch. Eigler via Elfutils-devel > wrote: > > - not sure I understand why the code worries about dots in or not in > > section names. Why not just pass them verbatim throughout the code > > base, and not worry about whether or not there's a dot? Does the > > ELF standard even require a dot? > > I agree that just passing them through as is might be better. The ELF > standard doesn't say much about section names, just: > >Section names with a dot (.) prefix are reserved for the system, >although applications may use these sections if their existing >meanings are satisfactory. Applications may use names without the >prefix to avoid conflicts with system sections. Agreed, will fix. > I would drop the maybe_debuginfo_section heuristics. There are some > sections like .strtab/.symtab that are probably in the debug file, but > might be in the executable. I would assume that a named section can > normally be found in the debugfile and only use the executable as > fallback. > > So see if you can find the .debug file, if you can, then look for the > section by name. If it isn't SHT_NOBITS you found it. If it is > SHT_NOBITS the section should be in the exe. If the section cannot be > found by name (in the .debug file) you can stop searching, it also > won't be in the exe. If you cannot find the .debug file, or the section > was in the .debug file, but had type SHT_NOBITS then search for the exe > file and the named section in there. I like this heuristic. It's simpler and we don't have to update anything if/when a new section becomes common. > Finally, if the section comes from a file in the cache or if we have to > download it in full anyway, then extracting the section into its own > file seems slightly wasteful. It would be great if we could just report > back "here is the full exe/debug file which does contain the requested > section name". But that might make the interface a little ugly. > > int > debuginfod_find_section (debuginfod_client *client, > const unsigned char *build_id, > int build_id_len, > const char *section, char **path, > bool *file_is_elf) > > Maybe that is over-designed to avoid a little bit of disk waste? We'd save a bit of disk space but complicate the API and often cause client tools to have to do more work to get at the section contents. In the case of .gdb_index, gdb already knows how to read the index from a separate file. Of course it can read the section from an ELF file too but I suspect there might need to be some changes to teach it how to handle "unusual" ELF files that only contain a single section. > Since debuginfod-client.c already includes system.h it can use: > > static inline ssize_t > write_retry (int fd, const void *buf, size_t len) > > Which takes care of partial and/or interrupted write calls. Thanks that's exactly what I'm looking for. Aaron
[PATCH v3] debuginfod: Support queries for ELF/DWARF sections
v3 addresses Frank and Mark's v2 feedback.
v2 available here:
https://sourceware.org/pipermail/elfutils-devel/2022q4/005476.html
---
ChangeLog | 4 +
NEWS| 2 +
debuginfod/ChangeLog| 21 ++
debuginfod/Makefile.am | 2 +-
debuginfod/debuginfod-client.c | 263 +-
debuginfod/debuginfod-find.c| 21 +-
debuginfod/debuginfod.cxx | 383 ++--
debuginfod/debuginfod.h.in | 6 +
debuginfod/libdebuginfod.map| 1 +
doc/ChangeLog | 7 +
doc/Makefile.am | 1 +
doc/debuginfod.8| 10 +
doc/debuginfod_find_debuginfo.3 | 22 +-
doc/debuginfod_find_section.3 | 1 +
tests/ChangeLog | 5 +
tests/Makefile.am | 4 +-
tests/run-debuginfod-section.sh | 134 +++
17 files changed, 804 insertions(+), 83 deletions(-)
create mode 100644 doc/debuginfod_find_section.3
create mode 100755 tests/run-debuginfod-section.sh
diff --git a/ChangeLog b/ChangeLog
index 60624183..6f9b416a 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,7 @@
+2022-10-27 Aaron Merey
+
+ * NEWS: Add debuginfod_find_section.
+
2022-09-13 Aleksei Vetrov
* NEWS (libdwfl): Add dwfl_report_offline_memory.
diff --git a/NEWS b/NEWS
index 6ebd172c..3e290ff7 100644
--- a/NEWS
+++ b/NEWS
@@ -4,6 +4,8 @@ readelf: Add -D, --use-dynamic option.
debuginfod: Add --disable-source-scan option.
+debuginfod-client: Add new function debuginfod_find_section.
+
libdwfl: Add new function dwfl_get_debuginfod_client.
Add new function dwfl_frame_reg.
Add new function dwfl_report_offline_memory.
diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
index 59d50df1..b1d076fe 100644
--- a/debuginfod/ChangeLog
+++ b/debuginfod/ChangeLog
@@ -1,3 +1,24 @@
+2022-10-27 Aaron Merey
+
+ * Makefile.am (libdebuginfod_so_LDLIBS): Add libelf.
+ * debuginfod-client.c (debuginfod_find_section): New function.
+ (extract_section): New function.
+ (cache_find_section): New function.
+ (debuginfod_query_server): Add support for section queries.
+ * debuginfod-find.c (main): Add support for section queries.
+ * debuginfod.cxx (extract_section): New function.
+ (handle_buildid_f_match): Add section parameter. When non-empty,
+ try to create response from section contents.
+ (handle_buildid_r_match): Add section parameter. When non-empty,
+ try to create response from section contents.
+ (handle_buildid_match): Add section parameter. Pass to
+ handle_buildid_{f,r}_match.
+ (handle_buildid): Handle section name when artifacttype is set to
+ "section". Query upstream servers via debuginfod_find_section
+ when necessary.
+ (debuginfod.h.in): Add declaration for debuginfod_find_section.
+ (libdebuginfod.map): Add debuginfod_find_section.
+
2022-10-17 Frank Ch. Eigler
* debuginfod.cxx (main): Report libmicrohttpd version.
diff --git a/debuginfod/Makefile.am b/debuginfod/Makefile.am
index 435cb8a6..f27d6e2e 100644
--- a/debuginfod/Makefile.am
+++ b/debuginfod/Makefile.am
@@ -97,7 +97,7 @@ libdebuginfod_so_LIBS = libdebuginfod_pic.a
if DUMMY_LIBDEBUGINFOD
libdebuginfod_so_LDLIBS =
else
-libdebuginfod_so_LDLIBS = -lpthread $(libcurl_LIBS) $(fts_LIBS)
+libdebuginfod_so_LDLIBS = -lpthread $(libcurl_LIBS) $(fts_LIBS) $(libelf)
endif
$(LIBDEBUGINFOD_SONAME): $(srcdir)/libdebuginfod.map $(libdebuginfod_so_LIBS)
$(AM_V_CCLD)$(LINK) $(dso_LDFLAGS) -o $@ \
diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index 2a14d9d9..0e8ca89c 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -44,6 +44,7 @@
#include "system.h"
#include
#include
+#include
/* We might be building a bootstrap dummy library, which is really simple. */
#ifdef DUMMY_LIBDEBUGINFOD
@@ -55,6 +56,9 @@ int debuginfod_find_executable (debuginfod_client *c, const
unsigned char *b,
int s, char **p) { return -ENOSYS; }
int debuginfod_find_source (debuginfod_client *c, const unsigned char *b,
int s, const char *f, char **p) { return -ENOSYS;
}
+int debuginfod_find_section (debuginfod_client *c, const unsigned char *b,
+int s, const char *scn, char **p)
+ { return -ENOSYS; }
void debuginfod_set_progressfn(debuginfod_client *c,
debuginfod_progressfn_t fn) { }
void debuginfod_set_verbose_fd(debuginfod_client *c, int fd) { }
@@ -129,6 +133,9 @@ struct debuginfod_client
debuginfod_end needs to terminate. */
int default_progressfn_printed_p;
+ /* Indicates whether the last query was cancelled by progressfn. */
+ bool progressfn_cancel;
+
/* File descriptor to output any verbose messages if > 0. */
in
[PATCH v3] debuginfod: Support queries for ELF/DWARF sections
Hi Mark, Thanks again for the detailed review. I fixed the issues you pointed out. On Sat, Oct 29, 2022 at 8:29 PM Mark Wielaard wrote: > > @@ -1008,6 +1206,9 @@ debuginfod_query_server (debuginfod_client *c, > > snprintf(data[i].url, PATH_MAX, "%s/%s/%s/%s", server_url, > > build_id_bytes, type, escaped_string); > > } > > + else if (section) > > + snprintf(data[i].url, PATH_MAX, "%s/%s/%s/%s", server_url, > > + build_id_bytes, type, section); > > Does the section part need to be path escaped? Like the filename is > with curl_easy_escape? And if it is how does that interact with the > cache file name? I assumed that section names would not contain '/'. However I noticed that if we call debuginfod_find_section with a section name containing '/', it will return -EINVAL. This triggers the downloading of debuginfo in order to attempt client-side extraction. This is a waste so I changed the client to path-escape the section name for caching purposes just like the source query filename. > > + /* The servers may have lacked support for section queries. Attempt to > > + download the debuginfo or executable containing the section in order > > + to extract it. */ > > This does somewhat defeat the purpose of just getting the section data > of course. So we could also just return EINVAL to the user here, so > they have to get the whole debuginfo themselves. But maybe just > pretending it worked is fine to keep the code path of the user > simpler? This keeps the code path simpler for the user and like Frank mentioned it enables an intermediate server to extract the section and provide it to the client. This server will also cache the debuginfo/executable, possibly speeding up future requests. > > +tempfiles vlog-find.1 vlog-find.2 > > What are these tempfiles? This was left over from v2 of this patch and is now removed. Aaron --- Add new function debuginfod_find_section which queries debuginfod servers for the raw binary contents of the specified ELF/DWARF section in a file matching the given build-id. Extend the server webapi to support section queries. Section query URLS have the following format: /buildid/BUILDID/section/SECTION The server will attempt to extract the section from a debuginfo file matching the given build-id. If the debuginfo file cannot be found or the section has type SHT_NOBITS, the server will attempt to extract the section from the executable file matching the build-id. If the server is built without section query support, the client will attempt to download the debuginfo matching the build-id and extract the section. If the debuginfo file cannot be found or the section has type SHT_NOBITS, the server will attempt to download the executable file matching the build-id and extract the section. Signed-off-by: Aaron Merey --- ChangeLog | 4 + NEWS| 2 +- debuginfod/ChangeLog| 22 ++ debuginfod/Makefile.am | 2 +- debuginfod/debuginfod-client.c | 363 +++--- debuginfod/debuginfod-find.c| 15 +- debuginfod/debuginfod.cxx | 383 ++-- debuginfod/debuginfod.h.in | 6 + debuginfod/libdebuginfod.map| 1 + doc/ChangeLog | 7 + doc/Makefile.am | 1 + doc/debuginfod.8| 11 + doc/debuginfod_find_debuginfo.3 | 23 +- doc/debuginfod_find_section.3 | 1 + tests/ChangeLog | 5 + tests/Makefile.am | 4 +- tests/run-debuginfod-section.sh | 135 +++ 17 files changed, 882 insertions(+), 103 deletions(-) create mode 100644 doc/debuginfod_find_section.3 create mode 100755 tests/run-debuginfod-section.sh diff --git a/ChangeLog b/ChangeLog index 7bbb2c0f..9179dcea 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,7 @@ +2022-10-31 Aaron Merey + + * NEWS: Add debuginfod_find_section. + 2022-10-20 Mark Wielaard * Makefile.am (rpm): Remove --sign. diff --git a/NEWS b/NEWS index 3df652e3..9369f809 100644 --- a/NEWS +++ b/NEWS @@ -3,7 +3,7 @@ Version 0.188 some time after 0.187 readelf: Add -D, --use-dynamic option. debuginfod-client: Add $DEBUGINFOD_HEADERS_FILE setting to supply outgoing - HTTP headers. + HTTP headers. Add new function debuginfod_find_section. debuginfod: Add --disable-source-scan option. diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog index 1df903fe..92a880f8 100644 --- a/debuginfod/ChangeLog +++ b/debuginfod/ChangeLog @@ -1,3 +1,25 @@ +2022-10-31 Aaron Merey + + * Makefile.am (libdebuginfod_so_LDLIBS): Add libelf. + * debuginfod-client.c (debuginfod_find_section): New function. + (path_escape): New function. + (extract_section): New function. + (cache_find_section): New function. + (debuginfod_query_server): Add support for section queries.
[PATCH OB] debuginfod-client: Fix out-of-bounds write
Pushed as obvious.
Return early from path_escape when '\0' is seen in order to prevent
an out-of-bounds write to the dest buffer.
Signed-off-by: Aaron Merey
---
debuginfod/debuginfod-client.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index d097ca49..0c4a00cf 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -595,8 +595,7 @@ path_escape (const char *src, char *dest)
{
case '\0':
dest[q] = '\0';
-q = PATH_MAX-1; /* escape for loop too */
-break;
+ return;
case '/': /* escape / to prevent dir escape */
dest[q++]='#';
dest[q++]='#';
--
2.37.3
[PATCH OB] run-debuginfod-section.sh: Avoid zstd-compressed rpms
Pushed as obvious.
Only test using rpms without zstd compression. Older versions of
libarchive may fail to handle these.
Signed-off-by: Aaron Merey
---
tests/run-debuginfod-section.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/run-debuginfod-section.sh b/tests/run-debuginfod-section.sh
index a3cae349..66e53e83 100755
--- a/tests/run-debuginfod-section.sh
+++ b/tests/run-debuginfod-section.sh
@@ -79,7 +79,7 @@ wait_ready $PORT1 'thread_busy{role="scan"}' 0
# Build-id for a file in the one of the testsuite's F31 rpms
-RPM_BUILDID=420e9e3308971f4b817cc5bf83928b41a6909d88
+RPM_BUILDID=d44d42cbd7d915bc938c81333a21e355a6022fb7
# Download sections from files indexed with -F
testrun ${abs_top_builddir}/debuginfod/debuginfod-find -vvv section $BUILDID
.debug_info
--
2.37.3
[PATCH OB] Changelog: Update entries from previous commits.
Pushed as obvious. Update Changelogs with details from commits 04b1a3aa and 054b3bde9. Signed-off-by: Aaron Merey --- debuginfod/ChangeLog | 4 tests/ChangeLog | 5 + 2 files changed, 9 insertions(+) diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog index 92a880f8..50668e61 100644 --- a/debuginfod/ChangeLog +++ b/debuginfod/ChangeLog @@ -1,3 +1,7 @@ +2022-11-01 Aaron Merey + + * debuginfod-client.c (path_escape): Add early return. + 2022-10-31 Aaron Merey * Makefile.am (libdebuginfod_so_LDLIBS): Add libelf. diff --git a/tests/ChangeLog b/tests/ChangeLog index fb0dce82..b656029f 100644 --- a/tests/ChangeLog +++ b/tests/ChangeLog @@ -1,3 +1,8 @@ +2022-11-01 Aaron Merey + + * run-debuginfod-section.sh (RPM_BUILDID): Use buildid from non-zstd + compressed rpm. + 2022-10-31 Aaron Merey * Makefile.am (TESTS): Add run-debuginfod-section.sh. -- 2.37.3
[COMMITTED] debuginfod_find_section: Always update rc with most recent error code
debuginfod_find_section may attempt to download both the debuginfo
and executable matching the given build-id. If neither of these
files can be found, update rc to ensure that we always return an
accurate error code in this case.
Signed-off-by: Aaron Merey
---
debuginfod/ChangeLog | 5 +
debuginfod/debuginfod-client.c | 6 +-
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
index 4d576cae..5678002a 100644
--- a/debuginfod/ChangeLog
+++ b/debuginfod/ChangeLog
@@ -1,3 +1,8 @@
+2022-11-04 Aaron Merey
+
+ * debuginfod-client.c (debuginfod_find_section): Ensure rc
+ is always updated with the most recent error code.
+
2022-11-03 Frank Ch. Eigler
* debuginfod.cxx (handle_buildid): Correctly manage lifetime
diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index f48e32cc..99da05ef 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -1944,7 +1944,8 @@ debuginfod_find_section (debuginfod_client *client,
if (rc == -EEXIST)
{
- /* The section should be found in the executable. */
+ /* Either the debuginfo couldn't be found or the section should
+be in the executable. */
fd = debuginfod_find_executable (client, build_id,
build_id_len, &tmp_path);
if (fd > 0)
@@ -1952,6 +1953,9 @@ debuginfod_find_section (debuginfod_client *client,
rc = extract_section (fd, section, tmp_path, path);
close (fd);
}
+ else
+ /* Update rc so that we return the most recent error code. */
+ rc = fd;
}
free (tmp_path);
--
2.37.3
Re: [COMMITTED] debuginfod_find_section: Always update rc with most recent error code
Hi Mark,
On Mon, Nov 7, 2022 at 9:19 AM Mark Wielaard wrote:
> >if (rc == -EEXIST)
> > {
> > - /* The section should be found in the executable. */
> > + /* Either the debuginfo couldn't be found or the section should
> > + be in the executable. */
> >fd = debuginfod_find_executable (client, build_id,
> > build_id_len, &tmp_path);
> >if (fd > 0)
>
> I know this is in existing code, so this might have missed in a
> previous review. But shouldn't this be fd >= 0 ?
>
> That is what is checked in the rest of the code. Except for the
> debuginfod_find_section function which uses fd >0 twice.
>
> It is unlikely, but I think fd can be zero if it (stdin) was closed by
> the program for some reason. Then I think zero can be reused as new
> file descriptor?
Thanks for catching this typo, will merge a fix.
Aaron
[COMMITTED] debuginfod-client.c: Don't treat 0 as an error code.
Replace 'fd > 0' with 'fd >= 0' to avoid treating a possible file
descriptor as an error code.
Signed-off-by: Aaron Merey
---
debuginfod/ChangeLog | 5 +
debuginfod/debuginfod-client.c | 4 ++--
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
index 5678002a..a17bc5ab 100644
--- a/debuginfod/ChangeLog
+++ b/debuginfod/ChangeLog
@@ -1,3 +1,8 @@
+2022-11-07 Aaron Merey
+
+ * debuginfod-client.c (debuginfod_find_section): Don't treat 0 as an
+ error code.
+
2022-11-04 Aaron Merey
* debuginfod-client.c (debuginfod_find_section): Ensure rc
diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index 99da05ef..f9f26fd5 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -1936,7 +1936,7 @@ debuginfod_find_section (debuginfod_client *client,
}
return -ENOENT;
}
- if (fd > 0)
+ if (fd >= 0)
{
rc = extract_section (fd, section, tmp_path, path);
close (fd);
@@ -1948,7 +1948,7 @@ debuginfod_find_section (debuginfod_client *client,
be in the executable. */
fd = debuginfod_find_executable (client, build_id,
build_id_len, &tmp_path);
- if (fd > 0)
+ if (fd >= 0)
{
rc = extract_section (fd, section, tmp_path, path);
close (fd);
--
2.37.3
[OB PATCH] debuginfod-client.c: Download section even if cached executable didn't contain it.
Committing as obvious.
Before attempting to download a section, cache_find_section tries to
extract the section from existing files in the cache. If it's determined
that the section must not exist, cache_find_section returns -ENOENT to
indicate that the download should be skipped.
This patch fixes a bug where cache_find_section returns -ENOENT even
though the section exists. If the cache contains the executable but
not the debuginfo with the given build-id and the section only exists
in the debuginfo (such as any of the .debug_* sections), then
debuginfod_find_section returns -ENOENT even if the section could be
downloaded.
Fix this by having cache_find_section not return -ENOENT unless cached
debuginfo was able to be read.
Signed-off-by: Aaron Merey
---
debuginfod/ChangeLog | 5 +
debuginfod/debuginfod-client.c | 26 --
2 files changed, 21 insertions(+), 10 deletions(-)
diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
index 2ddb7ca0..e961cce9 100644
--- a/debuginfod/ChangeLog
+++ b/debuginfod/ChangeLog
@@ -1,3 +1,8 @@
+2023-02-07 Aaron Merey
+
+ * debuginfod-client.c (cache_find_section): Avoid returning -ENOENT if
+ debuginfo wasn't found.
+
2022-12-21 Mark Wielaard
* debuginfod-client.c: Define CURL_AT_LEAST_VERSION.
diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index a16165bd..430d3cf4 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -785,23 +785,24 @@ out:
an ELF/DWARF section with name SCN_NAME. If found, extract the section
to a separate file in TARGET_CACHE_DIR and return a file descriptor
for the section file. The path for this file will be stored in USR_PATH.
- Return a negative errno if unsuccessful. */
+ Return a negative errno if unsuccessful. -ENOENT indicates that SCN_NAME
+ is confirmed to not exist. */
static int
cache_find_section (const char *scn_name, const char *target_cache_dir,
char **usr_path)
{
- int fd;
+ int debug_fd;
int rc = -EEXIST;
char parent_path[PATH_MAX];
/* Check the debuginfo first. */
snprintf (parent_path, PATH_MAX, "%s/debuginfo", target_cache_dir);
- fd = open (parent_path, O_RDONLY);
- if (fd >= 0)
+ debug_fd = open (parent_path, O_RDONLY);
+ if (debug_fd >= 0)
{
- rc = extract_section (fd, scn_name, parent_path, usr_path);
- close (fd);
+ rc = extract_section (debug_fd, scn_name, parent_path, usr_path);
+ close (debug_fd);
}
/* If the debuginfo file couldn't be found or the section type was
@@ -809,12 +810,17 @@ cache_find_section (const char *scn_name, const char
*target_cache_dir,
if (rc == -EEXIST)
{
snprintf (parent_path, PATH_MAX, "%s/executable", target_cache_dir);
- fd = open (parent_path, O_RDONLY);
+ int exec_fd = open (parent_path, O_RDONLY);
- if (fd >= 0)
+ if (exec_fd >= 0)
{
- rc = extract_section (fd, scn_name, parent_path, usr_path);
- close (fd);
+ rc = extract_section (exec_fd, scn_name, parent_path, usr_path);
+ close (exec_fd);
+
+ /* Don't return -ENOENT if the debuginfo wasn't opened. The
+section may exist in the debuginfo but not the executable. */
+ if (debug_fd < 0 && rc == -ENOENT)
+ rc = -EREMOTE;
}
}
--
2.39.1
Re: ☠ Buildbot (Sourceware): elfutils - failed test (failure) (master)
On Tue, Feb 7, 2023 at 9:43 PM builder--- via Elfutils-devel wrote: > > A new failure has been detected on builder elfutils-gentoo-sparc while > building elfutils. > > Full details are available at: > https://builder.sourceware.org/buildbot/#builders/225/builds/10 > > Build state: failed test (failure) > Revision: 53b596ef4018693403395d702045c15762af3da7 > Worker: gentoo-sparc > Build Reason: (unknown) > Blamelist: Aaron Merey > > Steps: > [...] > - 7: make check ( failure ) > Logs: > - stdio: > https://builder.sourceware.org/buildbot/#builders/225/builds/10/steps/7/logs/stdio > - test-suite.log: > https://builder.sourceware.org/buildbot/#builders/225/builds/10/steps/7/logs/test-suite_log Failure due to make command timeout. It looks like the only test that didn't finish is run-debuginfod-section.sh, which is related to my changes. But I'm not seeing how the changes might cause a timeout and unfortunately test-suite.log is empty. Aaron
[PATCH OB] debuginfod-client.c: Skip empty file creation for cancelled queries
Committing as obvious.
Empty files in the client cache are used to indicate that contacted
servers could not find a requested resource. Future queries for this
resource will not be attempted until the cache_miss_s duration has
passed.
Currently these empty files are also created when a query is cancelled
through the client's progressfn. This can occur, for example, when a
user cancels a download with ctrl-c.
This prevents user-cancelled queries from being retried promptly without
having to modify cache_miss_s. Fix this by skipping the creation of an
empty cache file when progressfn cancels a query.
Signed-off-by: Aaron Merey
---
debuginfod/ChangeLog | 5 +
debuginfod/debuginfod-client.c | 6 +++---
2 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
index f861eb72..5db5a753 100644
--- a/debuginfod/ChangeLog
+++ b/debuginfod/ChangeLog
@@ -1,3 +1,8 @@
+2023-03-17 Aaron Merey
+
+ * debuginfod-client.c (debuginfod_query_server): Do not create an
+ empty file in the cache if the query was cancelled by the progressfn.
+
2023-02-07 Aaron Merey
* debuginfod-client.c (cache_find_section): Avoid returning -ENOENT
diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index ef4d47e3..b33408eb 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -1667,9 +1667,9 @@ debuginfod_query_server (debuginfod_client *c,
}
} while (num_msg > 0);
- /* Create an empty file named as $HOME/.cache if the query fails
- with ENOENT.*/
- if (rc == -ENOENT)
+ /* Create an empty file in the cache if the query fails with ENOENT and
+ it wasn't cancelled early. */
+ if (rc == -ENOENT && !c->progressfn_cancel)
{
int efd = open (target_cache_path, O_CREAT|O_EXCL, DEFFILEMODE);
if (efd >= 0)
--
2.39.2
[OB PATCH] debuginfod-client.c: Avoid sscanf on mixed-case component of string
Committing as obvious.
sscanf is used to get the value of x-debuginfod-size from the http
headers. The format string used assumes that the header field name
is entirely lower case. However mixed-case field names are possible,
resulting in the value not being read.
Fix this by removing "x-debuginfod-size" from the format string.
Signed-off-by: Aaron Merey
---
debuginfod/ChangeLog | 5 +
debuginfod/debuginfod-client.c | 4 ++--
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
index 44dc3a15..c8de6ca0 100644
--- a/debuginfod/ChangeLog
+++ b/debuginfod/ChangeLog
@@ -1,3 +1,8 @@
+2023-03-30 Aaron Merey
+
+ * debuginfod-client.c (debuginfod_query_server): Avoid sscanf on
+ mixed-case component of string.
+
2023-03-29 Jan Alexander Steffens (heftig)
* debuginfod-client.c (debuginfod_query_server): s/futimes/futimens/
diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index 4b6f93a7..5dfc8e62 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -1495,9 +1495,9 @@ debuginfod_query_server (debuginfod_client *c,
{
long xdl;
char *hdr = strcasestr(c->winning_headers, "x-debuginfod-size");
+ size_t off = strlen("x-debuginfod-size:");
- if (hdr != NULL
- && sscanf(hdr, "x-debuginfod-size: %ld", &xdl) == 1)
+ if (hdr != NULL && sscanf(hdr + off, "%ld", &xdl) == 1)
dl_size = xdl;
}
}
--
2.39.2
[PATCH] [PATCH] debuginfod: Use the debuginfod-size response header
In some cases the content-length header may not be available in order
to pass to a progressfn. If content-length isn't available then attempt
to get the size of the download from the debuginfod-size header instead.
It should be mentioned that if a compressed file (ex. gzip) is being
transferred, the actual transfer length will be less than debuginfod-size.
In this case debuginfod-size is a best-guess upper bound on the size of
the transfer.
Signed-off-by: Aaron Merey
---
debuginfod/debuginfod-client.c | 83 ++
1 file changed, 43 insertions(+), 40 deletions(-)
diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index 024b0954..bf5f8b23 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -1106,11 +1106,45 @@ debuginfod_query_server (debuginfod_client *c,
goto out2;
}
+ long dl_size = 0;
+ if (target_handle && (c->progressfn || maxsize > 0))
+{
+ /* Get size of file being downloaded. NB: If going through
+ deflate-compressing proxies, this number is likely to be
+ unavailable, so -1 may show. */
+ CURLcode curl_res;
+#ifdef CURLINFO_CONTENT_LENGTH_DOWNLOAD_T
+ curl_off_t cl;
+ curl_res = curl_easy_getinfo(target_handle,
+ CURLINFO_CONTENT_LENGTH_DOWNLOAD_T,
+ &cl);
+ if (curl_res == CURLE_OK && cl >= 0)
+dl_size = (cl > LONG_MAX ? LONG_MAX : (long)cl);
+#else
+ double cl;
+ curl_res = curl_easy_getinfo(target_handle,
+ CURLINFO_CONTENT_LENGTH_DOWNLOAD,
+ &cl);
+ if (curl_res == CURLE_OK)
+dl_size = (cl >= (double)(LONG_MAX+1UL) ? LONG_MAX : (long)cl);
+#endif
+ /* If Content-Length is -1, try to get the size from
+ X-Debuginfod-Size */
+ if (dl_size == -1 && c->winning_headers != NULL)
+{
+ double xdl;
+ char *hdr = strstr(c->winning_headers, "x-debuginfod-size");
+
+ if (hdr != NULL
+ && sscanf(hdr, "x-debuginfod-size: %lf", &xdl) == 1)
+dl_size = (xdl >= (double)(LONG_MAX+1UL) ? LONG_MAX :
(long)xdl);
+}
+}
+
if (c->progressfn) /* inform/check progress callback */
{
loops ++;
- long pa = loops; /* default params for progress callback */
- long pb = 0; /* transfer_timeout tempting, but loops != elapsed-time
*/
+ long pa = loops; /* default param for progress callback */
if (target_handle) /* we've committed to a server; report its
download progress */
{
CURLcode curl_res;
@@ -1130,50 +1164,19 @@ debuginfod_query_server (debuginfod_client *c,
pa = (dl >= (double)(LONG_MAX+1UL) ? LONG_MAX : (long)dl);
#endif
- /* NB: If going through deflate-compressing proxies, this
- number is likely to be unavailable, so -1 may show. */
-#ifdef CURLINFO_CONTENT_LENGTH_DOWNLOAD_T
- curl_off_t cl;
- curl_res = curl_easy_getinfo(target_handle,
- CURLINFO_CONTENT_LENGTH_DOWNLOAD_T,
- &cl);
- if (curl_res == 0 && cl >= 0)
-pb = (cl > LONG_MAX ? LONG_MAX : (long)cl);
-#else
- double cl;
- curl_res = curl_easy_getinfo(target_handle,
- CURLINFO_CONTENT_LENGTH_DOWNLOAD,
- &cl);
- if (curl_res == 0)
-pb = (cl >= (double)(LONG_MAX+1UL) ? LONG_MAX : (long)cl);
-#endif
}
- if ((*c->progressfn) (c, pa, pb))
+ if ((*c->progressfn) (c, pa, dl_size))
break;
}
+
/* Check to see if we are downloading something which exceeds maxsize,
if set.*/
- if (maxsize > 0 && target_handle)
+ if (target_handle && dl_size > maxsize && maxsize > 0)
{
- long dl_size = 0;
-#ifdef CURLINFO_SIZE_DOWNLOAD_T
- curl_off_t download_size_t;
- if (curl_easy_getinfo(target_handle,
CURLINFO_CONTENT_LENGTH_DOWNLOAD_T,
-&download_size_t) ==
CURLE_OK)
-dl_size = download_size_t >= (double)(LONG_MAX+1UL) ? LONG_MAX :
(long)download_size_t;
-#else
- double download_size;
- if (curl_easy_getinfo(target_handle,
CURLINFO_CONTENT_LENGTH_DOWNLOAD,
-&download_size) ==
CURLE_OK)
-dl_size = download_size >= (double)(LONG_MAX+1UL) ? LONG_MAX :
(long)download_size;
-#endif
-if (dl_size > maxsize)
- {
-if (vfd >=0)
- dprintf(v
[PATCH] PR29022: 000-permissions files cause problems for backups
000-permission files currently used for negative caching can cause
permission problems for some backup software and disk usage checkers.
Fix this by using empty files to for negative caching instead.
https://sourceware.org/bugzilla/show_bug.cgi?id=29022
Signed-off-by: Aaron Merey
---
debuginfod/ChangeLog | 5 ++
debuginfod/debuginfod-client.c| 90 +++
tests/ChangeLog | 5 ++
tests/Makefile.am | 4 +-
tests/run-debuginfod-federation-link.sh | 4 +-
tests/run-debuginfod-federation-metrics.sh| 4 +-
tests/run-debuginfod-federation-sqlite.sh | 4 +-
...on.sh => run-debuginfod-negative-cache.sh} | 6 +-
tests/run-debuginfod-tmp-home.sh | 2 +-
9 files changed, 77 insertions(+), 47 deletions(-)
rename tests/{run-debuginfod-000-permission.sh =>
run-debuginfod-negative-cache.sh} (92%)
diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
index 578d951d..6c2edbdf 100644
--- a/debuginfod/ChangeLog
+++ b/debuginfod/ChangeLog
@@ -1,3 +1,8 @@
+2022-04-05 Aaron Merey
+
+ * debuginfod-client.c: Represent negative caching with empty files
+ instead of 000-permission files.
+
2022-04-03 Frank Ch. Eigler
* debuginfod.cxx (main): Use single dual-stack daemon setup,
diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index 41ba88a5..0fe791a0 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -138,7 +138,7 @@ static const char *cache_clean_interval_filename =
"cache_clean_interval_s";
static const long cache_clean_default_interval_s = 86400; /* 1 day */
/* The cache_miss_default_s within the debuginfod cache specifies how
- frequently the 000-permision file should be released.*/
+ frequently the empty file should be released.*/
static const long cache_miss_default_s = 600; /* 10 min */
static const char *cache_miss_filename = "cache_miss_s";
@@ -767,41 +767,61 @@ debuginfod_query_server (debuginfod_client *c,
if (rc != 0)
goto out;
- struct stat st;
- /* Check if the file exists and it's of permission 000; must check
- explicitly rather than trying to open it first (PR28240). */
- if (stat(target_cache_path, &st) == 0
- && (st.st_mode & 0777) == 0)
+ /* Check if the target is already in the cache. */
+ int fd = open(target_cache_path, O_RDONLY);
+ if (fd >= 0)
{
- time_t cache_miss;
-
- rc = debuginfod_config_cache(cache_miss_path, cache_miss_default_s, &st);
- if (rc < 0)
-goto out;
+ struct stat st;
+ if (fstat(fd, &st) != 0)
+{
+ close (fd);
+ rc = -errno;
+ goto out;
+}
- cache_miss = (time_t)rc;
- if (time(NULL) - st.st_mtime <= cache_miss)
+ /* If the file is non-empty, then we are done. */
+ if (st.st_size > 0)
{
- rc = -ENOENT;
- goto out;
- }
+ if (path != NULL)
+{
+ *path = strdup(target_cache_path);
+ if (*path == NULL)
+{
+ close (fd);
+ rc = -errno;
+ goto out;
+}
+}
+ /* Success */
+ rc = fd;
+ goto out;
+}
else
-/* TOCTOU non-problem: if another task races, puts a working
- download or a 000 file in its place, unlinking here just
- means WE will try to download again as uncached. */
-unlink(target_cache_path);
-}
-
- /* If the target is already in the cache (known not-000 - PR28240),
- then we are done. */
- int fd = open (target_cache_path, O_RDONLY);
- if (fd >= 0)
-{
- /* Success */
- if (path != NULL)
-*path = strdup(target_cache_path);
- rc = fd;
- goto out;
+{
+ /* The file is empty. Attempt to download only if enough time
+ has passed since the last attempt. */
+ time_t cache_miss;
+ rc = debuginfod_config_cache(cache_miss_path,
+ cache_miss_default_s, &st);
+ if (rc < 0)
+{
+ close(fd);
+ goto out;
+}
+
+ cache_miss = (time_t)rc;
+ if (time(NULL) - st.st_mtime <= cache_miss)
+{
+ close(fd);
+ rc = -ENOENT;
+ goto out;
+}
+ else
+/* TOCTOU non-problem: if another task races, puts a working
+ download or an empty file in its place, unlinking here just
+ means WE will try to download again as uncached. */
+unlink(target_cache_path);
+}
}
long timeout = default_timeout;
@@ -1298,11 +1318,11 @@ debuginfod_query_server (debuginfod_client *c,
}
} while (num_msg > 0);
- /* Create a 000-permission file named as $HOME/.
Re: caching failed lookups of debuginfo?
Hi Milian, On Fri, Apr 8, 2022 at 5:08 PM Milian Wolff wrote: > I can reproduce it now suddenly with debuginfod-find too: > > ``` > $ > debuginfod-find debuginfo 85766e9d8458b16e9c7ce6e07c712c02b8471dbc > debuginfod_find_debuginfo 85766e9d8458b16e9c7ce6e07c712c02b8471dbc > server urls "https://debuginfod.archlinux.org/"; > checking build-id > checking cache dir /home/milian/.cache/debuginfod_client > using timeout 90 > init server 0 https://debuginfod.archlinux.org/buildid > url 0 https://debuginfod.archlinux.org/buildid/ > 85766e9d8458b16e9c7ce6e07c712c02b8471dbc/debuginfo > query 1 urls in parallel > server response HTTP response code said error > url 0 The requested URL returned error: 404 > not found No such file or directory (err=-2) > Server query failed: No such file or directory > ``` > > I do see an empty `/home/milian/.cache/debuginfod_client/ > 85766e9d8458b16e9c7ce6e07c712c02b8471dbc/debuginfo` file. But the server is > still queried (i.e. rerunning the above command always produces the same > output for me). The lookup is costly too at ~70ms overall: I am not able to reproduce this on Fedora 35 using commit 8db849976f070. The first time I run debuginfod-find I get similar output to you: ``` debuginfod_find_debuginfo 85766e9d8458b16e9c7ce6e07c712c02b8471dbc server urls "https://debuginfod.archlinux.org/"; checking build-id checking cache dir /home/amerey/.cache/debuginfod_client using timeout 90 init server 0 https://debuginfod.archlinux.org/buildid url 0 https://debuginfod.archlinux.org/buildid/85766e9d8458b16e9c7ce6e07c712c02b8471dbc/debuginfo query 1 urls in parallel server response HTTP response code said error url 0 The requested URL returned error: 404 not found No such file or directory (err=-2) Server query failed: No such file or directory ``` But when I rerun debuginfod-find I get output indicating that the cache was checked: ``` debuginfod_find_debuginfo 85766e9d8458b16e9c7ce6e07c712c02b8471dbc server urls "https://debuginfod.archlinux.org/"; checking build-id checking cache dir /home/amerey/.cache/debuginfod_client not found No such file or directory (err=-2) Server query failed: No such file or directory ``` No calls to curl_multi_wait occur. Is there a "cache_miss_s" file in the top level of your debuginfod cache? It should contain the number of seconds that must elapse after a failed attempt before another query would be attempted (default is 600 seconds). Aaron
Re: caching failed lookups of debuginfo?
Thanks for spotting this Milian. On Fri, Apr 8, 2022 at 6:40 PM Mark Wielaard wrote: > I think we as developers keep clearing the cache to test stuff. But > that means we recreate the cache_miss file every time, so that gets a > new mtime. And if you are just testing for 10 minutes everything looks > fine... This explains why I couldn't reproduce it at first, 10 minutes hadn't elapsed. I tried it again sometime later and noticed that every debuginfod-find had started resulting in a query, but Milian got to the bottom of it first. Aaron
Re: [PATCH] PR29022: 000-permissions files cause problems for backups
I've revised this patch so that the negative-hit file's mtime is used
to calculate time since last download attempt instead of the cache_miss_s
file. I've also added a check for old 000-permission files so that they
are unlinked immediately if found.
Aaron
---
PR29022: 000-permissions files cause problems for backups
000-permission files currently used for negative caching can cause
permission problems for some backup software and disk usage checkers.
Fix this by using empty files to for negative caching instead.
https://sourceware.org/bugzilla/show_bug.cgi?id=29022
Signed-off-by: Aaron Merey
---
debuginfod/ChangeLog | 5 +
debuginfod/debuginfod-client.c| 94 ---
tests/ChangeLog | 5 +
tests/Makefile.am | 4 +-
tests/run-debuginfod-federation-link.sh | 4 +-
tests/run-debuginfod-federation-metrics.sh| 4 +-
tests/run-debuginfod-federation-sqlite.sh | 4 +-
...on.sh => run-debuginfod-negative-cache.sh} | 6 +-
tests/run-debuginfod-tmp-home.sh | 2 +-
9 files changed, 81 insertions(+), 47 deletions(-)
rename tests/{run-debuginfod-000-permission.sh =>
run-debuginfod-negative-cache.sh} (92%)
diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
index 578d951d..6c2edbdf 100644
--- a/debuginfod/ChangeLog
+++ b/debuginfod/ChangeLog
@@ -1,3 +1,8 @@
+2022-04-05 Aaron Merey
+
+ * debuginfod-client.c: Represent negative caching with empty files
+ instead of 000-permission files.
+
2022-04-03 Frank Ch. Eigler
* debuginfod.cxx (main): Use single dual-stack daemon setup,
diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index 41ba88a5..0d213bc9 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -138,7 +138,7 @@ static const char *cache_clean_interval_filename =
"cache_clean_interval_s";
static const long cache_clean_default_interval_s = 86400; /* 1 day */
/* The cache_miss_default_s within the debuginfod cache specifies how
- frequently the 000-permision file should be released.*/
+ frequently the empty file should be released.*/
static const long cache_miss_default_s = 600; /* 10 min */
static const char *cache_miss_filename = "cache_miss_s";
@@ -767,42 +767,66 @@ debuginfod_query_server (debuginfod_client *c,
if (rc != 0)
goto out;
- struct stat st;
- /* Check if the file exists and it's of permission 000; must check
- explicitly rather than trying to open it first (PR28240). */
- if (stat(target_cache_path, &st) == 0
- && (st.st_mode & 0777) == 0)
+ /* Check if the target is already in the cache. */
+ int fd = open(target_cache_path, O_RDONLY);
+ if (fd >= 0)
{
- time_t cache_miss;
-
- rc = debuginfod_config_cache(cache_miss_path, cache_miss_default_s, &st);
- if (rc < 0)
-goto out;
+ struct stat st;
+ if (fstat(fd, &st) != 0)
+{
+ close (fd);
+ rc = -errno;
+ goto out;
+}
- cache_miss = (time_t)rc;
- if (time(NULL) - st.st_mtime <= cache_miss)
+ /* If the file is non-empty, then we are done. */
+ if (st.st_size > 0)
{
- rc = -ENOENT;
- goto out;
- }
+ if (path != NULL)
+{
+ *path = strdup(target_cache_path);
+ if (*path == NULL)
+{
+ close (fd);
+ rc = -errno;
+ goto out;
+}
+}
+ /* Success */
+ rc = fd;
+ goto out;
+}
else
-/* TOCTOU non-problem: if another task races, puts a working
- download or a 000 file in its place, unlinking here just
- means WE will try to download again as uncached. */
-unlink(target_cache_path);
-}
-
- /* If the target is already in the cache (known not-000 - PR28240),
- then we are done. */
- int fd = open (target_cache_path, O_RDONLY);
- if (fd >= 0)
-{
- /* Success */
- if (path != NULL)
-*path = strdup(target_cache_path);
- rc = fd;
- goto out;
+{
+ /* The file is empty. Attempt to download only if enough time
+ has passed since the last attempt. */
+ time_t cache_miss;
+ time_t target_mtime = st.st_mtime;
+ rc = debuginfod_config_cache(cache_miss_path,
+ cache_miss_default_s, &st);
+ if (rc < 0)
+{
+ close(fd);
+ goto out;
+}
+
+ cache_miss = (time_t)rc;
+ if (time(NULL) - target_mtime <= cache_miss)
+{
+ close(fd);
+ rc = -ENOENT;
+ goto out;
+}
+ else
+/* TOCTOU non-problem: if another task races, puts a working
+ download or an empt
Re: [PATCH] PR29022: 000-permissions files cause problems for backups
Hi Mark, Thanks for the review. I fixed the issues you pointed out (good catch re. setting rc before closing fd) and pushed as commit 8b568fdea8 with Tested-by: Milian Wolff added. Milian, thanks again for taking a look at this. Aaron
Re: [PATCH] [PATCH] debuginfod: Use the debuginfod-size response header
On Thu, Mar 31, 2022 at 1:44 PM Mark Wielaard wrote:
> Just a question about this part:
>
> > + /* If Content-Length is -1, try to get the size from
> > + X-Debuginfod-Size */
> > + if (dl_size == -1 && c->winning_headers != NULL)
> > +{
> > + double xdl;
> > + char *hdr = strstr(c->winning_headers, "x-debuginfod-size");
> > +
> > + if (hdr != NULL
> > + && sscanf(hdr, "x-debuginfod-size: %lf", &xdl) == 1)
> > +dl_size = (xdl >= (double)(LONG_MAX+1UL) ? LONG_MAX :
> > (long)xdl);
> > +}
> > +}
>
> In debuginfod.cxx the header is spelled all uppercase as "X-DEBUGINFOD-
> SIZE" which is also what is checked for in the run-debuginfod-response-
> headers.sh test. So shouldn't the above also be all uppercase or should
> you use strcasestr?
strcasestr is definitely better here. I meant to replace strstr with it
but it looks like I missed that.
> When using sscanf why are you using a double and %lf? Isn't it simpler
> to use a long and %ld?
It was done out of an (excessive) abundance of caution in case the
value from the header was greater than LONG_MAX. x-debuginfod-size is
derived from an off_t value so this currently isn't possible, so yes it
would be simpler to stick with long and %ld. Fixed.
> Is there a way to test this easily? When would Content-Length not be
> available?
AFAIK Content-Length isn't available only when a debuginfod server is
configured with an httpd proxy that compresses files on-the-fly. If the
response headers are finalized before compression is finished,
Content-Length won't be present.
Short of setting up an httpd-proxied server in the testsuite I'm not sure
exactly how to test this. We currently have tests that check for the
presence of x-debuginfod-size and we could add tests to at least verify
that the value in the header matches the uncompressed size of the file
being transferred.
Aaron
[PATCH 2/2] debuginfod: ensure X-DEBUGINFOD-SIZE contains file size
For archived files X-DEBUGINFOD-SIZE currently contains the size of the
archive instead of the size of the uncompressed file. Fix this.
Also add testcases to verify X-DEBUGINFOD-SIZE contains uncompressed
file sizes.
Signed-off-by: Aaron Merey
---
debuginfod/debuginfod.cxx| 11 +--
tests/run-debuginfod-response-headers.sh | 8
2 files changed, 17 insertions(+), 2 deletions(-)
diff --git a/debuginfod/debuginfod.cxx b/debuginfod/debuginfod.cxx
index 9c0217f6..d61757ae 100644
--- a/debuginfod/debuginfod.cxx
+++ b/debuginfod/debuginfod.cxx
@@ -1789,11 +1789,18 @@ handle_buildid_r_match (bool internal_req_p,
std::string file = b_source1.substr(b_source1.find_last_of("/")+1,
b_source1.length());
add_mhd_response_header (r, "Content-Type",
"application/octet-stream");
- add_mhd_response_header (r, "X-DEBUGINFOD-SIZE",
- to_string(fs.st_size).c_str());
add_mhd_response_header (r, "X-DEBUGINFOD-ARCHIVE",
b_source0.c_str());
add_mhd_response_header (r, "X-DEBUGINFOD-FILE", file.c_str());
+ rc = fstat (fd, &fs);
+ if (rc == 0)
+add_mhd_response_header (r, "X-DEBUGINFOD-SIZE",
+ to_string(fs.st_size).c_str());
+ else
+{
+ close (fd);
+ throw libc_exception (errno, string("stat ") + b_source1 + "
archive " + b_source0);
+}
add_mhd_last_modified (r, archive_entry_mtime(e));
if (verbose > 1)
obatched(clog) << "serving archive " << b_source0 << " file " <<
b_source1 << endl;
diff --git a/tests/run-debuginfod-response-headers.sh
b/tests/run-debuginfod-response-headers.sh
index 10b2ab49..62c43887 100755
--- a/tests/run-debuginfod-response-headers.sh
+++ b/tests/run-debuginfod-response-headers.sh
@@ -86,6 +86,14 @@ grep 'X-DEBUGINFOD-FILE: ' vlog-find$PORT1.2
grep 'X-DEBUGINFOD-SIZE: ' vlog-find$PORT1.2
grep 'X-DEBUGINFOD-ARCHIVE: ' vlog-find$PORT1.2
+# Check that X-DEBUGINFOD-SIZE matches the size of each file
+for file in vlog-find$PORT1.1 vlog-find$PORT1.2
+do
+st_size=$(stat -c%s $(tail -n 1 $file))
+x_debuginfod_size=$(grep 'X-DEBUGINFOD-SIZE' $file | egrep -o '[0-9]+')
+test $st_size -eq $x_debuginfod_size
+done
+
kill $PID1
wait $PID1
PID1=0
--
2.35.1
Re: [PATCH 2/2] debuginfod: ensure X-DEBUGINFOD-SIZE contains file size
Please ignore the "2/2" in the subject line, this patch is not part of a series. Aaron
Re: [PATCH] debuginfod: Use the debuginfod-size response header
I've updated the patch below with the changes Mark recommended.
A couple X-DEBUGINFOD-SIZE tests were added in another patch I recently
posted [1] that also fixes a bug when computing this header's value for
an archived file.
Aaron
[1] https://sourceware.org/pipermail/elfutils-devel/2022q2/004936.html
>From b56f1568b832fe1c23ffb711aa0486fbd2c5067f Mon Sep 17 00:00:00 2001
From: Aaron Merey
Date: Tue, 11 Jan 2022 22:07:55 -0500
debuginfod: Use the debuginfod-size response header
In some cases the content-length header may not be available in order
to pass to a progressfn. If content-length isn't available then attempt
to get the size of the download from the debuginfod-size header instead.
It should be mentioned that if a compressed file (ex. gzip) is being
transferred, the actual transfer length will be less than debuginfod-size.
In this case debuginfod-size is a best-guess upper bound on the size of
the transfer.
Signed-off-by: Aaron Merey
---
debuginfod/debuginfod-client.c | 83 ++
1 file changed, 43 insertions(+), 40 deletions(-)
diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index 58ef6442..de7ea1af 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -1130,11 +1130,45 @@ debuginfod_query_server (debuginfod_client *c,
goto out2;
}
+ long dl_size = 0;
+ if (target_handle && (c->progressfn || maxsize > 0))
+{
+ /* Get size of file being downloaded. NB: If going through
+ deflate-compressing proxies, this number is likely to be
+ unavailable, so -1 may show. */
+ CURLcode curl_res;
+#ifdef CURLINFO_CONTENT_LENGTH_DOWNLOAD_T
+ curl_off_t cl;
+ curl_res = curl_easy_getinfo(target_handle,
+ CURLINFO_CONTENT_LENGTH_DOWNLOAD_T,
+ &cl);
+ if (curl_res == CURLE_OK && cl >= 0)
+dl_size = (cl > LONG_MAX ? LONG_MAX : (long)cl);
+#else
+ double cl;
+ curl_res = curl_easy_getinfo(target_handle,
+ CURLINFO_CONTENT_LENGTH_DOWNLOAD,
+ &cl);
+ if (curl_res == CURLE_OK)
+dl_size = (cl >= (double)(LONG_MAX+1UL) ? LONG_MAX : (long)cl);
+#endif
+ /* If Content-Length is -1, try to get the size from
+ X-Debuginfod-Size */
+ if (dl_size == -1 && c->winning_headers != NULL)
+{
+ long xdl;
+ char *hdr = strcasestr(c->winning_headers, "x-debuginfod-size");
+
+ if (hdr != NULL
+ && sscanf(hdr, "x-debuginfod-size: %ld", &xdl) == 1)
+dl_size = xdl;
+}
+}
+
if (c->progressfn) /* inform/check progress callback */
{
loops ++;
- long pa = loops; /* default params for progress callback */
- long pb = 0; /* transfer_timeout tempting, but loops != elapsed-time
*/
+ long pa = loops; /* default param for progress callback */
if (target_handle) /* we've committed to a server; report its
download progress */
{
CURLcode curl_res;
@@ -1154,50 +1188,19 @@ debuginfod_query_server (debuginfod_client *c,
pa = (dl >= (double)(LONG_MAX+1UL) ? LONG_MAX : (long)dl);
#endif
- /* NB: If going through deflate-compressing proxies, this
- number is likely to be unavailable, so -1 may show. */
-#ifdef CURLINFO_CONTENT_LENGTH_DOWNLOAD_T
- curl_off_t cl;
- curl_res = curl_easy_getinfo(target_handle,
- CURLINFO_CONTENT_LENGTH_DOWNLOAD_T,
- &cl);
- if (curl_res == 0 && cl >= 0)
-pb = (cl > LONG_MAX ? LONG_MAX : (long)cl);
-#else
- double cl;
- curl_res = curl_easy_getinfo(target_handle,
- CURLINFO_CONTENT_LENGTH_DOWNLOAD,
- &cl);
- if (curl_res == 0)
-pb = (cl >= (double)(LONG_MAX+1UL) ? LONG_MAX : (long)cl);
-#endif
}
- if ((*c->progressfn) (c, pa, pb))
+ if ((*c->progressfn) (c, pa, dl_size))
break;
}
+
/* Check to see if we are downloading something which exceeds maxsize,
if set.*/
- if (maxsize > 0 && target_handle)
+ if (target_handle && dl_size > maxsize && maxsize > 0)
{
- long dl_size = 0;
-#ifdef CURLINFO_SIZE_DOWNLOAD_T
- curl_off_t download_size_t;
- if (curl_easy_getinfo(target_handle,
CURLINFO_CONTENT_LENGTH_DOWNLOAD_T,
-&download_size_t) ==
CURLE_OK)
-dl_size = download_size_t >= (double)(LONG_MAX+1UL) ? LONG_MAX :
(long
Re: [PATCH] debuginfod: Use the debuginfod-size response header
On Sun, Apr 24, 2022 at 11:05 AM Mark Wielaard wrote: > Looks good. Pleas commit. Thanks, pushed as: commit 55fee962676fbff60c6b0469305bcb077910d64f Author: Aaron Merey Date: Tue Jan 11 22:07:55 2022 -0500 debuginfod: Use the debuginfod-size response header In some cases the content-length header may not be available in order to pass to a progressfn. If content-length isn't available then attempt to get the size of the download from the debuginfod-size header instead. It should be mentioned that if a compressed file (ex. gzip) is being transferred, the actual transfer length will be less than debuginfod-size. In this case debuginfod-size is a best-guess upper bound on the size of the transfer. Signed-off-by: Aaron Merey
[PATCH] debuginfod: ensure X-DEBUGINFOD-SIZE contains file size
Hi Frank,
On Fri, Apr 22, 2022 at 7:27 PM Frank Ch. Eigler wrote:
> > - add_mhd_response_header (r, "X-DEBUGINFOD-SIZE",
> > - to_string(fs.st_size).c_str());
>
> > + rc = fstat (fd, &fs);
> > + if (rc == 0)
> > +add_mhd_response_header (r, "X-DEBUGINFOD-SIZE",
> > + to_string(fs.st_size).c_str());
> > + else
> > +{
> > + close (fd);
> > + throw libc_exception (errno, string("stat ") + b_source1 + "
> > archive " + b_source0);
> > +}
>
> It shouldn't require a new fstat -- the archive component file's size
> should be available from libarchive already: archive_entry_size(e);
Fixed.
Aaron
>From 08e448456e27339aeb326828d44069028518038a Mon Sep 17 00:00:00 2001
From: Aaron Merey
Date: Mon, 25 Apr 2022 11:10:46 -0400
For archived files X-DEBUGINFOD-SIZE currently contains the size of the
archive instead of the size of the uncompressed file. Fix this.
Also add testcases to verify X-DEBUGINFOD-SIZE contains uncompressed
file sizes.
Signed-off-by: Aaron Merey
---
debuginfod/debuginfod.cxx| 2 +-
tests/run-debuginfod-response-headers.sh | 8
2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/debuginfod/debuginfod.cxx b/debuginfod/debuginfod.cxx
index adca8208..4aaf41c0 100644
--- a/debuginfod/debuginfod.cxx
+++ b/debuginfod/debuginfod.cxx
@@ -1790,7 +1790,7 @@ handle_buildid_r_match (bool internal_req_p,
add_mhd_response_header (r, "Content-Type",
"application/octet-stream");
add_mhd_response_header (r, "X-DEBUGINFOD-SIZE",
- to_string(fs.st_size).c_str());
+ to_string(archive_entry_size(e)).c_str());
add_mhd_response_header (r, "X-DEBUGINFOD-ARCHIVE",
b_source0.c_str());
add_mhd_response_header (r, "X-DEBUGINFOD-FILE", file.c_str());
diff --git a/tests/run-debuginfod-response-headers.sh
b/tests/run-debuginfod-response-headers.sh
index 10b2ab49..62c43887 100755
--- a/tests/run-debuginfod-response-headers.sh
+++ b/tests/run-debuginfod-response-headers.sh
@@ -86,6 +86,14 @@ grep 'X-DEBUGINFOD-FILE: ' vlog-find$PORT1.2
grep 'X-DEBUGINFOD-SIZE: ' vlog-find$PORT1.2
grep 'X-DEBUGINFOD-ARCHIVE: ' vlog-find$PORT1.2
+# Check that X-DEBUGINFOD-SIZE matches the size of each file
+for file in vlog-find$PORT1.1 vlog-find$PORT1.2
+do
+st_size=$(stat -c%s $(tail -n 1 $file))
+x_debuginfod_size=$(grep 'X-DEBUGINFOD-SIZE' $file | egrep -o '[0-9]+')
+test $st_size -eq $x_debuginfod_size
+done
+
kill $PID1
wait $PID1
PID1=0
--
2.35.1
Re: [PATCH] Introduce public dwfl_get_debuginfod_client API
Hi Milian, On Thu, Jul 7, 2022 at 10:47 AM Milian Wolff wrote: > > Dwfl can use debuginfod internally, which was so far totally opaque > to the outside. While the functionality is great for users of the > dwfl API, the long wait times induced by downloading of data over > debuginfod lead to complaints by endusers. To offer them a bit more > insight into the internal ongoings, one can now use e.g. > `debuginfod_set_progressfn` on the handle returned by > `dwfl_get_debuginfod_client` to report download progress. > > Rename get_client to dwfl_get_debuginfod_client and make it public. > Unconditionally compile debuginfod-client.c and stub the new public > function and always return NULL when debuginfod integration was > disabled. Thanks for the patch. This looks ok and I was able to successfully run the testsuite with and without debuginfod enabled. > @@ -70,7 +70,7 @@ libdwfl_a_SOURCES = dwfl_begin.c dwfl_end.c dwfl_error.c > dwfl_version.c \ Some line breaks may have accidentally snuck into the patch. I had to manually remove the line break right after "dwfl_error.c" for git to apply the patch without error. > +/* Return the internal debuginfod-client connection handle for the DWFL > session. > + When the client connection has not yet been initialized, it will be done > on the > + first call to this function. If elfutils is compiled without support for > debuginfod, Same goes for these line breaks. Aaron
Re: [PATCH] Introduce public dwfl_get_debuginfod_client API
On Thu, Jul 7, 2022 at 12:59 PM Milian Wolff wrote: > > On Donnerstag, 7. Juli 2022 18:40:05 CEST Aaron Merey wrote: >> > > Some line breaks may have accidentally snuck into the patch. I had to > > manually remove the line break right after "dwfl_error.c" for git to > > apply the patch without error. > > Ah yes sorry, I did not think about that when copy'n'pasting the git format- > patch output into my mail client. Should I resend with the line breaks fixed? Sure might as well resend. Aaron
Re: [PATCH] Introduce public dwfl_get_debuginfod_client API
Thanks. Let's wait for a maintainer to give it the ok before merging. Aaron
Re: [PATCH] Introduce public dwfl_get_debuginfod_client API
Hi Milian, There weren't any concerns with the patch so I've gone ahead and merged it as commit a4b1839c3c46. Thanks, Aaron
Re: [PATCH] Introduce public dwfl_get_debuginfod_client API
On Wed, Jul 13, 2022 at 3:36 PM Milian Wolff wrote: > I got spammed by a flood of mails from buildbot, but from what I can tell the > issues are all unrelated to my patch? Or did I break something? See e.g.: > > [1]: https://builder.sourceware.org/buildbot/#/builders/39/builds/41 These issues are unrelated to your patch. One has been fixed and we are looking into the ppc/s390x failures. Aaron
