[PATCH] debuginfod-client.c: Fix download size not correctly fallbacks to x-debuginfod-size header

[PATCH] debuginfod-client.c: Fix download size not correctly fallbacks to x-debuginfod-size header

[lilydjwg]

From: lilydjwg <lilydjwg@gmail.com>

Currently when the debuginfod server doesn't set Content-Length but set
x-debuginfod-size header, it's ignored and the progressfn doesn't know
the total size.

This happens for me with Arch Linux's debuginfod server and gdb.

The reason is that when Content-Length is unavailable, cl is set to -1
by curl but dl_size remains as 0, but later dl_size == -1 is checked.

Signed-off-by: lilydjwg <lilydjwg@gmail.com>
---
 ChangeLog                      | 5 +++++
 debuginfod/debuginfod-client.c | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/ChangeLog b/ChangeLog
index 10c23002..05697a02 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2023-03-29  lilydjwg  <lilydjwg@gmail.com>
+
+	* debuginfod/debuginfod-client.c: Fix download size not correctly
+	fallbacks to x-debuginfod-size header
+
 2023-03-03  Mark Wielaard  <mark@klomp.org>
 
 	* NEWS: Add ELFCOMPRESS_ZSTD support for libelf and elfcompress.
diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index b33408eb..e494adec 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -1479,7 +1479,7 @@ debuginfod_query_server (debuginfod_client *c,
           curl_res = curl_easy_getinfo(target_handle,
                                        CURLINFO_CONTENT_LENGTH_DOWNLOAD_T,
                                        &cl);
-          if (curl_res == CURLE_OK && cl >= 0)
+          if (curl_res == CURLE_OK)
             dl_size = (cl > LONG_MAX ? LONG_MAX : (long)cl);
 #else
           double cl;
-- 
2.40.0

Re: [PATCH] debuginfod-client.c: Fix download size not correctly fallbacks to x-debuginfod-size header

[Frank Ch. Eigler]

Hi -

> [...]
> The reason is that when Content-Length is unavailable, cl is set to -1
> by curl

Is that behaviour from new versions of curl?

>  but dl_size remains as 0, but later dl_size == -1 is checked.

Or perhaps dl_size needs to be initialized to -1 ("unknown") vs 0
("known to be zero"), and that value mapped to 0 only in the
*c->progressfn() call down below.


- FChE

Re: [PATCH] debuginfod-client.c: Fix download size not correctly fallbacks to x-debuginfod-size header

[lilydjwg]

On Wed, Mar 29, 2023 at 08:28:35AM -0400, Frank Ch. Eigler wrote:
> Hi -
> 
> > [...]
> > The reason is that when Content-Length is unavailable, cl is set to -1
> > by curl
> 
> Is that behaviour from new versions of curl?

Yes. curl 8.0.1 to be exact.

> >  but dl_size remains as 0, but later dl_size == -1 is checked.
> 
> Or perhaps dl_size needs to be initialized to -1 ("unknown") vs 0
> ("known to be zero"), and that value mapped to 0 only in the
> *c->progressfn() call down below.

Yes, that can be done. Just more places to be changed.

However, I find a bigger issue just now: the CURLINFO_SIZE_DOWNLOAD_T
counts differently than x-debuginfod-size. I'm sending a v2 patch now.

-- 
Best regards,
lilydjwg

[PATCH v2 1/2] debuginfod-client.c: Fix download size not correctly fallbacks to x-debuginfod-size header

[lilydjwg]

Signed-off-by: lilydjwg <lilydjwg@gmail.com>
---
 ChangeLog                      | 5 +++++
 debuginfod/debuginfod-client.c | 6 +++---
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 10c23002..05697a02 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2023-03-29  lilydjwg  <lilydjwg@gmail.com>
+
+	* debuginfod/debuginfod-client.c: Fix download size not correctly
+	fallbacks to x-debuginfod-size header
+
 2023-03-03  Mark Wielaard  <mark@klomp.org>
 
 	* NEWS: Add ELFCOMPRESS_ZSTD support for libelf and elfcompress.
diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index b33408eb..d6d3f0dd 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -1467,7 +1467,7 @@ debuginfod_query_server (debuginfod_client *c,
           goto out2;
         }
 
-      long dl_size = 0;
+      long dl_size = -1;
       if (target_handle && (c->progressfn || maxsize > 0))
         {
           /* Get size of file being downloaded. NB: If going through
@@ -1486,7 +1486,7 @@ debuginfod_query_server (debuginfod_client *c,
           curl_res = curl_easy_getinfo(target_handle,
                                        CURLINFO_CONTENT_LENGTH_DOWNLOAD,
                                        &cl);
-          if (curl_res == CURLE_OK)
+          if (curl_res == CURLE_OK && cl >= 0)
             dl_size = (cl >= (double)(LONG_MAX+1UL) ? LONG_MAX : (long)cl);
 #endif
           /* If Content-Length is -1, try to get the size from
@@ -1527,7 +1527,7 @@ debuginfod_query_server (debuginfod_client *c,
 
             }
 
-          if ((*c->progressfn) (c, pa, dl_size))
+          if ((*c->progressfn) (c, pa, dl_size == -1 ? 0 : dl_size))
 	    {
 	      c->progressfn_cancel = true;
               break;
-- 
2.40.0

[PATCH v2 2/2] debuginfod-client.c: Fix x-debuginfod-size counts differently than CURLINFO_SIZE_DOWNLOAD_T

[lilydjwg]

x-debuginfod-size is the actual file size, but CURLINFO_SIZE_DOWNLOAD_T
is transferred size, i.e. the gzipped one if gzip is on.

Let's count written data and use that if and only if x-debuginfod-size
is used.

Signed-off-by: lilydjwg <lilydjwg@gmail.com>
---
 ChangeLog                      |  2 ++
 debuginfod/debuginfod-client.c | 45 ++++++++++++++++++++++------------
 2 files changed, 32 insertions(+), 15 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 05697a02..903b3494 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -2,6 +2,8 @@
 
 	* debuginfod/debuginfod-client.c: Fix download size not correctly
 	fallbacks to x-debuginfod-size header
+	* debuginfod-client.c: Fix x-debuginfod-size counts differently than
+	CURLINFO_SIZE_DOWNLOAD_T
 
 2023-03-03  Mark Wielaard  <mark@klomp.org>
 
diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index d6d3f0dd..ebc8e6b2 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -215,6 +215,9 @@ struct handle_data
   /* Response http headers for this client handle, sent from the server */
   char *response_data;
   size_t response_data_size;
+
+  /* The data size that has been written */
+  long written_size;
 };
 
 static size_t
@@ -243,6 +246,7 @@ debuginfod_write_callback (char *ptr, size_t size, size_t nmemb, void *data)
   if (*d->target_handle != d->handle)
     return -1;
 
+  d->written_size += count;
   return (size_t) write(d->fd, (void*)ptr, count);
 }
 
