PATCH: PR27784 debuginfod: notify interactive clients one time about usage

PATCH: PR27784 debuginfod: notify interactive clients one time about usage

[Frank Ch. Eigler]

Hi -

Another request from fedora fesco.  (I don't have anything else on the
queue for the next elfutils release.)

commit be5ebebb1f12134b673c5b8cbede62390d077b0d
Author: Frank Ch. Eigler <fche@redhat.com>
Date:   Thu Apr 29 14:34:06 2021 -0400

    PR27784 debuginfod: notify interactive clients one time about usage
    
    Because debuginfod-client functionality makes a user dependent on the
    correct operation of remote debuginfod server, it was suggested that
    new users be notified of this.  This patch adds a one-time
    notification to stderr if it isatty(), commemorated by a new cache
    notify_p file.  (It cannot easily be tested because our test scripts
    run without pty/tty enclosure.)
    
    Signed-off-by: Frank Ch. Eigler <fche@redhat.com>

diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
index 20744497e383..800709416c61 100644
--- a/debuginfod/ChangeLog
+++ b/debuginfod/ChangeLog
@@ -1,3 +1,9 @@
+2021-04-29  Frank Ch. Eigler <fche@redhat.com>
+
+	PR27784
+	* debuginfod-client.c (debuginfod_query_server): Notify interactive
+	users, one time, about impending debuginfod-client usage.
+
 2021-04-29  Frank Ch. Eigler <fche@redhat.com>
 
 	PR27783
diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index e65aac8b4c1c..d3fbf7caae6d 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -141,6 +141,10 @@ static const time_t cache_clean_default_interval_s = 86400; /* 1 day */
 static const char *cache_max_unused_age_filename = "max_unused_age_s";
 static const time_t cache_default_max_unused_age_s = 604800; /* 1 week */
 
+/* The cache_notified_p_filename file within the cache indicates the user has
+   received a one-time notification of debuginfod client operation on a TTY. */
+static const char *cache_notified_p_filename = "notified_p";
+
 /* Location of the cache of files downloaded from debuginfods.
    The default parent directory is $HOME, or '/' if $HOME doesn't exist.  */
 static const char *cache_default_name = ".debuginfod_client_cache";
