Re: [PATCH v4 3/4] trace2: don't overload target directories
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Oct 4, 2019, 09:12 UTC
- Message-ID
- <nycvar.QRO.7.76.6.1910041105290.46@tvgsbejvaqbjf.bet>
- In-Reply-To
- <98a8440d3f0ef3cd3cdd0291051d976d4a659cc4.1570144820.git.steadmon@google.com>
Hi Josh,
On Thu, 3 Oct 2019, Josh Steadmon wrote:
Show 8 quoted lines
> [...] > diff --git a/trace2/tr2_dst.c b/trace2/tr2_dst.c > index 5dda0ca1cd..af3405f179 100644 > --- a/trace2/tr2_dst.c > +++ b/trace2/tr2_dst.c > @@ -1,3 +1,5 @@ > +#include <dirent.h> > +
This completely breaks the Windows build:
In file included from trace2/tr2_dst.c:1: compat/win32/dirent.h:13:14: error: 'MAX_PATH' undeclared here (not in a function) 13 | char d_name[MAX_PATH * 3]; /* file name (* 3 for UTF-8 conversion) */ | ^~~~~~~~
See the full build log in all its glory here:
https://dev.azure.com/gitgitgadget/git/_build/results?buildId=17409&view=logs&jobId=fd490c07-0b22-5182-fac9-6d67fe1e939b&taskId=ce91d5d6-0c55-50f5-8ab9-6695c03ab102&lineStart=252&lineEnd=255&colStart=1&colEnd=30
The fix is as easy as probably surprising: simply delete that `#include`. It is redundant anyway: https://github.com/git/git/blob/v2.23.0/git-compat-util.h#L184
Deleting that `#include` line from your patch not only lets the file compile cleanly on Windows, it also conforms to our coding style where `cache.h` or `git-compat-util.h` need to be the first `#include`. That rule's purpose is precisely to prevent platform-specific compile errors like the one shown above.
Ciao, Johannes
Show 139 quoted lines
> #include "cache.h"
> #include "trace2/tr2_dst.h"
> #include "trace2/tr2_sid.h"
> @@ -8,6 +10,19 @@
> */
> #define MAX_AUTO_ATTEMPTS 10
>
> +/*
> + * Sentinel file used to detect when we're overloading a directory with too many
> + * trace files.
> + */
> +#define OVERLOAD_SENTINEL_NAME "git-trace2-overload"
> +
> +/*
> + * When set to zero, disables directory overload checking. Otherwise, controls
> + * how many files we can write to a directory before entering overload mode.
> + * This can be overridden via the TR2_SYSENV_MAX_FILES setting.
> + */
> +static int tr2env_max_files = 0;
> +
> static int tr2_dst_want_warning(void)
> {
> static int tr2env_dst_debug = -1;
> @@ -32,6 +47,67 @@ void tr2_dst_trace_disable(struct tr2_dst *dst)
> dst->need_close = 0;
> }
>
> +/*
> + * Check to make sure we're not overloading the target directory with too many
> + * files. First get the threshold (if present) from the config or envvar. If
> + * it's zero or unset, disable this check. Next check for the presence of a
> + * sentinel file, then check file count. If we are overloaded, create the
> + * sentinel file if it doesn't already exist.
> + *
> + * We expect that some trace processing system is gradually collecting files
> + * from the target directory; after it removes the sentinel file we'll start
> + * writing traces again.
> + */
> +static int tr2_dst_overloaded(const char *tgt_prefix)
> +{
> + int file_count = 0, max_files = 0, ret = 0;
> + const char *max_files_var;
> + DIR *dirp;
> + struct strbuf path = STRBUF_INIT, sentinel_path = STRBUF_INIT;
> + struct stat statbuf;
> +
> + /* Get the config or envvar and decide if we should continue this check */
> + max_files_var = tr2_sysenv_get(TR2_SYSENV_MAX_FILES);
> + if (max_files_var && *max_files_var && ((max_files = atoi(max_files_var)) >= 0))
> + tr2env_max_files = max_files;
> +
> + if (!tr2env_max_files) {
> + ret = 0;
> + goto cleanup;
> + }
> +
> + strbuf_addstr(&path, tgt_prefix);
> + if (!is_dir_sep(path.buf[path.len - 1])) {
> + strbuf_addch(&path, '/');
> + }
> +
> + /* check sentinel */
> + strbuf_addbuf(&sentinel_path, &path);
> + strbuf_addstr(&sentinel_path, OVERLOAD_SENTINEL_NAME);
> + if (!stat(sentinel_path.buf, &statbuf)) {
> + ret = 1;
> + goto cleanup;
> + }
> +
> + /* check file count */
> + dirp = opendir(path.buf);
> + while (file_count < tr2env_max_files && dirp && readdir(dirp))
> + file_count++;
> + if (dirp)
> + closedir(dirp);
> +
> + if (file_count >= tr2env_max_files) {
> + creat(sentinel_path.buf, 0666);
> + ret = 1;
> + goto cleanup;
> + }
> +
> +cleanup:
> + strbuf_release(&path);
> + strbuf_release(&sentinel_path);
> + return ret;
> +}
> +
> static int tr2_dst_try_auto_path(struct tr2_dst *dst, const char *tgt_prefix)
> {
> int fd;
> @@ -50,6 +126,16 @@ static int tr2_dst_try_auto_path(struct tr2_dst *dst, const char *tgt_prefix)
> strbuf_addstr(&path, sid);
> base_path_len = path.len;
>
> + if (tr2_dst_overloaded(tgt_prefix)) {
> + strbuf_release(&path);
> + if (tr2_dst_want_warning())
> + warning("trace2: not opening %s trace file due to too "
> + "many files in target directory %s",
> + tr2_sysenv_display_name(dst->sysenv_var),
> + tgt_prefix);
> + return 0;
> + }
> +
> for (attempt_count = 0; attempt_count < MAX_AUTO_ATTEMPTS; attempt_count++) {
> if (attempt_count > 0) {
> strbuf_setlen(&path, base_path_len);
> diff --git a/trace2/tr2_sysenv.c b/trace2/tr2_sysenv.c
> index 5958cfc424..3c3792eca2 100644
> --- a/trace2/tr2_sysenv.c
> +++ b/trace2/tr2_sysenv.c
> @@ -49,6 +49,9 @@ static struct tr2_sysenv_entry tr2_sysenv_settings[] = {
> "trace2.perftarget" },
> [TR2_SYSENV_PERF_BRIEF] = { "GIT_TRACE2_PERF_BRIEF",
> "trace2.perfbrief" },
> +
> + [TR2_SYSENV_MAX_FILES] = { "GIT_TRACE2_MAX_FILES",
> + "trace2.maxfiles" },
> };
> /* clang-format on */
>
> diff --git a/trace2/tr2_sysenv.h b/trace2/tr2_sysenv.h
> index 8dd82a7a56..d4364a7b85 100644
> --- a/trace2/tr2_sysenv.h
> +++ b/trace2/tr2_sysenv.h
> @@ -24,6 +24,8 @@ enum tr2_sysenv_variable {
> TR2_SYSENV_PERF,
> TR2_SYSENV_PERF_BRIEF,
>
> + TR2_SYSENV_MAX_FILES,
> +
> TR2_SYSENV_MUST_BE_LAST
> };
>
> --
> 2.23.0.581.g78d2f28ef7-goog
>
>