Re: patch to commit soon: PR25375 debuginfod prefetching
Hi Frank, On Mon, 2020-02-24 at 17:29 -0500, Frank Ch. Eigler wrote: > This patch has been baking on my public servers awhile and can make a > huge difference in performance. It's not something immediately > obvious how or whether to test, as it's a pure performance improvement. > Planning to push shortly. As far as I understand it, it looks good. One tiny question: > @@ -2809,7 +2857,8 @@ main (int argc, char *argv[]) > fdcache_mbs = 1024; // 1 gigabyte >else > fdcache_mbs = sfs.f_bavail * sfs.f_bsize / 1024 / 1024 / 4; // 25% of > free space > - fdcache_fds = concurrency * 2; > + fdcache_prefetch = 64; // guesstimate storage is this much less costly > than re-decompression > + fdcache_fds = (concurrency + fdcache_prefetch) * 2; Here fdcache_prefetch is set and used before argp_parse () is called, which would set it to the user supplied value (if any). Is that intentional? Cheers, Mark
[Bug debuginfod/25583] Use libarchive to extract packages?
https://sourceware.org/bugzilla/show_bug.cgi?id=25583 Mark Wielaard changed: What|Removed |Added CC||mark at klomp dot org --- Comment #2 from Mark Wielaard --- (In reply to Frank Ch. Eigler from comment #1) > Interesting idea. OTOH, rpm2cpio and dpkg binaries are not too hard to come > by. > > One can experiment with the former already with git-master debuginfod with > the > "-Z .rpm" option instead of "-R". Performance seems to be roughly the same. If this works as well, then I would simply switch -R to: diff --git a/debuginfod/debuginfod.cxx b/debuginfod/debuginfod.cxx index 0acd70e4..c68aafa3 100644 --- a/debuginfod/debuginfod.cxx +++ b/debuginfod/debuginfod.cxx @@ -423,7 +423,7 @@ parse_opt (int key, char *arg, break; case 'F': scan_files = true; break; case 'R': - scan_archives[".rpm"]="rpm2cpio"; + scan_archives[".rpm"]="cat"; break; case 'U': scan_archives[".deb"]="dpkg-deb --fsys-tarfile"; It seems to just pass the run-debuginfod-find.sh testcase. > The latter is less compelling in that it'd require hard-coding the inner > data.tar.xz name and its processing ... meh. > > Maybe the status quo is good enough? If we can get rid of some extra dependencies with the same performance and no real downsides, except a couple of lines of extra code, I would go with this. Of course, still needs someone to write those couple of extra lines of code. -- You are receiving this mail because: You are on the CC list for the bug.
[Bug general/25525] configure: error: gcc with GNU99 support required
https://sourceware.org/bugzilla/show_bug.cgi?id=25525 Mark Wielaard changed: What|Removed |Added Status|WAITING |RESOLVED Resolution|--- |INVALID --- Comment #4 from Mark Wielaard --- (In reply to tjyr from comment #3) > Created attachment 12313 [details] > config.log > > here is the config.log. Thanks. So it shows clang is pretending to be gcc, but it doesn't really support the GNU99 C features that are needed to build elfutils. > and I use this tool in MacOS(10.14.6)。Is it possible run in macos？ Yes, but you need to install gcc first if you want to build it from source. -- You are receiving this mail because: You are on the CC list for the bug.
Re: patch to commit soon: PR25375 debuginfod prefetching
Hi - > > + fdcache_prefetch = 64; // guesstimate storage is this much less costly > > than re-decompression > > + fdcache_fds = (concurrency + fdcache_prefetch) * 2; > > Here fdcache_prefetch is set and used before argp_parse () is called, > which would set it to the user supplied value (if any). Is that > intentional? Yeah, the idea is to provide some reasonable defaults, and then allow the command line options to override them. In this case, the user might want to override both. It'll be specific to the site anyway, depending on distribution of rpm sizes vs. available storage. - FChE
[Bug debuginfod/25583] Use libarchive to extract packages?
https://sourceware.org/bugzilla/show_bug.cgi?id=25583 Frank Ch. Eigler changed: What|Removed |Added Status|WAITING |ASSIGNED --- Comment #3 from Frank Ch. Eigler --- ok, confirmed that relatively old bsdtar (libarchive 3.1.2 rhel7) supports rpm natively too. Will switch that over as Mark suggests. -- You are receiving this mail because: You are on the CC list for the bug.
Re: PR25369 rfc: debuginfod client api extension for progressfn prettying etc.
Hi Frank, On Mon, 2020-02-24 at 22:35 -0500, Frank Ch. Eigler wrote: > As a part of PR25369, I propose this small set of client api > extensions, requested by gdb developers and needed by personal > experience. (I plan to roll it out on my debuginfod servers shortly > to let it soak.) An end-user visible immediate difference is in the > DEBUGINFOD_PROGRESS=1 format message, which now looks like this: > > Downloading from debuginfod / > Downloading from debuginfod - > Downloading from debuginfod \ > Downloading from > http://very:8222/buildid/817dcbd2ce0ecce19f22cbe5e136b6d9196428aa/executable > 433130328/433130328 > > This latter is a bit long and should probably be abbreviated, wdyt? Might want to abbrev the build-id part to /81..aa/. The interesting part is which server is used imho. > I'd start reviewing this from the debuginfod.h header file outward. OK. Lets start there: > diff --git a/debuginfod/debuginfod.h b/debuginfod/debuginfod.h > index b4b6a3d2a6b9..53aeee7133ca 100644 > --- a/debuginfod/debuginfod.h > +++ b/debuginfod/debuginfod.h > @@ -1,5 +1,5 @@ > /* External declarations for the libdebuginfod client library. > - Copyright (C) 2019 Red Hat, Inc. > + Copyright (C) 2019-2020 Red Hat, Inc. > This file is part of elfutils. > > This file is free software; you can redistribute it and/or modify > @@ -71,10 +71,23 @@ int debuginfod_find_source (debuginfod_client *client, > const char *filename, > char **path); > > +/* Set a progress callback function. */ > typedef int (*debuginfod_progressfn_t)(debuginfod_client *c, long a, long b); > void debuginfod_set_progressfn(debuginfod_client *c, > debuginfod_progressfn_t fn); > > +/* Add an outgoing HTTP request "Header: Value". Copies string. */ > +int debuginfod_add_http_header (debuginfod_client *client, const char* > header); This one seems different from the others and has a specific use case just for the debuginfod server. Are you sure it is generic enough to be added as a new public interface? If we add this can we do it separately from other debuginfo-client progress improvements? > +/* Return currently active URL, if known. String owned by curl, do not > free. */ > +const char* debuginfod_get_url (debuginfod_client *client); This does seem useful with the comment that was already made, that lifetime of the returned string should be documented. I assume it is valid to call this after debuginfod_find_* has returned, but before debuginfod_end has been called? > +/* Set the user parameter. */ > +void debuginfod_set_user_data (debuginfod_client *client, void *value); > + > +/* Get the user parameter. */ > +void* debuginfod_get_user_data (debuginfod_client *client); In theory I like these additions. But I don't really see the point of how they are used. Is the only use case to pass the string "Progress"? If there are no real users for this then I think we should not add these at this time. Or is some other client using them? I am not really against it, but would prefer if we add them separately to keep the patches concise. Thanks, Mark
Re: PR25369 rfc: debuginfod client api extension for progressfn prettying etc.
Hi - > > This latter is a bit long and should probably be abbreviated, wdyt? > > Might want to abbrev the build-id part to /81..aa/. The interesting > part is which server is used imho. Yeah, OK. > > +/* Add an outgoing HTTP request "Header: Value". Copies string. */ > > +int debuginfod_add_http_header (debuginfod_client *client, const char* > > header); > > This one seems different from the others and has a specific use case > just for the debuginfod server. Are you sure it is generic enough to be > added as a new public interface? If we add this can we do it separately > from other debuginfo-client progress improvements? I think it has a chance to be useful to other clients too, for example for other proxy / authentication schemes. And given that there is a shared library boundary, private APIs aren't in easy reach. "separately" as in separate commits? ... I suppose, if it really matters. > > +/* Return currently active URL, if known. String owned by curl, do not > > free. */ > > +const char* debuginfod_get_url (debuginfod_client *client); > > This does seem useful with the comment that was already made, that > lifetime of the returned string should be documented. I assume it is > valid to call this after debuginfod_find_* has returned, but before > debuginfod_end has been called? Clarified this in a followup patch. No, only valid during the progress callback function itself. > > +/* Set the user parameter. */ > > +void debuginfod_set_user_data (debuginfod_client *client, void *value); > > + > > +/* Get the user parameter. */ > > +void* debuginfod_get_user_data (debuginfod_client *client); > > In theory I like these additions. But I don't really see the point of > how they are used. Is the only use case to pass the string "Progress"? That is for test coverage. > If there are no real users for this then I think we should not add > these at this time. Or is some other client using them? I am not really > against it, but would prefer if we add them separately to keep the > patches concise. GDB would use them pretty much immediately, to be able to prepare a more informative progress notification (like the file name whose debuginfo is being sought, instead of its buildid). - FChE
[Bug debuginfod/25375] optimization: speculative fdcache'ing of peers in decompressed archive
https://sourceware.org/bugzilla/show_bug.cgi?id=25375 Frank Ch. Eigler changed: What|Removed |Added Status|NEW |RESOLVED Resolution|--- |FIXED --- Comment #2 from Frank Ch. Eigler --- commit 577170fc84e1 includes slight merge tweaks from previous version -- You are receiving this mail because: You are on the CC list for the bug.
Re: PR25369 rfc: debuginfod client api extension for progressfn prettying etc.
Hi Frank, On Tue, 2020-02-25 at 10:32 -0500, Frank Ch. Eigler wrote: > > > +/* Add an outgoing HTTP request "Header: Value". Copies string. */ > > > +int debuginfod_add_http_header (debuginfod_client *client, const char* > > > header); > > > > This one seems different from the others and has a specific use case > > just for the debuginfod server. Are you sure it is generic enough to be > > added as a new public interface? If we add this can we do it separately > > from other debuginfo-client progress improvements? > > I think it has a chance to be useful to other clients too, for example > for other proxy / authentication schemes. And given that there is a > shared library boundary, private APIs aren't in easy reach. > "separately" as in separate commits? ... I suppose, if it really > matters. Yes, please as a separate commit. It makes it so much easier on the reviewers if the patches are smaller. The reason I am slightly hesitant is because this feels like a feature with corner cases that might not be clear. What about other headers than Agent string if they have been set/will be set for example. Could you expand a little on the use case? I see you set an X- Forwarded-For header, but that seems it can be easily forged. I see it might be interesting for testing, but would you use it in production? > > > +/* Return currently active URL, if known. String owned by curl, do not > > > free. */ > > > +const char* debuginfod_get_url (debuginfod_client *client); > > > > This does seem useful with the comment that was already made, that > > lifetime of the returned string should be documented. I assume it is > > valid to call this after debuginfod_find_* has returned, but before > > debuginfod_end has been called? > > Clarified this in a followup patch. No, only valid during the progress > callback function itself. That is a pity, it seems useful without having to add a progress function. Then we should probably make sure it doesn't return a value in that case, because I suspect people will use it otherwise and will complain if we break it later. But if we can make it work when the target_handle is valid, that would be nice. > > > +/* Set the user parameter. */ > > > +void debuginfod_set_user_data (debuginfod_client *client, void *value); > > > + > > > +/* Get the user parameter. */ > > > +void* debuginfod_get_user_data (debuginfod_client *client); > > > > In theory I like these additions. But I don't really see the point of > > how they are used. Is the only use case to pass the string "Progress"? > > That is for test coverage. Better to have a "real" test imho. Or at least comment it, so someone doesn't clean up the code. > > If there are no real users for this then I think we should not add > > these at this time. Or is some other client using them? I am not > > really > > against it, but would prefer if we add them separately to keep the > > patches concise. > > GDB would use them pretty much immediately, to be able to prepare a > more informative progress notification (like the file name whose > debuginfo is being sought, instead of its buildid). OK. Yes, that seems like a good use case. But can we add this as a separate feature in a separate commit? Thanks, Mark
Re: PR25369 rfc: debuginfod client api extension for progressfn prettying etc.
Hi - > > I think it has a chance to be useful to other clients too, for example > > for other proxy / authentication schemes. And given that there is a > > shared library boundary, private APIs aren't in easy reach. > > "separately" as in separate commits? ... I suppose, if it really > > matters. > > Yes, please as a separate commit. It makes it so much easier on the > reviewers if the patches are smaller. OK, trading merge pains I guess. > The reason I am slightly hesitant is because this feels like a feature > with corner cases that might not be clear. What about other headers > than Agent string if they have been set/will be set for example. The agent one is the only one that the client library currently sets on its own initiative by default. That's why it's a special case. The underlying libcurl api does not let one -remove- a custom header, only add new ones. > Could you expand a little on the use case? I see you set an X- > Forwarded-For header, but that seems it can be easily forged. I see > it might be interesting for testing, but would you use it in > production? These XFF headers are interesting when one wants to backtrace a federated request. Within a trusted tree of servers, it lets an administrator figure out the ultimate client, for blacklisting or special processing if necessary. So yes, absolutely in production. (It doesn't matter if the initial parts are forged by a client as long as the trailing parts are appended by the trusted tree.) > > Clarified this in a followup patch. No, only valid during the progress > > callback function itself. > > That is a pity, it seems useful without having to add a progress > function. Then we should probably make sure it doesn't return a value > in that case, because I suspect people will use it otherwise and will > complain if we break it later. It returns NULL. > But if we can make it work when the target_handle is valid, that would > be nice. The duration of a debuginfod_find_* call is exactly when the target_handle is valid. The latter gets cleaned up when the former finishes up. > > > > +/* Set the user parameter. */ > > > > +void debuginfod_set_user_data (debuginfod_client *client, void *value); > > > > + > > > > +/* Get the user parameter. */ > > > > +void* debuginfod_get_user_data (debuginfod_client *client); > > > > > > In theory I like these additions. But I don't really see the point of > > > how they are used. Is the only use case to pass the string "Progress"? > > > > That is for test coverage. > > Better to have a "real" test imho. Or at least comment it, so someone > doesn't clean up the code. It's a real test in that the testsuite asserts that the value is preserved. I can certainly clarify the comment. > But can we add this as a separate feature in a separate commit? OK. - FChE
[Bug debuginfod/25583] Use libarchive to extract packages?
https://sourceware.org/bugzilla/show_bug.cgi?id=25583 Frank Ch. Eigler changed: What|Removed |Added Status|ASSIGNED|RESOLVED Resolution|--- |FIXED --- Comment #4 from Frank Ch. Eigler --- pushed as obvious diff --git a/debuginfod/debuginfod.cxx b/debuginfod/debuginfod.cxx index be3868bb1e42..7c7e85eb6d14 100644 --- a/debuginfod/debuginfod.cxx +++ b/debuginfod/debuginfod.cxx @@ -426,7 +426,7 @@ parse_opt (int key, char *arg, break; case 'F': scan_files = true; break; case 'R': - scan_archives[".rpm"]="rpm2cpio"; + scan_archives[".rpm"]="cat"; // libarchive groks rpm natively break; case 'U': scan_archives[".deb"]="dpkg-deb --fsys-tarfile"; -- You are receiving this mail because: You are on the CC list for the bug.
[Bug debuginfod/25600] New: debuginfo-client should handle file:// URLs
https://sourceware.org/bugzilla/show_bug.cgi?id=25600
Bug ID: 25600
Summary: debuginfo-client should handle file:// URLs
Product: elfutils
Version: unspecified
Status: NEW
Severity: normal
Priority: P2
Component: debuginfod
Assignee: unassigned at sourceware dot org
Reporter: fche at redhat dot com
CC: elfutils-devel at sourceware dot org
Target Milestone: ---
While this may be useful only for local testing, it's still good to have.
It's being blocked by error handling logic in -client.c line 728ish, as
identified by kkleine, because the 200 response code is only valid for http.
file:// scheme returns 0 for success here. So this code would need to do
some scheme-sensitive logic, such as curl's own src/tool_operate.c:
if(CURLE_OK == result) {
char *effective_url = NULL;
curl_easy_getinfo(curl, CURLINFO_EFFECTIVE_URL, &effective_url);
if(effective_url &&
curl_strnequal(effective_url, "http", 4)) {
/* This was HTTP(S) */
curl_easy_getinfo(curl, CURLINFO_RESPONSE_CODE, &response);
if(response != 200 && response != 206) {
i.e., if result was CURLE_OK, assume success,
unless http and we have a bad response code.
--
You are receiving this mail because:
You are on the CC list for the bug.