@@ -1265,6 +1269,7 @@ debuginfod_query_server (debuginfod_client *c,
       data[i].errbuf[0] = '\0';
       data[i].response_data = NULL;
       data[i].response_data_size = 0;
+      data[i].written_size = 0;
     }
 
   char *escaped_string = NULL;
@@ -1468,6 +1473,7 @@ debuginfod_query_server (debuginfod_client *c,
         }
 
       long dl_size = -1;
+      bool size_compressed = true;
       if (target_handle && (c->progressfn || maxsize > 0))
         {
           /* Get size of file being downloaded. NB: If going through
@@ -1498,7 +1504,10 @@ debuginfod_query_server (debuginfod_client *c,
 
               if (hdr != NULL
                   && sscanf(hdr, "x-debuginfod-size: %ld", &xdl) == 1)
-                dl_size = xdl;
+                {
+                  dl_size = xdl;
+                  size_compressed = false;
+                }
             }
         }
 
@@ -1508,23 +1517,29 @@ debuginfod_query_server (debuginfod_client *c,
           long pa = loops; /* default param for progress callback */
           if (target_handle) /* we've committed to a server; report its download progress */
             {
-              CURLcode curl_res;
+              if (size_compressed)
+                {
+                  CURLcode curl_res;
 #if CURL_AT_LEAST_VERSION(7, 55, 0)
-              curl_off_t dl;
-              curl_res = curl_easy_getinfo(target_handle,
-                                           CURLINFO_SIZE_DOWNLOAD_T,
-                                           &dl);
-              if (curl_res == 0 && dl >= 0)
-                pa = (dl > LONG_MAX ? LONG_MAX : (long)dl);
+                  curl_off_t dl;
+                  curl_res = curl_easy_getinfo(target_handle,
+                                              CURLINFO_SIZE_DOWNLOAD_T,
+                                              &dl);
+                  if (curl_res == 0 && dl >= 0)
+                    pa = (dl > LONG_MAX ? LONG_MAX : (long)dl);
 #else
-              double dl;
-              curl_res = curl_easy_getinfo(target_handle,
-                                           CURLINFO_SIZE_DOWNLOAD,
-                                           &dl);
-              if (curl_res == 0)
-                pa = (dl >= (double)(LONG_MAX+1UL) ? LONG_MAX : (long)dl);
+                  double dl;
+                  curl_res = curl_easy_getinfo(target_handle,
+                                              CURLINFO_SIZE_DOWNLOAD,
+                                              &dl);
+                  if (curl_res == 0)
+                    pa = (dl >= (double)(LONG_MAX+1UL) ? LONG_MAX : (long)dl);
 #endif
-
+                }
+              else
+                {
+                  pa = data[committed_to].written_size;
+                }
             }
 
           if ((*c->progressfn) (c, pa, dl_size == -1 ? 0 : dl_size))
-- 
2.40.0

Re: [PATCH] debuginfod-client.c: Fix download size not correctly fallbacks to x-debuginfod-size header

[lilydjwg]

On Wed, Mar 29, 2023 at 10:57:47PM +0800, lilydjwg wrote:
> On Wed, Mar 29, 2023 at 08:28:35AM -0400, Frank Ch. Eigler wrote:
> > Hi -
> > 
> > > [...]
> > > The reason is that when Content-Length is unavailable, cl is set to -1
> > > by curl
> > 
> > Is that behaviour from new versions of curl?
> 
> Yes. curl 8.0.1 to be exact.

To be clear, I mean that I'm having the issue when using curl 8.0.1, not
the value is set to -1. The latter has been like that even in the
"#else" branch which is for older curl.

-- 
Best regards,
lilydjwg

Re: [PATCH v2 2/2] debuginfod-client.c: Fix x-debuginfod-size counts differently than CURLINFO_SIZE_DOWNLOAD_T

[Frank Ch. Eigler]

Hi -

> x-debuginfod-size is the actual file size, but CURLINFO_SIZE_DOWNLOAD_T
> is transferred size, i.e. the gzipped one if gzip is on.
> Let's count written data and use that if and only if x-debuginfod-size
> is used.

Hey, great idea actually tallying up writes in the callback function.
(We need to take care to clear that counter, in case of client object
reuse.)  Also, can you think of some reason not to just use that value
at all times, i.e., without any of that "if and only if ..." business?

- FChE

Re: [PATCH v2 1/2] debuginfod-client.c: Fix download size not correctly fallbacks to x-debuginfod-size header

[Frank Ch. Eigler]

> +2023-03-29  lilydjwg  <lilydjwg@gmail.com>
> +
> +	* debuginfod/debuginfod-client.c: Fix download size not correctly
> +	fallbacks to x-debuginfod-size header

Thanks, merged.

- FChE

Re: [PATCH v2 2/2] debuginfod-client.c: Fix x-debuginfod-size counts differently than CURLINFO_SIZE_DOWNLOAD_T

[lilydjwg]

On Wed, Mar 29, 2023 at 03:14:43PM -0400, Frank Ch. Eigler wrote:
> Hi -
> 
> > x-debuginfod-size is the actual file size, but CURLINFO_SIZE_DOWNLOAD_T
> > is transferred size, i.e. the gzipped one if gzip is on.
> > Let's count written data and use that if and only if x-debuginfod-size
> > is used.
> 
> Hey, great idea actually tallying up writes in the callback function.
> (We need to take care to clear that counter, in case of client object
> reuse.)  Also, can you think of some reason not to just use that value
> at all times, i.e., without any of that "if and only if ..." business?

The counter is in handle_data, and it is already cleared at
"query_in_parallel:". I don't find other places that may reuse them.

The written_size is actual file size (uncompressed), but IIUC
Content-Length is the compressed size if Content-Encoding says the
content is compressed. I haven't seen any compressed responses with
Content-Length, but from the spec I don't read they are not allowed.

-- 
Best regards,
lilydjwg

Re: [PATCH v2 2/2] debuginfod-client.c: Fix x-debuginfod-size counts differently than CURLINFO_SIZE_DOWNLOAD_T

[Frank Ch. Eigler]

Hi -

> > Hey, great idea actually tallying up writes in the callback function.
> > (We need to take care to clear that counter, in case of client object
> > reuse.)  Also, can you think of some reason not to just use that value
> > at all times, i.e., without any of that "if and only if ..." business?
> 
> The counter is in handle_data, and it is already cleared at
> "query_in_parallel:". I don't find other places that may reuse them.

OK.

> The written_size is actual file size (uncompressed), but IIUC
> Content-Length is the compressed size if Content-Encoding says the
> content is compressed. I haven't seen any compressed responses with
> Content-Length, but from the spec I don't read they are not allowed.

OK, so to spell out the hypothetical problem: what if a httpd server
does send back a Content-Length: response header for a compressed
file, and we use that as the denominator for progress reporting.  If
we use the decompressed actual file length as numerator, we'd go over
100%.

Then ISTM a simpler way to handle this would be to say that if the
x-debuginfod-size: response header is found (as denominator), then go
ahead and use the actual data[committed_to].written_size (as
numerator).  Don't even try the CURLINFO_SIZE* queries then.

- FChE

Re: [PATCH v2 2/2] debuginfod-client.c: Fix x-debuginfod-size counts differently than CURLINFO_SIZE_DOWNLOAD_T

[lilydjwg]

On Thu, Mar 30, 2023 at 01:24:13PM -0400, Frank Ch. Eigler wrote:
> > The written_size is actual file size (uncompressed), but IIUC
> > Content-Length is the compressed size if Content-Encoding says the
> > content is compressed. I haven't seen any compressed responses with
> > Content-Length, but from the spec I don't read they are not allowed.
> 
> OK, so to spell out the hypothetical problem: what if a httpd server
> does send back a Content-Length: response header for a compressed
> file, and we use that as the denominator for progress reporting.  If
> we use the decompressed actual file length as numerator, we'd go over
> 100%.
> 
> Then ISTM a simpler way to handle this would be to say that if the
> x-debuginfod-size: response header is found (as denominator), then go
> ahead and use the actual data[committed_to].written_size (as
> numerator).  Don't even try the CURLINFO_SIZE* queries then.

It's not tried in that case. size_compressed indicates where the total
size is from and those CURLINFO_SIZE* is skipped. Maybe I should rename
the variable to something else (it's not always compressed).

Or do you mean that you want to always use written_size even when the
progress may go beyond 100%?

-- 
Best regards,
lilydjwg

Re: [PATCH v2 2/2] debuginfod-client.c: Fix x-debuginfod-size counts differently than CURLINFO_SIZE_DOWNLOAD_T

[Mark Wielaard]

Hi,

On Fri, 2023-03-31 at 12:50 +0800, lilydjwg via Elfutils-devel wrote:
> On Thu, Mar 30, 2023 at 01:24:13PM -0400, Frank Ch. Eigler wrote:
> > > The written_size is actual file size (uncompressed), but IIUC
> > > Content-Length is the compressed size if Content-Encoding says the
> > > content is compressed. I haven't seen any compressed responses with
> > > Content-Length, but from the spec I don't read they are not allowed.
> > 
> > OK, so to spell out the hypothetical problem: what if a httpd server
> > does send back a Content-Length: response header for a compressed
> > file, and we use that as the denominator for progress reporting.  If
> > we use the decompressed actual file length as numerator, we'd go over
> > 100%.
> > 
> > Then ISTM a simpler way to handle this would be to say that if the
> > x-debuginfod-size: response header is found (as denominator), then go
> > ahead and use the actual data[committed_to].written_size (as
> > numerator).  Don't even try the CURLINFO_SIZE* queries then.
> 
> It's not tried in that case. size_compressed indicates where the total
> size is from and those CURLINFO_SIZE* is skipped. Maybe I should rename
> the variable to something else (it's not always compressed).
> 
> Or do you mean that you want to always use written_size even when the
> progress may go beyond 100%?

What is the status of this patch/discussion?

Thanks,

Mark

PR30809, was Re: [PATCH v2 2/2] debuginfod-client.c: Fix x-debuginfod-size counts differently than CURLINFO_SIZE_DOWNLOAD_T

[Frank Ch. Eigler]

Hi -

> What is the status of this patch/discussion?

Ummmm forgot about it.  But that's OK, filed PR30809, and wrote &
tested this little patch:

commit 3ef3fab0d64c89a52dd6e2ce0d01dd5e713d7b5a
Author: Frank Ch. Eigler <fche@redhat.com>
Date:   Tue Aug 29 14:08:04 2023 -0400

    PR30809: improve debuginfod client progress-callback parameters
    
    * debuginfod-client.c (debuginfod_query_server): Use fstat(3)
      of the file handle being downloaded into as the preferred
      source of download progress.
    
    Tested by hand, as the testsuite doesn't have enough machinery to
    simulate compressed vs. uncompressed service.  Hand testing with
    (unmodified) fedora-38 gdb and debuginfod-find shows dramatically
    improved progress displays: all have quantitative figures when
    fetching from real (unmodified) upstream servers.
    
    Signed-off-by: Frank Ch. Eigler <fche@redhat.com>

diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index d92d8d62c982..6882cb190d3c 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -1533,6 +1533,20 @@ debuginfod_query_server (debuginfod_client *c,
           long pa = loops; /* default param for progress callback */
           if (target_handle) /* we've committed to a server; report its download progress */
             {
+              /* PR30809: Check actual size of cached file.  This same
+                 fd is shared by all the multi-curl handles (but only
+                 one will end up writing to it).  Another way could be
+                 to tabulate totals in debuginfod_write_callback(). */
+              struct stat cached;
+              int statrc = fstat(fd, &cached);
+              if (statrc == 0)
+                pa = (long) cached.st_size;
+              else
+                {
+                  /* Otherwise, query libcurl for its tabulated total.
+                     However, that counts http body length, not
+                     decoded/decompressed content length, so does not
+                     measure quite the same thing as dl. */
                   CURLcode curl_res;
 #if CURL_AT_LEAST_VERSION(7, 55, 0)
                   curl_off_t dl;
@@ -1549,7 +1563,7 @@ debuginfod_query_server (debuginfod_client *c,
                   if (curl_res == 0)
                     pa = (dl >= (double)(LONG_MAX+1UL) ? LONG_MAX : (long)dl);
 #endif
-
+                }
             }
 
           if ((*c->progressfn) (c, pa, dl_size == -1 ? 0 : dl_size))

Re: PR30809, was Re: [PATCH v2 2/2] debuginfod-client.c: Fix x-debuginfod-size counts differently than CURLINFO_SIZE_DOWNLOAD_T

[Mark Wielaard]

Hi Frank,

On Tue, Aug 29, 2023 at 02:33:07PM -0400, Frank Ch. Eigler wrote:
> commit 3ef3fab0d64c89a52dd6e2ce0d01dd5e713d7b5a
> Author: Frank Ch. Eigler <fche@redhat.com>
> Date:   Tue Aug 29 14:08:04 2023 -0400
> 
>     PR30809: improve debuginfod client progress-callback parameters
>     
>     * debuginfod-client.c (debuginfod_query_server): Use fstat(3)
>       of the file handle being downloaded into as the preferred
>       source of download progress.
>     
>     Tested by hand, as the testsuite doesn't have enough machinery to
>     simulate compressed vs. uncompressed service.  Hand testing with
>     (unmodified) fedora-38 gdb and debuginfod-find shows dramatically
>     improved progress displays: all have quantitative figures when
>     fetching from real (unmodified) upstream servers.
>     
>     Signed-off-by: Frank Ch. Eigler <fche@redhat.com>

Very nice and simple. Looks good.

Thanks,

Mark