[PATCH] PR25628: Debuginfod client should cache negative results.

[Mark Wielaard]

Hi Alice,

On Wed, Apr 28, 2021 at 08:10:49AM -0400, Alice Zhang via Elfutils-devel wrote:
> * debuginfod/debuginfod-client.c:
>   - add debuginfod_config_cache for reading and writing to cache
>     configuration files, make use of the function within
>     debuginfod_clean_cache and debuginfod_query_server.
>   - in debuginfod_query_server, create 000-permission file on failed
>     queries. Before querying each BUILDID, if corresponding 000 file
>     detected, compare its stat mtime with parameter from
>     .cache/cache_miss_s. if mtime is fresher, then return ENOENT and
>     exit; otherwise unlink the 000 file and proceed to a new query.

I like the idea.

> diff --git a/ChangeLog b/ChangeLog
> index e18746fb..3c6f5cc0 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,7 @@
> +2021-04-28  Alice Zhang  <azhang@redhat.com>
> +        * debuginfod/debuginfod-client.c: Make client able to cache negative
> +	results.
> +	* tests/run-debuginfod-find.sh: Added tests for the feature.

Normally ChangeLog entries go into the ChangeLog files in the
subdirectories.

>  2021-03-30  Frank Ch. Eigler  <fche@redhat.com>
>  
>  	* configure.ac: Look for pthread_setname_np.
> diff --git a/NEWS b/NEWS
> index 038c6019..6d652696 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -2,6 +2,15 @@ Version 0.184
>  
>  debuginfod: Use libarchive's bsdtar as the .deb-family file unpacker.
>  
> +debuginfod-client: Now client would cache negative results. When a
> +                   query fails with 404, a 000-permission file is
> +                   created. When query for a buildid, check whether
> +                   such 000-permission file exists in the cache. If
> +                   so, check its mtime: if mtime is older than the
> +                   parameter from .cache/cache_miss_s, unlink the
> +                   000 file and proceed to a new query; otherwise
> +                   return an -ENOENT.

This is good as commit message, for NEWS try to summarize in one or
two sentences.

> +/* handle config file read and write */
> +static int
> +debuginfod_config_cache(char *config_path,
> +                       long cache_config_default_s)
> +{
> +  int fd;
> +  struct stat st;
> +  /* if the config file doesn't exist, create one with DEFFILEMODE*/
> +  if(stat(config_path, &st) == -1)
> +    {
> +      fd = open(config_path, O_CREAT | O_RDWR, DEFFILEMODE);
> +      if (fd < 0)
> +        return -errno;
> +
> +      if (dprintf(fd, "%ld", cache_config_default_s) < 0)
> +        return -errno;
> +    }
> +
> +  long cache_config;
> +  FILE *config_file = fopen(config_path, "r");
> +  if (config_file)
> +    {
> +      if (fscanf(config_file, "%ld", &cache_config) != 1)
> +        cache_config = cache_config_default_s;
> +      fclose(config_file);
> +    }
> +  else
> +    cache_config = cache_config_default_s;
> +
> +  return cache_config;
> +}

Good idea to generalize this in a new function.  I would suggest to
add a struct stat &st argument so the caller gets the struct stat
result, because...

> +  stat(interval_path, &st);
>    if (time(NULL) - st.st_mtime < clean_interval)
>      /* Interval has not passed, skip cleaning.  */
>      return 0;

Then you can simply use it here ^

> @@ -690,6 +704,24 @@ debuginfod_query_server (debuginfod_client *c,
>        goto out;
>      }

The code ^ tries to open the target_cache_path file.  I think it would
be good to put the below in inside and else branch that checks whether
the open failed with EACCESS, so you know it is a 0000 mode file.

     else if (errno == EACCES)
       {

> +  struct stat st;
> +  time_t cache_miss;
> +  if (stat(target_cache_path, &st) == 0)

And although a race is unlikely, it could be some other client
happened to have created/replaced the file (less likely if you add the
EACCES check). So it would be good to check st.st_mode & 0777 == 0.

> +    {
> +      rc = debuginfod_config_cache(cache_miss_path, cache_miss_default_s);
> +      if (rc < 0)
> +        goto out;
> +
> +      cache_miss = (time_t)rc;
> +      if (time(NULL) - st.st_mtime <= cache_miss)
> +        {
> +         rc = -ENOENT;
> +         goto out;
> +       }
> +      else
> +        unlink(target_cache_path);
> +    }
> +
[...]
> +  /* create a 000-permission file named as $HOME/.cache
> +   * if the query fails with ENOENT.*/
> +  if (rc == -ENOENT)
> +    {
> +      int efd = open (target_cache_path, O_CREAT|O_TRUNC, 0000);
> +      if (efd >= 0)
> +        close(efd);
> +    }

Do we want O_TRUNC here? Doesn't that risk truncating an existing
file? Maybe use O_CREAT|O_EXCL?

Thanks,

Mark