On Fri, Feb 21, 2020 at 11:13 AM Mark Wielaard <m...@klomp.org> wrote:
> On Wed, 2020-02-19 at 16:04 -0500, Aaron Merey wrote:
> > 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...?
We can try a simple rename and fallback if there's any error. We could
mention this briefly in the man page.
> > + 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") ?: "/");
You're right it should be 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);
Sure. My goal was to have any nonexisting parent directories created as
easily as possible and I wasn't aware of anything besides `mkdir -p`.
Should we not bother creating nonexisting parent directories?
> > + 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.
Ok.
> > .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.
Here I could mention the possibility of a rename when using a client
cache at the old location.
> > .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.
That's a good idea for a future patch, maybe separate client and server
tests into two files (to the extent that they can be separated).
> 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
Ok I'll include that.
Thanks,
Aaron
