[PATCH] debuginfod-client: default to XDG cache.

[Aaron Merey]

On Fri, Feb 21, 2020 at 11:13 AM Mark Wielaard <mark@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  <amerey@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