@@ -503,6 +507,7 @@ debuginfod_query_server (debuginfod_client *c,
   char *urls_envvar;
   char *cache_path = NULL;
   char *maxage_path = NULL;
+  char *notified_p_path = NULL;
   char *interval_path = NULL;
   char *target_cache_dir = NULL;
   char *target_cache_path = NULL;
@@ -678,7 +683,28 @@ debuginfod_query_server (debuginfod_client *c,
   /* XXX combine these */
   xalloc_str (interval_path, "%s/%s", cache_path, cache_clean_interval_filename);
   xalloc_str (maxage_path, "%s/%s", cache_path, cache_max_unused_age_filename);
+  xalloc_str (notified_p_path, "%s/%s", cache_path, cache_notified_p_filename);
+
+
+  /* If on a TTY, notify the user if this is the first time
+     debuginfod-client is activated. */
+  if (isatty (STDERR_FILENO) &&
+      access (notified_p_path, R_OK))
+    {
+      int fd = open (notified_p_path, O_CREAT | O_RDWR, 0400);
+      if (fd >= 0)
+        close (fd);
 
+      if (vfd >= 0)
+        dprintf (vfd, "recorded first notification %s\n", notified_p_path);
+
+      fprintf (stderr,
+               "\nNOTICE: This system is configured to auto-download debuginfo from:\n%s\n\n",
+               urls_envvar);
+      fflush (stderr);
+      sleep (3);
+    }
+  
   if (vfd >= 0)
     dprintf (vfd, "checking cache dir %s\n", cache_path);
 
@@ -1112,6 +1138,7 @@ debuginfod_query_server (debuginfod_client *c,
     }
 
   free (cache_path);
+  free (notified_p_path);
   free (maxage_path);
   free (interval_path);
   free (target_cache_dir);
diff --git a/tests/ChangeLog b/tests/ChangeLog
index bc2016f40203..1f1b2dd8695b 100644
--- a/tests/ChangeLog
+++ b/tests/ChangeLog
@@ -1,3 +1,8 @@
+2021-04-29  Frank Ch. Eigler <fche@redhat.com>
+
+	PR27784
+	* run-debuginfod-find.sh: Don't test tty first-use notification.
+
 2021-04-29  Frank Ch. Eigler <fche@redhat.com>
 
 	PR27783
diff --git a/tests/run-debuginfod-find.sh b/tests/run-debuginfod-find.sh
index 9a217121b7e2..cc364c861ca4 100755
--- a/tests/run-debuginfod-find.sh
+++ b/tests/run-debuginfod-find.sh
@@ -384,6 +384,12 @@ wait_ready $PORT1 'groomed_total{decision="stale"}' 4
 
 rm -rf $DEBUGINFOD_CACHE_PATH # clean it from previous tests
 
+# check for first-use notice; don't bother run this one under valgrind, to collect stderr
+# ... but need a pty/tty to trigger the message, so just comment this out for now
+# ${abs_top_builddir}/debuginfod/debuginfod-find executable 0000111122223333 2>errfile && false || true
+# tempfiles errfile
+# grep NOTICE errfile
+
 # this is one of the buildids from the groom-deleted rpms
 testrun ${abs_top_builddir}/debuginfod/debuginfod-find executable $RPM_BUILDID && false || true
 # but this one was not deleted so should be still around

Re: PATCH: PR27784 debuginfod: notify interactive clients one time about usage

[Mark Wielaard]

Hi Frank,

On Thu, 2021-04-29 at 14:44 -0400, Frank Ch. Eigler via Elfutils-devel
wrote:
> Another request from fedora fesco.  (I don't have anything else on
> the queue for the next elfutils release.)
> 
> commit be5ebebb1f12134b673c5b8cbede62390d077b0d
> Author: Frank Ch. Eigler <fche@redhat.com>
> Date:   Thu Apr 29 14:34:06 2021 -0400
> 
>     PR27784 debuginfod: notify interactive clients one time about usage
>     
>     Because debuginfod-client functionality makes a user dependent on the
>     correct operation of remote debuginfod server, it was suggested that
>     new users be notified of this.  This patch adds a one-time
>     notification to stderr if it isatty(), commemorated by a new cache
>     notify_p file.  (It cannot easily be tested because our test scripts
>     run without pty/tty enclosure.)

I cannot say I am a fan of the library being the one to do this check,
especially not with the sleep (3) in it. It also is not really
accurate, it only shows the debuginfod server URLs the first time they
are set, not when they are changed. I would hide this behind
DEBUGINFOD_VERBOSE=1 but that already shows the URLs anyway.

If there is such a "notification" then it should be from the actual
application using the library IMHO. Or when installing a profile if it
provides default DEBUGINFOD_URLS.

Cheers,

Mark

Re: PATCH: PR27784 debuginfod: notify interactive clients one time about usage

[Frank Ch. Eigler]

Hi -


> I cannot say I am a fan of the library being the one to do this check,
> especially not with the sleep (3) in it. 

It's a clumsy compromise, yes, but it is a one-time thing that only
humans should see, and only once.

> It also is not really accurate, it only shows the debuginfod server
> URLs the first time they are set, not when they are changed. [...]

Would you find it more palatable if it were to compare previous and
current $DEBUGINFOD_URLS (say, written into that notify_p file)?

> If there is such a "notification" then it should be from the actual
> application using the library IMHO. 

That would be a dozen+ separate applications.

> Or when installing a profile if it provides default DEBUGINFOD_URLS.

Like at "rpm -i"?  Those times are by design not chatty, and not
visible to the right people anyway.


- FChE

Re: PATCH: PR27784 debuginfod: notify interactive clients one time about usage

[Frank Ch. Eigler]

Hi -

OK, how about this?


diff --git a/config/profile.sh.in b/config/profile.sh.in
index aa228a0dcd16..aec9e7df30f3 100644
--- a/config/profile.sh.in
+++ b/config/profile.sh.in
@@ -1,4 +1,18 @@
 if [ -n "@DEBUGINFOD_URLS@" ]; then
-	DEBUGINFOD_URLS="${DEBUGINFOD_URLS-}${DEBUGINFOD_URLS:+ }@DEBUGINFOD_URLS@"
-	export DEBUGINFOD_URLS
+    DEBUGINFOD_URLS="${DEBUGINFOD_URLS-}${DEBUGINFOD_URLS:+ }@DEBUGINFOD_URLS@"
+    debuginfod_cachedir="${XDG_CACHE_HOME-$HOME/.cache}/debuginfod_client"
+    notify_p="$debuginfod_cachedir"/notify_p
+    if [ ! -f "$notify_p" ]; then
+        msg="NOTICE: This system is configured to auto-download debuginfo from:
+$DEBUGINFOD_URLS
+NOTICE: Set your \$DEBUGINFOD_URLS differently if desired."
+        if [ -x /usr/bin/notify-send -a -n "$DISPLAY" ]; then
+            /usr/bin/notify-send "$msg"
+	fi
+        echo "$msg" >&2
+        mkdir -p "$debuginfod_cachedir"
+	touch "$notify_p"
+    fi
+    unset debuginfod_cachedir notify_p msg
+    export DEBUGINFOD_URLS
 fi

Re: PATCH: PR27784 debuginfod: notify interactive clients one time about usage

[Mark Wielaard]

Hi Frank,

On Wed, May 05, 2021 at 04:20:36PM -0400, Frank Ch. Eigler wrote:
> OK, how about this?

I like this a lot better than doing it in the library code.

Maybe the notify_p file should be stored under
${XDG_CONFIG_HOME-$HOME/.config} instead, because it isn't really a
cache file. And maybe s/This system/Your user profile/? But those are
just nitpicks and not objections to what you proposed.

Cheers,

Mark

> diff --git a/config/profile.sh.in b/config/profile.sh.in
> index aa228a0dcd16..aec9e7df30f3 100644
> --- a/config/profile.sh.in
> +++ b/config/profile.sh.in
> @@ -1,4 +1,18 @@
>  if [ -n "@DEBUGINFOD_URLS@" ]; then
> -	DEBUGINFOD_URLS="${DEBUGINFOD_URLS-}${DEBUGINFOD_URLS:+ }@DEBUGINFOD_URLS@"
> -	export DEBUGINFOD_URLS
> +    DEBUGINFOD_URLS="${DEBUGINFOD_URLS-}${DEBUGINFOD_URLS:+ }@DEBUGINFOD_URLS@"
> +    debuginfod_cachedir="${XDG_CACHE_HOME-$HOME/.cache}/debuginfod_client"
> +    notify_p="$debuginfod_cachedir"/notify_p
> +    if [ ! -f "$notify_p" ]; then
> +        msg="NOTICE: This system is configured to auto-download debuginfo from:
> +$DEBUGINFOD_URLS
> +NOTICE: Set your \$DEBUGINFOD_URLS differently if desired."
> +        if [ -x /usr/bin/notify-send -a -n "$DISPLAY" ]; then
> +            /usr/bin/notify-send "$msg"
> +	fi
> +        echo "$msg" >&2
> +        mkdir -p "$debuginfod_cachedir"
> +	touch "$notify_p"
> +    fi
> +    unset debuginfod_cachedir notify_p msg
> +    export DEBUGINFOD_URLS
>  fi
>