Re: [PATCH v3 0/1] Write trace2 output to directories

[Jeff Hostetler <git@jeffhostetler.com>]

On 3/21/2019 5:09 PM, Josh Steadmon wrote:
> Persistently enabling trace2 output is difficult because it requires
> specifying a full filename. This series teaches tr2_dst_get_trace_fd()
> to create files underneath a given directory provided as the target of
> the GIT_TR2_* envvars.
> 
> Changes since V2:
> * No longer appends a suffix to filenames on the first creation attempt.
> * Renamed function & constant now that randomization is no longer used.
> * Simplified strbuf usage.
> 
> Changes since V1:
> * No longer supports timestamp templates.
> * No longer supports filename prefixes.
> * Always creates filenames based on the final component of the trace2
>    SID.
> 
> Josh Steadmon (1):
>    trace2: write to directory targets
> 
>   Documentation/technical/api-trace2.txt |  5 ++
>   t/t0210-trace2-normal.sh               | 15 ++++++
>   trace2/tr2_dst.c                       | 63 +++++++++++++++++++++++++-
>   3 files changed, 81 insertions(+), 2 deletions(-)
> 
> Range-diff against v2:
> 1:  59d8c6511b ! 1:  ce5258610f trace2: write to directory targets
>      @@ -23,8 +23,8 @@
>        
>       +	If the target already exists and is a directory, the traces will be
>       +	written to files (one per process) underneath the given directory. They
>      -+	will be named according to the last component of the SID followed by a
>      -+	counter to avoid potential collisions.
>      ++	will be named according to the last component of the SID (optionally
>      ++	followed by a counter to avoid filename collisions).
>       +
>        `af_unix:[<socket_type>:]<absolute-pathname>`::
>        
>      @@ -37,7 +37,7 @@
>        	test_cmp expect actual
>        '
>        
>      -+test_expect_success 'randomized filename' '
>      ++test_expect_success 'automatic filename' '
>       +	test_when_finished "rm -r traces actual expect" &&
>       +	mkdir traces &&
>       +	GIT_TR2="$(pwd)/traces" test-tool trace2 001return 0 &&
>      @@ -71,9 +71,9 @@
>        #define TR2_ENVVAR_DST_DEBUG "GIT_TR2_DST_DEBUG"
>        
>       +/*
>      -+ * How many attempts we will make at creating a random trace output path.
>      ++ * How many attempts we will make at creating an automatically-named trace file.
>       + */
>      -+#define MAX_RANDOM_ATTEMPTS 10
>      ++#define MAX_AUTO_ATTEMPTS 10
>       +
>        static int tr2_dst_want_warning(void)
>        {
>      @@ -82,45 +82,47 @@
>        	dst->need_close = 0;
>        }
>        
>      -+static int tr2_dst_try_random_path(struct tr2_dst *dst, const char *tgt_prefix)
>      ++static int tr2_dst_try_auto_path(struct tr2_dst *dst, const char *tgt_prefix)
>       +{
>       +	int fd;
>       +	const char *last_slash, *sid = tr2_sid_get();
>      -+	struct strbuf base_path = STRBUF_INIT, final_path = STRBUF_INIT;
>      ++	struct strbuf path = STRBUF_INIT;
>      ++	size_t base_path_len;
>       +	unsigned attempt_count;
>       +
>       +	last_slash = strrchr(sid, '/');
>       +	if (last_slash)
>       +		sid = last_slash + 1;
>       +
>      -+	strbuf_addstr(&base_path, tgt_prefix);
>      -+	if (!is_dir_sep(base_path.buf[base_path.len - 1]))
>      -+		strbuf_addch(&base_path, '/');
>      -+	strbuf_addstr(&base_path, sid);
>      ++	strbuf_addstr(&path, tgt_prefix);
>      ++	if (!is_dir_sep(path.buf[path.len - 1]))
>      ++		strbuf_addch(&path, '/');
>      ++	strbuf_addstr(&path, sid);
>      ++	base_path_len = path.len;
>       +
>      -+	for (attempt_count = 0; attempt_count < MAX_RANDOM_ATTEMPTS; attempt_count++) {
>      -+		strbuf_reset(&final_path);
>      -+		strbuf_addbuf(&final_path, &base_path);
>      -+		strbuf_addf(&final_path, ".%d", attempt_count);
>      ++	for (attempt_count = 0; attempt_count < MAX_AUTO_ATTEMPTS; attempt_count++) {
>      ++		if (attempt_count > 0) {
>      ++			strbuf_setlen(&path, base_path_len);
>      ++			strbuf_addf(&path, ".%d", attempt_count);
>      ++		}
>       +
>      -+		fd = open(final_path.buf, O_WRONLY | O_CREAT | O_EXCL, 0666);
>      ++		fd = open(path.buf, O_WRONLY | O_CREAT | O_EXCL, 0666);
>       +		if (fd != -1)
>       +			break;
>       +	}
>       +
>       +	if (fd == -1) {
>       +		if (tr2_dst_want_warning())
>      -+			warning("trace2: could not open '%s' for '%s' tracing: %s",
>      -+				base_path.buf, dst->env_var_name, strerror(errno));
>      ++			warning("trace2: could not open '%.*s' for '%s' tracing: %s",
>      ++				(int) base_path_len, path.buf,
>      ++				dst->env_var_name, strerror(errno));
>       +
>       +		tr2_dst_trace_disable(dst);
>      -+		strbuf_release(&base_path);
>      -+		strbuf_release(&final_path);
>      ++		strbuf_release(&path);
>       +		return 0;
>       +	}
>       +
>      -+	strbuf_release(&base_path);
>      -+	strbuf_release(&final_path);
>      ++	strbuf_release(&path);
>       +
>       +	dst->fd = fd;
>       +	dst->need_close = 1;
>      @@ -140,7 +142,7 @@
>       -		return tr2_dst_try_path(dst, tgt_value);
>       +	if (is_absolute_path(tgt_value)) {
>       +		if (is_directory(tgt_value))
>      -+			return tr2_dst_try_random_path(dst, tgt_value);
>      ++			return tr2_dst_try_auto_path(dst, tgt_value);
>       +		else
>       +			return tr2_dst_try_path(dst, tgt_value);
>       +	}
> 

Looks good.  Thanks.
Jeff