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 <azh...@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 <f...@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
