PR25369 rfc slice 2: debuginfod get_url

PR25369 rfc slice 2: debuginfod get_url

[Frank Ch. Eigler]

Hi -

Slice 2/3, the debuginfod_client get_url function.  This new
version works during or after the progressfn callback.

Author: Frank Ch. Eigler <fche@redhat.com>
Date:   Fri Mar 20 21:33:52 2020 -0400

    debuginfod client API: add get_url function
    
    This function lets a client know, during or after a progressfn
    callback, what the url of the winning outgoing download is/was.
    
    Signed-off-by: Frank Ch. Eigler <fche@redhat.com>

diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index 2730dbf7aee5..2a1c267b712c 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -82,6 +82,9 @@ struct debuginfod_client
   /* Stores user data. */
   void* user_data;
 
+  /* Stores current/last url. */
+  char url[PATH_MAX];
+
   /* Can contain all other context, like cache_path, server_urls,
      timeout or other info gotten from environment variables, the
      handle data, etc. So those don't have to be reparsed and
@@ -128,6 +131,9 @@ struct handle_data
   /* This handle.  */
   CURL *handle;
 
+  /* The client object whom we're serving. */
+  debuginfod_client *client;
+
   /* Pointer to handle that should write to fd. Initially points to NULL,
      then points to the first handle that begins writing the target file
      to the cache. Used to ensure that a file is not downloaded from
@@ -144,7 +150,11 @@ debuginfod_write_callback (char *ptr, size_t size, size_t nmemb, void *data)
 
   /* Indicate to other handles that they can abort their transfer.  */
   if (*d->target_handle == NULL)
-    *d->target_handle = d->handle;
+    {
+      *d->target_handle = d->handle;
+      /* update the client object */
+      strncpy(d->client->url, d->url, sizeof(d->client->url));
+    }
 
   /* If this handle isn't the target handle, abort transfer.  */
   if (*d->target_handle != d->handle)
