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 <amerey@redhat.com>
---
 debuginfod/debuginfod-client.c | 83 ++++++++++++++++++----------------
 1 file changed, 43 insertions(+), 40 deletions(-)
  

Hi Aaron,

On Tue, 2022-01-11 at 22:07 -0500, Aaron Merey via Elfutils-devel
wrote:
> 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.

Sorry this patch wasn't reviewed for such a long time. It looks correct
and is also a nice cleanup. Consolidating getting the download size in
one place instead of two (for the progress function and the maxsize
check).

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?

When using sscanf why are you using a double and %lf? Isn't it simpler
to use a long and %ld?

Is there a way to test this easily? When would Content-Length not be
available?

Cheers,

Mark
  

On Thu, Mar 31, 2022 at 1:44 PM Mark Wielaard <mark@klomp.org> 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
  

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 <amerey@redhat.com>
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 <amerey@redhat.com>
---
 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)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(vfd, "Content-Length too large.\n");
-                rc = -EFBIG;
-                goto out2;
-              }
+          if (vfd >=0)
+            dprintf(vfd, "Content-Length too large.\n");
+          rc = -EFBIG;
+          goto out2;
         }
     } while (still_running);
  

Hi Aaron,

On Fri, Apr 22, 2022 at 06:56:41PM -0400, Aaron Merey via Elfutils-devel wrote:
> 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 <amerey@redhat.com>
> 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.

Looks good. Pleas commit.

Cheers,

Mark
  

On Sun, Apr 24, 2022 at 11:05 AM Mark Wielaard <mark@klomp.org> wrote:
> Looks good. Pleas commit.

Thanks, pushed as:

commit 55fee962676fbff60c6b0469305bcb077910d64f
Author: Aaron Merey <amerey@redhat.com>
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 <amerey@redhat.com>