Hi Aaron,
On Wed, 2020-02-19 at 16:04 -0500, Aaron Merey wrote:
> From 8eb82eb58747078a3e914576a7c6ff3f7b2c7cb4 Mon Sep 17 00:00:00 2001
> From: Aaron Merey <ame...@redhat.com>
> Date: Wed, 19 Feb 2020 12:30:14 -0500
> Subject: [PATCH] PR25502: debuginfod-client default to XDG cache.
>
> Location of client cache now defaults to $XDG_CACHE_HOME/debuginfod_client.
> If XDG_CACHE_HOME is not set then fallback to $HOME/.cache/debuginfod_client.
> Also maintain backwards compatibility with the previous default path-
> if $HOME/.debuginfod_client_cache exists, use that instead of the new
> defaults.
Another option might be to rename $HOME/.debuginfod_client_cache to the
new XDG cache debuginfod_client location. But that might be tricky if
it crosses file systems/mount points. So then you would probably need
to give up and fallback to what you do now if you get EXDEV from rename
(or actually any error). Might be too much work and a little bit too
tricky/clever...?
> diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
> index d812e6d7..fc076737 100644
> --- a/debuginfod/ChangeLog
> +++ b/debuginfod/ChangeLog
> @@ -1,3 +1,9 @@
> +2020-02-19 Aaron Merey <ame...@redhat.com>
> + * debuginfod-client.c (xalloc_str): New macro. Call
> + asprintf with error checking.
> + (debuginfod_query_server): Use XDG_CACHE_HOME as a default
> + cache location if it is set. Replace snprintf with xalloc_str.
> +
> 2020-02-05 Frank Ch. Eigler <f...@redhat.com>
>
> * debuginfod.cxx (argp options): Add -Z option.
> diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
> index e5a2e824..5772367c 100644
> --- a/debuginfod/debuginfod-client.c
> +++ b/debuginfod/debuginfod-client.c
> @@ -98,6 +98,7 @@ static const time_t cache_default_max_unused_age_s =
> 604800; /* 1 week */
> /* 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";
> +static const char *cache_xdg_name = "debuginfod_client";
> static const char *cache_path_envvar = DEBUGINFOD_CACHE_PATH_ENV_VAR;
>
> /* URLs of debuginfods, separated by url_delim. */
> @@ -359,6 +360,16 @@ add_extra_headers(CURL *handle)
> }
>
>
> +#define xalloc_str(p, fmt, args...) \
> + do \
> + { \
> + if (asprintf (&p, fmt, args) < 0) \
> + { \
> + p = NULL; \
> + rc = -ENOMEM; \
> + goto out; \
> + } \
> + } while (0) \
>
> /* Query each of the server URLs found in $DEBUGINFOD_URLS for the file
> with the specified build-id, type (debuginfo, executable or source)
> @@ -373,15 +384,15 @@ debuginfod_query_server (debuginfod_client *c,
> const char *filename,
> char **path)
> {
> - char *urls_envvar;
> char *server_urls;
> - char cache_path[PATH_MAX];
> - char maxage_path[PATH_MAX*3]; /* These *3 multipliers are to shut up gcc
> -Wformat-truncation */
> - char interval_path[PATH_MAX*4];
> - char target_cache_dir[PATH_MAX*2];
> - char target_cache_path[PATH_MAX*4];
> - char target_cache_tmppath[PATH_MAX*5];
> - char suffix[PATH_MAX*2];
> + char *urls_envvar;
> + char *cache_path = NULL;
> + char *maxage_path = NULL;
> + char *interval_path = NULL;
> + char *target_cache_dir = NULL;
> + char *target_cache_path = NULL;
> + char *target_cache_tmppath = NULL;
> + char suffix[PATH_MAX];
> char build_id_bytes[MAX_BUILD_ID_BYTES * 2 + 1];
> int rc;
>
> @@ -447,24 +458,65 @@ debuginfod_query_server (debuginfod_client *c,
> target_cache_path:
> $HOME/.debuginfod_cache/0123abcd/source#PATH#TO#SOURCE ?
> */
>
> - if (getenv(cache_path_envvar))
> - strcpy(cache_path, getenv(cache_path_envvar));
> + /* Determine location of the cache. The path specified by the debuginfod
> + cache environment variable takes priority. */
> + char *cache_var = getenv(cache_path_envvar);
> + if (cache_var != NULL && strlen (cache_var) > 0)
> + xalloc_str (cache_path, "%s", cache_var);
> else
> {
> - if (getenv("HOME"))
> - sprintf(cache_path, "%s/%s", getenv("HOME"), cache_default_name);
> - else
> - sprintf(cache_path, "/%s", cache_default_name);
> + /* If a cache already exists in $HOME ('/' if $HOME isn't set), then
> use
> + that. Otherwise use the XDG cache directory naming format. */
> + xalloc_str (cache_path, "%s/%s", getenv ("HOME") ?: "/",
> cache_default_name);
> +
> + struct stat st;
> + if (stat (cache_path, &st) < 0)
> + {
> + char cachedir[PATH_MAX];
> + char *xdg = getenv ("XDG_CACHE_HOME");
> +
> + if (xdg != NULL && strlen (xdg) > 0)
> + snprintf (cachedir, PATH_MAX, "%s", xdg);
> + else
> + snprintf (cachedir, PATH_MAX, "%s/.cache", getenv ("HOME" ?:
> "/"));
Are these brackets correct? Maybe ..., getenv ("HOME") ?: "/");
> + /* Create XDG cache directory if it doesn't exist. */
> + if (stat (cachedir, &st) == 0)
> + {
> + if (! S_ISDIR (st.st_mode))
> + {
> + rc = -EEXIST;
> + goto out;
> + }
> + }
> + else
> + {
> + char *cmd;
> +
> + xalloc_str (cmd, "mkdir -p \'%s\'", cachedir);
> + rc = system (cmd);
Please don't use system. Use rc = mkdir (cachedir, 0700);
> + free (cmd);
> + if (rc == 0)
> + {
> + free (cache_path);
> + xalloc_str (cache_path, "%s/%s", cachedir, cache_xdg_name);
> + }
> + else
> + {
> + rc = -EINVAL;
> + goto out;
> + }
The creation of the directory might race with another client, so
specifically test for EEXIST and then test S_ISDIR again before
returning failure.
> + }
> + }
> }
>
> - /* avoid using snprintf here due to compiler warning. */
> - snprintf(target_cache_dir, sizeof(target_cache_dir), "%s/%s", cache_path,
> build_id_bytes);
> - snprintf(target_cache_path, sizeof(target_cache_path), "%s/%s%s",
> target_cache_dir, type, suffix);
> - snprintf(target_cache_tmppath, sizeof(target_cache_tmppath), "%s.XXXXXX",
> target_cache_path);
> + xalloc_str (target_cache_dir, "%s/%s", cache_path, build_id_bytes);
> + xalloc_str (target_cache_path, "%s/%s%s", target_cache_dir, type, suffix);
> + xalloc_str (target_cache_tmppath, "%s.XXXXXX", target_cache_path);
>
> /* XXX combine these */
> - snprintf(interval_path, sizeof(interval_path), "%s/%s", cache_path,
> cache_clean_interval_filename);
> - snprintf(maxage_path, sizeof(maxage_path), "%s/%s", cache_path,
> cache_max_unused_age_filename);
> + 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);
> rc = debuginfod_init_cache(cache_path, interval_path, maxage_path);
> if (rc != 0)
> goto out;
> @@ -479,7 +531,8 @@ debuginfod_query_server (debuginfod_client *c,
> /* Success!!!! */
> if (path != NULL)
> *path = strdup(target_cache_path);
> - return fd;
> + rc = fd;
> + goto out;
> }
>
> long timeout = default_timeout;
> @@ -754,12 +807,14 @@ debuginfod_query_server (debuginfod_client *c,
> curl_multi_cleanup (curlm);
> free (data);
> free (server_urls);
> +
> /* don't close fd - we're returning it */
> /* don't unlink the tmppath; it's already been renamed. */
> if (path != NULL)
> *path = strdup(target_cache_path);
>
> - return fd;
> + rc = fd;
> + goto out;
>
> /* error exits */
> out1:
> @@ -775,7 +830,14 @@ debuginfod_query_server (debuginfod_client *c,
> out0:
> free (server_urls);
>
> +/* general purpose exit */
> out:
> + free (cache_path);
> + free (maxage_path);
> + free (interval_path);
> + free (target_cache_dir);
> + free (target_cache_path);
> + free (target_cache_tmppath);
> return rc;
> }
I am happy with the more dynamic creating of the strings and the
cleanup. Thanks!
> diff --git a/doc/debuginfod.8 b/doc/debuginfod.8
> index ca844aed..ac231551 100644
> --- a/doc/debuginfod.8
> +++ b/doc/debuginfod.8
> @@ -406,8 +406,10 @@ or negative means "no timeout".)
> .B DEBUGINFOD_CACHE_PATH
> This environment variable governs the location of the cache where
> downloaded files are kept. It is cleaned periodically as this
> -program is reexecuted. The default is \%$HOME/.debuginfod_client_cache.
> -.\" XXX describe cache eviction policy
> +program is reexecuted. If XDG_CACHE_HOME is set then
> +$XDG_CACHE_HOME/debuginfod_client is the default location, otherwise
> +$HOME/.cache/debuginfod_client is used. For more information regarding
> +the client cache see \fIdebuginfod_find_debuginfo(3)\fP.
>
> .SH FILES
> .LP
> @@ -418,8 +420,10 @@ Default database file.
> .PD
>
> .TP 20
> -.B $HOME/.debuginfod_client_cache
> +.B $XDG_CACHE_HOME/debuginfod_client
> Default cache directory for content from upstream debuginfods.
> +If XDG_CACHE_HOME is not set then \fB$HOME/.cache/debuginfod_client\fP
> +is used.
> .PD
>
> diff --git a/doc/debuginfod_find_debuginfo.3 b/doc/debuginfod_find_debuginfo.3
> index c232ac71..f9e770b0 100644
> --- a/doc/debuginfod_find_debuginfo.3
> +++ b/doc/debuginfod_find_debuginfo.3
> @@ -180,7 +180,10 @@ function output.
> .B DEBUGINFOD_CACHE_PATH
> This environment variable governs the location of the cache where
> downloaded files are kept. It is cleaned periodically as this
> -program is reexecuted. The default is $HOME/.debuginfod_client_cache.
> +program is reexecuted. If XDG_CACHE_HOME is set then
> +$XDG_CACHE_HOME/debuginfod_client is the default location, otherwise
> +$HOME/.cache/debuginfod_client is used.
> +
>
> .SH "ERRORS"
> The following list is not comprehensive. Error codes may also
> @@ -244,7 +247,8 @@ the timeout duration. See debuginfod(8) for more
> information.
> .PD .1v
> .TP 20
> .B $HOME/.debuginfod_client_cache
> -Default cache directory.
> +Default cache directory. If XDG_CACHE_HOME is not set then
> +\fB$HOME/.cache/debuginfod_client\fP is used.
> .PD
Should we mention the old location might still be used?
Maybe simply ignoring it is fine.
> .SH "SEE ALSO"
> diff --git a/tests/ChangeLog b/tests/ChangeLog
> index 1f55a291..ec081b3c 100644
> --- a/tests/ChangeLog
> +++ b/tests/ChangeLog
> @@ -1,3 +1,7 @@
> +2020-02-19 Aaron Merey <ame...@redhat.com>
> + * run-debuginfod-find.sh: Run tests for verifying default
> + client cache locations.
Probably too much work for this patch. But it feels run-debuginfod-
find.sh is doing a lot of different testing. Might be good to put some
tests in their own test file.
> 2020-02-08 Mark Wielaard <m...@klomp.org>
>
> * run-pt_gnu_prop-tests.sh: New test.
> diff --git a/tests/run-debuginfod-find.sh b/tests/run-debuginfod-find.sh
> index 1cc8f406..9740f4aa 100755
> --- a/tests/run-debuginfod-find.sh
> +++ b/tests/run-debuginfod-find.sh
> @@ -40,7 +40,7 @@ cleanup()
> if [ $PID2 -ne 0 ]; then kill $PID2; wait $PID2; fi
> if [ $PID3 -ne 0 ]; then kill $PID3; wait $PID3; fi
>
> - rm -rf F R D L Z ${PWD}/.client_cache*
> + rm -rf F R D L Z ${PWD}/.client_cache* ${PWD}/tmp*
> exit_cleanup
> }
>
> @@ -147,6 +147,43 @@ cmp $filename ${PWD}/prog.c
>
> ########################################################################
>
> +# Test whether the cache default locations are correct
> +
> +mkdir tmphome
> +
> +# $HOME/.cache should be created.
> +env HOME=$PWD/tmphome XDG_CACHE_HOME= DEBUGINFOD_CACHE_PATH=
> ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID
> +if [ ! -f $PWD/tmphome/.cache/debuginfod_client/$BUILDID/debuginfo ]; then
> + echo "could not find cache in $PWD/tmphome/.cache"
> + exit 1
> +fi
> +
> +# $XDG_CACHE_HOME should take priority over $HOME.cache.
> +env HOME=$PWD/tmphome XDG_CACHE_HOME=$PWD/tmpxdg DEBUGINFOD_CACHE_PATH=
> ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID
> +if [ ! -f $PWD/tmpxdg/debuginfod_client/$BUILDID/debuginfo ]; then
> + echo "could not find cache in $PWD/tmpxdg/"
> + exit 1
> +fi
> +
> +# A cache at the old default location ($HOME/.debuginfod_client_cache)
> should take
> +# priority over $HOME/.cache, $XDG_CACHE_HOME.
> +cp -r $DEBUGINFOD_CACHE_PATH tmphome/.debuginfod_client_cache
> +
> +# Add a file that doesn't exist in $HOME/.cache, $XDG_CACHE_HOME.
> +mkdir tmphome/.debuginfod_client_cache/deadbeef
> +echo ELF... > tmphome/.debuginfod_client_cache/deadbeef/debuginfo
> +filename=`env HOME=$PWD/tmphome XDG_CACHE_HOME=$PWD/tmpxdg
> DEBUGINFOD_CACHE_PATH= ${abs_top_builddir}/debuginfod/debuginfod-find
> debuginfo deadbeef`
> +cmp $filename tmphome/.debuginfod_client_cache/deadbeef/debuginfo
> +
> +# $DEBUGINFO_CACHE_PATH should take priority over all else.
> +env HOME=$PWD/tmphome XDG_CACHE_HOME=$PWD/tmpxdg
> DEBUGINFOD_CACHE_PATH=$PWD/tmpcache
> ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID
> +if [ ! -f $PWD/tmpcache/$BUILDID/debuginfo ]; then
> + echo "could not find cache in $PWD/tmpcache/"
> + exit 1
> +fi
You need to use testrun to run "test" binaries (or at least make sure
LD_LIBRARY_PATH is setup correctly) to make sure the test environment
is setup correctly. The above doesn't work when using srcdir!=builddir
and testrun will make sure that valgrind is ran (as is done with make
distcheck) when you configure with --enable-valgrind (which would have
caught the previous small memleak).
diff --git a/tests/run-debuginfod-find.sh b/tests/run-debuginfod-find.sh
index 317c687c..083ff59b 100755
--- a/tests/run-debuginfod-find.sh
+++ b/tests/run-debuginfod-find.sh
@@ -152,14 +152,14 @@ cmp $filename ${PWD}/prog.c
mkdir tmphome
# $HOME/.cache should be created.
-env HOME=$PWD/tmphome XDG_CACHE_HOME= DEBUGINFOD_CACHE_PATH=
${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID
+testrun env HOME=$PWD/tmphome XDG_CACHE_HOME= DEBUGINFOD_CACHE_PATH=
${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID
if [ ! -f $PWD/tmphome/.cache/debuginfod_client/$BUILDID/debuginfo ]; then
echo "could not find cache in $PWD/tmphome/.cache"
exit 1
fi
# $XDG_CACHE_HOME should take priority over $HOME.cache.
-env HOME=$PWD/tmphome XDG_CACHE_HOME=$PWD/tmpxdg DEBUGINFOD_CACHE_PATH=
${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID
+testrun env HOME=$PWD/tmphome XDG_CACHE_HOME=$PWD/tmpxdg
DEBUGINFOD_CACHE_PATH= ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo
$BUILDID
if [ ! -f $PWD/tmpxdg/debuginfod_client/$BUILDID/debuginfo ]; then
echo "could not find cache in $PWD/tmpxdg/"
exit 1
@@ -172,11 +172,11 @@ cp -r $DEBUGINFOD_CACHE_PATH
tmphome/.debuginfod_client_cache
# Add a file that doesn't exist in $HOME/.cache, $XDG_CACHE_HOME.
mkdir tmphome/.debuginfod_client_cache/deadbeef
echo ELF... > tmphome/.debuginfod_client_cache/deadbeef/debuginfo
-filename=`env HOME=$PWD/tmphome XDG_CACHE_HOME=$PWD/tmpxdg
DEBUGINFOD_CACHE_PATH= ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo
deadbeef`
+filename=`testrun env HOME=$PWD/tmphome XDG_CACHE_HOME=$PWD/tmpxdg
DEBUGINFOD_CACHE_PATH= ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo
deadbeef`
cmp $filename tmphome/.debuginfod_client_cache/deadbeef/debuginfo
# $DEBUGINFO_CACHE_PATH should take priority over all else.
-env HOME=$PWD/tmphome XDG_CACHE_HOME=$PWD/tmpxdg
DEBUGINFOD_CACHE_PATH=$PWD/tmpcache
${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID
+testrun env HOME=$PWD/tmphome XDG_CACHE_HOME=$PWD/tmpxdg
DEBUGINFOD_CACHE_PATH=$PWD/tmpcache
${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID
if [ ! -f $PWD/tmpcache/$BUILDID/debuginfo ]; then
echo "could not find cache in $PWD/tmpcache/"
exit 1
Cheers,
Mark