@@ -410,6 +420,9 @@ debuginfod_query_server (debuginfod_client *c,
   char build_id_bytes[MAX_BUILD_ID_BYTES * 2 + 1];
   int rc;
 
+  /* Clear the saved url. */
+  c->url[0] = '\0';
+
   /* Is there any server we can query?  If not, don't do any work,
      just return with ENOSYS.  Don't even access the cache.  */
   urls_envvar = getenv(server_urls_envvar);
@@ -620,6 +633,7 @@ debuginfod_query_server (debuginfod_client *c,
       data[i].fd = fd;
       data[i].target_handle = &target_handle;
       data[i].handle = curl_easy_init();
+      data[i].client = c;
 
       if (data[i].handle == NULL)
         {
@@ -877,12 +891,20 @@ debuginfod_query_server (debuginfod_client *c,
 static int
 default_progressfn (debuginfod_client *c, long a, long b)
 {
-  (void) c;
-
-  dprintf(STDERR_FILENO,
-          "Downloading from debuginfod %ld/%ld%s", a, b,
-          ((a == b) ? "\n" : "\r"));
-  /* XXX: include URL - stateful */
+  const char* url = debuginfod_get_url (c);
+  assert (url != NULL); /* undocumented guarantee, might change later */
+
+  if (b == 0 || url[0]=='\0') /* early stage */
+    dprintf(STDERR_FILENO,
+            "Downloading %c\r", "-/|\\"[a % 4]);
+  else if (b < 0) /* download in progress but unknown total length */
+    dprintf(STDERR_FILENO,
+            "Downloading %s %ld\r",
+            url, a);
+  else /* download in progress, and known total length */
+    dprintf(STDERR_FILENO,
+            "Downloading %s %ld/%ld\r",
+            url, a, b);
 
   return 0;
 }
@@ -894,13 +916,11 @@ debuginfod_begin (void)
 {
   debuginfod_client *client;
   size_t size = sizeof (struct debuginfod_client);
-  client = (debuginfod_client *) malloc (size);
+  client = (debuginfod_client *) calloc (1, size);
   if (client != NULL)
     {
       if (getenv(DEBUGINFOD_PROGRESS_ENV_VAR))
 	client->progressfn = default_progressfn;
-      else
-	client->progressfn = NULL;
     }
   return client;
 }
@@ -918,6 +938,12 @@ debuginfod_get_user_data(debuginfod_client *client)
   return client->user_data;
 }
 
+const char *
+debuginfod_get_url(debuginfod_client *client)
+{
+  return client->url;
+}
+
 void
 debuginfod_end (debuginfod_client *client)
 {
diff --git a/debuginfod/debuginfod.h b/debuginfod/debuginfod.h
index fe72f16e9bd8..2ace5f350488 100644
--- a/debuginfod/debuginfod.h
+++ b/debuginfod/debuginfod.h
@@ -81,6 +81,9 @@ void debuginfod_set_user_data (debuginfod_client *client, void *value);
 /* Get the user parameter.  */
 void* debuginfod_get_user_data (debuginfod_client *client);
 
+/* Get the current or last active URL, if known.  */
+const char* debuginfod_get_url (debuginfod_client *client);
+
 /* Release debuginfod client connection context handle.  */
 void debuginfod_end (debuginfod_client *client);
 
diff --git a/debuginfod/libdebuginfod.map b/debuginfod/libdebuginfod.map
index a919630dacce..d84e8924f7f8 100644
--- a/debuginfod/libdebuginfod.map
+++ b/debuginfod/libdebuginfod.map
@@ -12,4 +12,5 @@ ELFUTILS_0.179 {
   global:
   debuginfod_set_user_data;
   debuginfod_get_user_data;
+  debuginfod_get_url;
 };
diff --git a/doc/Makefile.am b/doc/Makefile.am
index 87d1fee03302..38b8226d775c 100644
--- a/doc/Makefile.am
+++ b/doc/Makefile.am
@@ -30,6 +30,7 @@ notrans_dist_man3_MANS += debuginfod_find_debuginfo.3
 notrans_dist_man3_MANS += debuginfod_find_executable.3
 notrans_dist_man3_MANS += debuginfod_find_source.3
 notrans_dist_man3_MANS += debuginfod_get_user_data.3
+notrans_dist_man3_MANS += debuginfod_get_url.3
 notrans_dist_man3_MANS += debuginfod_set_progressfn.3
 notrans_dist_man3_MANS += debuginfod_set_user_data.3
 notrans_dist_man1_MANS += debuginfod-find.1
diff --git a/doc/debuginfod_find_debuginfo.3 b/doc/debuginfod_find_debuginfo.3
index d975927f757b..7a3838d23292 100644
--- a/doc/debuginfod_find_debuginfo.3
+++ b/doc/debuginfod_find_debuginfo.3
@@ -53,6 +53,7 @@ OPTIONAL FUNCTIONS
 .BI "void debuginfod_set_user_data(debuginfod_client *" client ","
 .BI "                              void *" data ");"
 .BI "void* debuginfod_get_user_data(debuginfod_client *" client ");"
+.BI "const char* debuginfod_get_url(debuginfod_client *" client ");"
 
 .SH DESCRIPTION
 
@@ -148,6 +149,16 @@ and retrieved via
 .BR \%debuginfod_get_user_data () .
 The value may be NULL.
 
+.SS "URL"
+
+The URL of the current or most recent outgoing download, if known,
+may be retrieved via
+.BR \%debuginfod_get_url ()
+from the progressfn callback, or afterwards.  It may be empty or NULL.
+The resulting string is owned by the library, and must not be modified
+or freed.  The caller should copy it if it is needed beyond the release
+of the client object.
+
 .SH "CACHE"
 If the query is successful, the \fBdebuginfod_find_*\fP() functions save
 the target file to a local cache. The location of the cache is controlled
diff --git a/doc/debuginfod_get_url.3 b/doc/debuginfod_get_url.3
new file mode 100644
index 000000000000..16279936e2ea
--- /dev/null
+++ b/doc/debuginfod_get_url.3
@@ -0,0 +1 @@
+.so man3/debuginfod_find_debuginfo.3
diff --git a/tests/run-debuginfod-find.sh b/tests/run-debuginfod-find.sh
index 587eaaa3afa1..30e0a7a40fdf 100755
--- a/tests/run-debuginfod-find.sh
+++ b/tests/run-debuginfod-find.sh
@@ -216,7 +216,7 @@ tempfiles vlog
 filename=`testrun env DEBUGINFOD_PROGRESS=1 ${abs_top_builddir}/debuginfod/debuginfod-find executable $BUILDID2 2>vlog2`
 cmp $filename F/prog2
 cat vlog2
-grep -q Downloading vlog2
+grep -q 'Downloading.*http' vlog2
 tempfiles vlog2
 filename=`testrun ${abs_top_builddir}/debuginfod/debuginfod-find source $BUILDID2 ${PWD}/prog2.c`
 cmp $filename ${PWD}/prog2.c

Re: PR25369 rfc slice 2: debuginfod get_url

[Mark Wielaard]

Hi Frank,

On Fri, 2020-03-20 at 21:37 -0400, Frank Ch. Eigler via Elfutils-devel wrote:
> Slice 2/3, the debuginfod_client get_url function.  This new
> version works during or after the progressfn callback.
> 
> Author: Frank Ch. Eigler <fche@redhat.com>
> Date:   Fri Mar 20 21:33:52 2020 -0400
> 
>     debuginfod client API: add get_url function
>     
>     This function lets a client know, during or after a progressfn
>     callback, what the url of the winning outgoing download is/was.

Looks good.

The only potential issue I see with this is the use of PATH_MAX and
strncpy, which potentially chops off the URL at an arbitrary place that
cannot easily be detected.

On my system PATH_MAX is "only" 1024 chars. This might seem large, but
a non-contrived (source) URL can already be > 200 chars (start of the
URL ~50 chars, the build-id ~40 chars, "source" plus a whole source
PATH). It is not completely theoretical that (generated) source paths
would exceed 1024 chars.

I think it would be good to just initialize to NULL and use strdup and
free to store/manage it. That would also make the interface slightly
simpler, so the client doesn't have to check for the empty string, just
for NULL.

Cheers,

Mark

Re: PR25369 rfc slice 2: debuginfod get_url

[Frank Ch. Eigler]

Hi -

> [...]
> I think it would be good to just initialize to NULL and use strdup and
> free to store/manage it. That would also make the interface slightly
> simpler, so the client doesn't have to check for the empty string, just
> for NULL.

OK.

- FChE

Re: PR25369 rfc slice 2: debuginfod get_url

[Mark Wielaard]

Hi Frank,

On Sun, 2020-03-22 at 14:53 -0400, Frank Ch. Eigler wrote:
> [...]
> > I think it would be good to just initialize to NULL and use strdup and
> > free to store/manage it. That would also make the interface slightly
> > simpler, so the client doesn't have to check for the empty string, just
> > for NULL.
> 
> OK.

Looks good. Thanks for updating and pushing. But please do sent the
final patch also to the list when the code changed. So people can track
which version eventually gets in.

Thanks,

Mark

Re: PR25369 rfc slice 2: debuginfod get_url

[Frank Ch. Eigler]

Hi -

> Looks good. Thanks for updating and pushing. But please do sent the
> final patch also to the list when the code changed. So people can track
> which version eventually gets in.

Sorry .... I just hate to spam people with more email, when the information
strikes me as such small value beyond the contents of the git repo.  Are
there folks who read only the mailing list but don't git update?

- FChE

Re: PR25369 rfc slice 2: debuginfod get_url

[Mark Wielaard]

Hi Frank,

On Mon, 2020-03-23 at 12:29 -0400, Frank Ch. Eigler via Elfutils-devel
wrote:
> > Looks good. Thanks for updating and pushing. But please do sent the
> > final patch also to the list when the code changed. So people can track
> > which version eventually gets in.
> 
> Sorry .... I just hate to spam people with more email, when the information
> strikes me as such small value beyond the contents of the git repo.  Are
> there folks who read only the mailing list but don't git update?

Personally I like more email. Especially if it shows which code got
pushed and why. Then I also know whether I can expect some patch to
show up when I do a git pull and whether it is (subtly) different from
what was posted to the list earlier.

Cheers,

Mark

Re: PR25369 rfc slice 2: debuginfod get_url

[Frank Ch. Eigler]

Hi -

> > Sorry .... I just hate to spam people with more email, when the information
> > strikes me as such small value beyond the contents of the git repo.  Are
> > there folks who read only the mailing list but don't git update?
> 
> Personally I like more email. Especially if it shows which code got
> pushed and why. Then I also know whether I can expect some patch to
> show up when I do a git pull and whether it is (subtly) different from
> what was posted to the list earlier.

OK, will do.

- FChE