Re: patch 3/3 debuginfod client interruptability

[Mark Wielaard <mark@klomp.org>]

[-- Attachment #1: Type: text/plain, Size: 1324 bytes --]

Hi,

On Sun, 2019-11-17 at 21:50 -0500, Frank Ch. Eigler wrote:
> > Attached is a variant that adds debuginfod_begin and debuginfo_end
> > (names matching elf/dwarf_begin/end) and adds a debuginfod_client
> > handle to each other function.
> 
> Sure, if you like.

OK, I rebased on the debuginfod-submit branch and added documentation
(see attached).

>   Would you be sympathetic to supporting a
> client=NULL entrypoint to the lookup functions, ergo no begin/end, for
> applications that don't want a progressfn callback?  That way the
> simple case looks simple.

I think it is better to be consistent and always require a valid client
connection handle for the reasons that Pedro gave.

> > diff --git a/debuginfod/debuginfod.cxx b/debuginfod/debuginfod.cxx
> >  
> > +static debuginfod_client *client;
> 
> Note that multiple http webapi handling threads may make
> federated debuginfod calls concurrently.  Is it your idea
> that they share a single client object?

No, it would be better to give every thread it own handle.
The attached patch does that.

All patches are also on 
https://code.wildebeest.org/git/user/mjw/elfutils/log/?h=debuginfod-client-context

If you think those patches are ok I can squash them and add them to the
debuginfod-submit branch.

Cheers,

Mark

[-- Attachment #2: 0001-update-documentation-for-client-connection-handle-us.patch --]
[-- Type: text/x-patch, Size: 4845 bytes --]

From 8006db933298ecca35a04207aad3991b242c21da Mon Sep 17 00:00:00 2001
From: Mark Wielaard <mark@klomp.org>
Date: Tue, 19 Nov 2019 13:09:24 +0100
Subject: [PATCH] update documentation for client connection handle usage

---
 doc/debuginfod_begin.3          |  1 +
 doc/debuginfod_end.3            |  1 +
 doc/debuginfod_find_debuginfo.3 | 51 ++++++++++++++++++++++++++-------
 3 files changed, 42 insertions(+), 11 deletions(-)
 create mode 100644 doc/debuginfod_begin.3
 create mode 100644 doc/debuginfod_end.3

diff --git a/doc/debuginfod_begin.3 b/doc/debuginfod_begin.3
new file mode 100644
index 00000000..16279936
--- /dev/null
+++ b/doc/debuginfod_begin.3
@@ -0,0 +1 @@
+.so man3/debuginfod_find_debuginfo.3
diff --git a/doc/debuginfod_end.3 b/doc/debuginfod_end.3
new file mode 100644
index 00000000..16279936
--- /dev/null
+++ b/doc/debuginfod_end.3
@@ -0,0 +1 @@
+.so man3/debuginfod_find_debuginfo.3
diff --git a/doc/debuginfod_find_debuginfo.3 b/doc/debuginfod_find_debuginfo.3
index b65a6c57..66619157 100644
--- a/doc/debuginfod_find_debuginfo.3
+++ b/doc/debuginfod_find_debuginfo.3
@@ -21,15 +21,39 @@ debuginfod_find_debuginfo \- request debuginfo from debuginfod
 .nf
 .B #include <elfutils/debuginfod.h>
 .PP
-.BI "int debuginfod_find_debuginfo(const unsigned char *" build_id ", int " build_id_len ", char ** " path ");"
-.BI "int debuginfod_find_executable(const unsigned char *" build_id ", int " build_id_len ", char ** " path ");"
-.BI "int debuginfod_find_source(const unsigned char *" build_id ", int " build_id_len ", const char *" filename ", char ** " path ");"
-.BI "typedef int (*debuginfo_progressfn_t)(long a, long b);"
-.BI "debuginfo_progressfn_t debuginfod_set_progressfn(debuginfo_progressfn_t " progressfn ");"
+.BI "debuginfod_client *debuginfod_begin(void);"
+.BI "void debuginfod_end(debuginfod_client *" client ");"
+
+.BI "int debuginfod_find_debuginfo(debuginfod_client *" client ","
+.BI "                              const unsigned char *" build_id ","
+.BI "                              int " build_id_len ","
+.BI "                              char ** " path ");"
+.BI "int debuginfod_find_executable(debuginfod_client *" client ","
+.BI "                               const unsigned char *" build_id ","
+.BI "                               int " build_id_len ","
+.BI "                               char ** " path ");"
+.BI "int debuginfod_find_source(debuginfod_client *" client ","
+.BI "                           const unsigned char *" build_id ","
+.BI "                           int " build_id_len ","
+.BI "                           const char *" filename ","
+.BI "                           char ** " path ");"
+
+.BI "typedef int (*debuginfo_progressfn_t)(debuginfod_client *" client ","
+.BI "                                      long a, long b);"
+.BI "void debuginfod_set_progressfn(debuginfod_client *" client ","
+.BI "                               debuginfo_progressfn_t " progressfn ");"
 
 Link with \fB-ldebuginfod\fP.
 
 .SH DESCRIPTION
+
+.BR debuginfod_begin ()
+creates a \fBdebuginfod_client\fP connection handle that should be used
+with all other calls.
+.BR debuginfod_end ()
+should be called on the \fBclient\fP handle to release all state and
+storage when done.
+
 .BR debuginfod_find_debuginfo (),
 .BR debuginfod_find_executable (),
 and
@@ -65,9 +89,14 @@ The URLs in \fB$DEBUGINFOD_URLS\fP may be queried in parallel. As soon
 as a debuginfod server begins transferring the target file all of the
 connections to the other servers are closed.
 
-These functions are MT-safe.
+A \fBclient\fP handle should be used from only one thread at a time.
 
 .SH "RETURN VALUE"
+
+\fBdebuginfod_begin\fP returns the \fBdebuginfod_client\fP handle to
+use with all other calls.  On error \fBNULL\fP will be returned and
+\fBerrno\fP will be set.
+
 If a find family function is successful, the resulting file is saved
 to the client cache and a file descriptor to that file is returned.
 The caller needs to \fBclose\fP() this descriptor.  Otherwise, a
@@ -75,12 +104,12 @@ negative error code is returned.
 
 .SH "PROGRESS CALLBACK"
 
-As the \fBdebuginfod_find_*\fP() functions may block for seconds or longer, a progress
-callback function is called periodically, if configured with
+As the \fBdebuginfod_find_*\fP() functions may block for seconds or
+longer, a progress callback function is called periodically, if
+configured with
 .BR debuginfod_set_progressfn ().
-This function sets a new progress callback function (or NULL) and
-returns the previously set function (or NULL).  This function may be
-MT-unsafe.
+This function sets a new progress callback function (or NULL) for the
+client handle.
 
 The given callback function is called from the context of each thread
 that is invoking any of the other lookup functions.  It is given two
-- 
2.18.1


[-- Attachment #3: 0001-debuginfod-create-a-client-connection-handle-for-eac.patch --]
[-- Type: text/x-patch, Size: 2169 bytes --]

From 9c3817f2a52d516a88961a25e4e0d8f177b42463 Mon Sep 17 00:00:00 2001
From: Mark Wielaard <mark@klomp.org>
Date: Tue, 19 Nov 2019 13:50:41 +0100
Subject: [PATCH] debuginfod: create a client connection handle for each
 handler thread

---
 debuginfod/debuginfod.cxx | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/debuginfod/debuginfod.cxx b/debuginfod/debuginfod.cxx
index 7d3d580f..7baf18ec 100644
--- a/debuginfod/debuginfod.cxx
+++ b/debuginfod/debuginfod.cxx
@@ -360,7 +360,6 @@ static struct argp argp =
   };
 
 
-static debuginfod_client *client;
 static string db_path;
 static sqlite3 *db;
 static unsigned verbose;
@@ -1039,8 +1038,11 @@ static struct MHD_Response* handle_buildid (const string& buildid /* unsafe */,
   // is to defer to other debuginfo servers.
 
   int fd = -1;
+  debuginfod_client *client = debuginfod_begin ();
   if (client != NULL)
     {
+      debuginfod_set_progressfn (client, & debuginfod_find_progress);
+
       if (artifacttype == "debuginfo")
 	fd = debuginfod_find_debuginfo (client,
 					(const unsigned char*) buildid.c_str(),
@@ -1055,7 +1057,8 @@ static struct MHD_Response* handle_buildid (const string& buildid /* unsafe */,
 				     0, suffix.c_str(), NULL);
     }
   else
-    fd = -ENOSYS;
+    fd = -errno; /* Set by debuginfod_begin.  */
+  debuginfod_end (client);
 
   if (fd >= 0)
     {
@@ -2518,10 +2521,6 @@ main (int argc, char *argv[])
              "cannot run database schema ddl: %s", sqlite3_errmsg(db));
     }
 
-  client = debuginfod_begin ();
-  if (client != NULL)
-    debuginfod_set_progressfn (client, & debuginfod_find_progress);
-
   // Start httpd server threads.  Separate pool for IPv4 and IPv6, in
   // case the host only has one protocol stack.
   MHD_Daemon *d4 = MHD_start_daemon (MHD_USE_THREAD_PER_CONNECTION
@@ -2649,7 +2648,6 @@ main (int argc, char *argv[])
   if (d6) MHD_stop_daemon (d6);
 
   /* With all threads known dead, we can clean up the global resources. */
-  debuginfod_end (client);
   delete scan_concurrency_sem;
   rc = sqlite3_exec (db, DEBUGINFOD_SQLITE_CLEANUP_DDL, NULL, NULL, NULL);
   if (rc != SQLITE_OK)
-- 
2.18.1