Re: [debuginfod] Minor run-debuginfod-find.sh test fixes
Hi Noah, On Wed, 2021-07-21 at 15:26 -0400, Noah Sanci via Elfutils-devel wrote: > Updated ChangeLog > On Wed, Jul 21, 2021 at 3:06 PM Noah Sanci wrote: > > Here are some small fixes for run-debuginfod-find.sh. Looks good. Pushed. Thanks, Mark
Re: [Bug debuginfod/28034] %-escape url characters
Hi Noah, On Wed, 2021-07-21 at 14:18 -0400, Noah Sanci via Elfutils-devel wrote: > > > > +Note: the client should %-escape characters in /SOURCE/FILE that > > > are not shown as "unreserved" in section 2.3 of RFC3986. > > Please read the new note just to ensure that it's reasonable, as my > updated note specifies some characters that will be percent escaped. > Otherwise fixed. Yes, that looks good. As do the alloc error checks and the test cleanups. I'll push the minor fixed version you posted. Thanks, Mark
Re: [Bug debuginfod/28034] %-escape url characters
Hi Noah,
On Wed, 2021-07-21 at 15:44 -0400, Noah Sanci via Elfutils-devel wrote:
> Here is a quick error fix.
Thanks, looks good. Failure results should indeed be negative.
For the record the actual fix was:
diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index 64936acd..26ba1891 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -831,14 +831,14 @@ debuginfod_query_server (debuginfod_client *c,
else
slashbuildid = "/buildid";
- if (filename)/* must start with / */
+ if (filename) /* must start with / */
{
/* PR28034 escape characters in completed url to %hh format. */
char *escaped_string;
escaped_string = curl_easy_escape(data[i].handle, filename, 0);
if (!escaped_string)
{
- rc = ENOMEM;
+ rc = -ENOMEM;
goto out1;
}
snprintf(data[i].url, PATH_MAX, "%s%s/%s/%s%s", server_url,
Pushed this version.
Thanks,
Mark
Buildbot failure in Wildebeest Builder on whole buildset
The Buildbot has detected a new failure on builder elfutils-fedora-x86_64 while building elfutils. Full details are available at: https://builder.wildebeest.org/buildbot/#builders/3/builds/790 Buildbot URL: https://builder.wildebeest.org/buildbot/ Worker for this Build: fedora-x86_64 Build Reason: Blamelist: Noah Sanci BUILD FAILED: failed update (failure) Sincerely, -The BuildbotThe Buildbot has detected a new failure on builder elfutils-debian-i386 while building elfutils. Full details are available at: https://builder.wildebeest.org/buildbot/#builders/4/builds/785 Buildbot URL: https://builder.wildebeest.org/buildbot/ Worker for this Build: debian-i386 Build Reason: Blamelist: Noah Sanci BUILD FAILED: failed update (failure) Sincerely, -The BuildbotThe Buildbot has detected a new failure on builder elfutils-fedora-ppc64le while building elfutils. Full details are available at: https://builder.wildebeest.org/buildbot/#builders/11/builds/740 Buildbot URL: https://builder.wildebeest.org/buildbot/ Worker for this Build: fedora-ppc64le Build Reason: Blamelist: Noah Sanci BUILD FAILED: failed update (failure) Sincerely, -The BuildbotThe Buildbot has detected a new failure on builder elfutils-fedora-ppc64 while building elfutils. Full details are available at: https://builder.wildebeest.org/buildbot/#builders/12/builds/738 Buildbot URL: https://builder.wildebeest.org/buildbot/ Worker for this Build: fedora-ppc64 Build Reason: Blamelist: Noah Sanci BUILD FAILED: failed update (failure) Sincerely, -The BuildbotThe Buildbot has detected a new failure on builder elfutils-debian-arm64 while building elfutils. Full details are available at: https://builder.wildebeest.org/buildbot/#builders/45/builds/221 Buildbot URL: https://builder.wildebeest.org/buildbot/ Worker for this Build: debian-arm64 Build Reason: Blamelist: Noah Sanci BUILD FAILED: failed update (failure) Sincerely, -The Buildbot
Re: Buildbot failure in Wildebeest Builder on whole buildset
On Thu, 2021-07-22 at 14:24 +, build...@builder.wildebeest.org wrote: > BUILD FAILED: failed update (failure) So this was some issue at sourceware, the git update broke for some builders. I have restarted the builds on the buildbot and things seems to be green again (s390x, amd64, i386 are still running though).
Re: [Bug debuginfod/27983] ignore duplicate urls
Hello
Please find the updated solution of PR27983 attached.
Noah
Noah
On Tue, Jul 20, 2021 at 10:42 AM Mark Wielaard wrote:
>
> Hi Noah,
>
> On Mon, Jul 19, 2021 at 09:31:17AM -0400, Noah Sanci wrote:
> > On Wed, Jul 14, 2021 at 12:36 PM Mark Wielaard wrote:
> > > You deduplicate the full URLs after they are fully constructed. Would
> > > it make sense to do the deduplication on server_url, maybe even as
> > > part of the Count number of URLs code? That might make the code
> > > simpler. And you can change num_urls upfront.
> > Deduplication before fully building the URL would work well, however I
> > was concerned about the slashbuildid situation. I would need to alter
> > all urls to either have a trailing '/' or no trailing '/' to ensure
> > comparison between 'http://127.0.0.1:8000/' and 'http://127.0.0.1:8000'
> > is considered equal. This is possible, but I ultimately decided to
> > wait until full construction as those issues would have been handled.
> > I would be glad to make the change if you want.
>
> I see what you mean, we have:
>
> /* Build handle url. Tolerate both http://foo:999 and
> http://foo:999/ forms */
> char *slashbuildid;
> if (strlen(server_url) > 1 && server_url[strlen(server_url)-1] == '/')
> slashbuildid = "buildid";
> else
> slashbuildid = "/buildid";
>
> I think the code could become simpler if we did the deduplication of
> the server_url list up-front. That also gives you the oppertunity to
> make sure all server_urls end with a slash. But if you think that is
> too much work for this patch we can do it as a followup patch. You
> already included a test, which makes checking such a refactoring
> easier (sadly the run-debuginfod-find.sh test is somewhat fragile).
>
> > From be4e07a8732dd688595b99f92ba275ef5060a34d Mon Sep 17 00:00:00 2001
> > From: Noah Sanci
> > Date: Fri, 9 Jul 2021 14:53:10 -0400
> > Subject: [PATCH] debuginfod: PR27983 - ignore duplicate urls
> >
> > Gazing at server logs, one sees a minority of clients who appear to have
> > duplicate query traffic coming in: the same URL, milliseconds apart.
> > Chances are the user accidentally doubled her $DEBUGINFOD_URLS somehow,
> > and the client library is dutifully asking the servers TWICE. Bug #27863
> > reduces the pain on the servers' CPU, but dupe network traffic is still
> > being paid. We should reject sending outright duplicate concurrent
> > traffic.
> >
> > https://sourceware.org/bugzilla/show_bug.cgi?id=27983
>
>
> > diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
> > index f417b40a..a9447cae 100644
> > --- a/debuginfod/debuginfod-client.c
> > +++ b/debuginfod/debuginfod-client.c
> > @@ -795,6 +795,7 @@ debuginfod_query_server (debuginfod_client *c,
> >
> >char *strtok_saveptr;
> >char *server_url = strtok_r(server_urls, url_delim, &strtok_saveptr);
> > + int unduplicated_urls = 0;
> >
> >/* Initialize each handle. */
> >for (int i = 0; i < num_urls && server_url != NULL; i++)
>
> Maybe this should be:
>
> /* Initialize each handle. */
>int i = 0;
>while (server_url != NULL)
>
> With an i++ at the end after the data[i] handle has been completely assigned.
> See below (*) for why.
>
> > + char *tmp_url = calloc(PATH_MAX, sizeof(char));
>
> This can fail. So you'll have to handle tmp_url == NULL. Otherwise
> snprintf will crash.
>
> >if (filename) /* must start with / */
> > -snprintf(data[i].url, PATH_MAX, "%s%s/%s/%s%s", server_url,
> > +snprintf(tmp_url, PATH_MAX, "%s%s/%s/%s%s", server_url,
> > slashbuildid, build_id_bytes, type, filename);
> >else
> > -snprintf(data[i].url, PATH_MAX, "%s%s/%s/%s", server_url,
> > +snprintf(tmp_url, PATH_MAX, "%s%s/%s/%s", server_url,
> > slashbuildid, build_id_bytes, type);
> >
> > + /* PR 27983: If the url is already set to be used use, skip it */
> > + int url_index = -1;
> > + for (url_index = 0; url_index < i; ++url_index)
>
> No need to initialize url_index twice. Just declar int url_index; it
> will be assigned 0 at the next line.
>
> > +{
> > + if(strcmp(tmp_url, data[url_index].url) == 0)
> > +{
> > + url_index = -1;
> > + break;
> > +}
> > +}
> > + if (url_index == -1)
> > +{
> > + if (vfd >= 0)
> > +dprintf(vfd, "duplicate url: %s, skipping\n", tmp_url);
> > + free(tmp_url);
> > + // Ensure that next iteration doesn't skip over an index
> > mid-array
> > + i--;
> > + server_url = strtok_r(NULL, url_delim, &strtok_saveptr);
> > + continue;
>
> (*) this i--; confused me a little. Which is why I suggest turning
> that for loop into a while loop.
>
> > +}
> > + else
> > +{
> > + unduplicated_urls++;
> > + strncpy(data[i].url, tmp_url, PATH_MAX);
>
> Bot
Buildbot failure in Wildebeest Builder on whole buildset
The Buildbot has detected a new failure on builder elfutils-debian-amd64 while building elfutils. Full details are available at: https://builder.wildebeest.org/buildbot/#builders/2/builds/786 Buildbot URL: https://builder.wildebeest.org/buildbot/ Worker for this Build: debian-amd64 Build Reason: Blamelist: Noah Sanci BUILD FAILED: failed update (failure) Sincerely, -The Buildbot
Re: [Bug debuginfod/27983] ignore duplicate urls
Hello,
PR27983 improvements attached
Noah
Noah
On Thu, Jul 22, 2021 at 12:25 PM Noah Sanci wrote:
>
> Hello
>
> Please find the updated solution of PR27983 attached.
>
> Noah
>
>
> Noah
>
> On Tue, Jul 20, 2021 at 10:42 AM Mark Wielaard wrote:
> >
> > Hi Noah,
> >
> > On Mon, Jul 19, 2021 at 09:31:17AM -0400, Noah Sanci wrote:
> > > On Wed, Jul 14, 2021 at 12:36 PM Mark Wielaard wrote:
> > > > You deduplicate the full URLs after they are fully constructed. Would
> > > > it make sense to do the deduplication on server_url, maybe even as
> > > > part of the Count number of URLs code? That might make the code
> > > > simpler. And you can change num_urls upfront.
> > > Deduplication before fully building the URL would work well, however I
> > > was concerned about the slashbuildid situation. I would need to alter
> > > all urls to either have a trailing '/' or no trailing '/' to ensure
> > > comparison between 'http://127.0.0.1:8000/' and 'http://127.0.0.1:8000'
> > > is considered equal. This is possible, but I ultimately decided to
> > > wait until full construction as those issues would have been handled.
> > > I would be glad to make the change if you want.
> >
> > I see what you mean, we have:
> >
> > /* Build handle url. Tolerate both http://foo:999 and
> > http://foo:999/ forms */
> > char *slashbuildid;
> > if (strlen(server_url) > 1 && server_url[strlen(server_url)-1] == '/')
> > slashbuildid = "buildid";
> > else
> > slashbuildid = "/buildid";
> >
> > I think the code could become simpler if we did the deduplication of
> > the server_url list up-front. That also gives you the oppertunity to
> > make sure all server_urls end with a slash. But if you think that is
> > too much work for this patch we can do it as a followup patch. You
> > already included a test, which makes checking such a refactoring
> > easier (sadly the run-debuginfod-find.sh test is somewhat fragile).
> >
> > > From be4e07a8732dd688595b99f92ba275ef5060a34d Mon Sep 17 00:00:00 2001
> > > From: Noah Sanci
> > > Date: Fri, 9 Jul 2021 14:53:10 -0400
> > > Subject: [PATCH] debuginfod: PR27983 - ignore duplicate urls
> > >
> > > Gazing at server logs, one sees a minority of clients who appear to have
> > > duplicate query traffic coming in: the same URL, milliseconds apart.
> > > Chances are the user accidentally doubled her $DEBUGINFOD_URLS somehow,
> > > and the client library is dutifully asking the servers TWICE. Bug #27863
> > > reduces the pain on the servers' CPU, but dupe network traffic is still
> > > being paid. We should reject sending outright duplicate concurrent
> > > traffic.
> > >
> > > https://sourceware.org/bugzilla/show_bug.cgi?id=27983
> >
> >
> > > diff --git a/debuginfod/debuginfod-client.c
> > > b/debuginfod/debuginfod-client.c
> > > index f417b40a..a9447cae 100644
> > > --- a/debuginfod/debuginfod-client.c
> > > +++ b/debuginfod/debuginfod-client.c
> > > @@ -795,6 +795,7 @@ debuginfod_query_server (debuginfod_client *c,
> > >
> > >char *strtok_saveptr;
> > >char *server_url = strtok_r(server_urls, url_delim, &strtok_saveptr);
> > > + int unduplicated_urls = 0;
> > >
> > >/* Initialize each handle. */
> > >for (int i = 0; i < num_urls && server_url != NULL; i++)
> >
> > Maybe this should be:
> >
> > /* Initialize each handle. */
> >int i = 0;
> >while (server_url != NULL)
> >
> > With an i++ at the end after the data[i] handle has been completely
> > assigned.
> > See below (*) for why.
> >
> > > + char *tmp_url = calloc(PATH_MAX, sizeof(char));
> >
> > This can fail. So you'll have to handle tmp_url == NULL. Otherwise
> > snprintf will crash.
> >
> > >if (filename) /* must start with / */
> > > -snprintf(data[i].url, PATH_MAX, "%s%s/%s/%s%s", server_url,
> > > +snprintf(tmp_url, PATH_MAX, "%s%s/%s/%s%s", server_url,
> > > slashbuildid, build_id_bytes, type, filename);
> > >else
> > > -snprintf(data[i].url, PATH_MAX, "%s%s/%s/%s", server_url,
> > > +snprintf(tmp_url, PATH_MAX, "%s%s/%s/%s", server_url,
> > > slashbuildid, build_id_bytes, type);
> > >
> > > + /* PR 27983: If the url is already set to be used use, skip it */
> > > + int url_index = -1;
> > > + for (url_index = 0; url_index < i; ++url_index)
> >
> > No need to initialize url_index twice. Just declar int url_index; it
> > will be assigned 0 at the next line.
> >
> > > +{
> > > + if(strcmp(tmp_url, data[url_index].url) == 0)
> > > +{
> > > + url_index = -1;
> > > + break;
> > > +}
> > > +}
> > > + if (url_index == -1)
> > > +{
> > > + if (vfd >= 0)
> > > +dprintf(vfd, "duplicate url: %s, skipping\n", tmp_url);
> > > + free(tmp_url);
> > > + // Ensure that next iteration doesn't skip over an index
> > > mid-array
> > > + i--;
>
