Re: [PATCH 2/9] trace2: convert tr2tls_thread_ctx.thread_name from strbuf to char*

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 21, 2021, 07:22 UTC
Message-ID: <xmqqsfumqv62.fsf@gitster.g>
In-Reply-To: <3a4fe07e40e967622035844ff10ded1ed71d94fc.1640012469.git.gitgitgadget@gmail.com>

"Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 6 quoted lines

> From: Jeff Hostetler <jeffhost@microsoft.com>
>
> Use a 'char *' to hold the thread name rather than a 'struct strbuf'.
> The thread name is set when the thread is created and should not be
> be modified afterwards.  Replace the strbuf with an allocated pointer
> to make that more clear.

Sounds good. Use of strbuf is perfectly fine while you compute the final value of the string, but as a more permanent location to store the result, it often is unsuitable (and strbuf_split_buf() is a prime example of how *not* to design your API function around the type).

Show 28 quoted lines

> diff --git a/trace2/tr2_tls.c b/trace2/tr2_tls.c
> index 7da94aba522..cd8b9f2f0a0 100644
> --- a/trace2/tr2_tls.c
> +++ b/trace2/tr2_tls.c
> @@ -35,6 +35,7 @@ struct tr2tls_thread_ctx *tr2tls_create_self(const char *thread_name,
>  					     uint64_t us_thread_start)
>  {
>  	struct tr2tls_thread_ctx *ctx = xcalloc(1, sizeof(*ctx));
> +	struct strbuf buf_name = STRBUF_INIT;
>  
>  	/*
>  	 * Implicitly "tr2tls_push_self()" to capture the thread's start
> @@ -47,12 +48,13 @@ struct tr2tls_thread_ctx *tr2tls_create_self(const char *thread_name,
>  
>  	ctx->thread_id = tr2tls_locked_increment(&tr2_next_thread_id);
>  
> -	strbuf_init(&ctx->thread_name, 0);
>  	if (ctx->thread_id)
> -		strbuf_addf(&ctx->thread_name, "th%02d:", ctx->thread_id);
> -	strbuf_addstr(&ctx->thread_name, thread_name);
> -	if (ctx->thread_name.len > TR2_MAX_THREAD_NAME)
> -		strbuf_setlen(&ctx->thread_name, TR2_MAX_THREAD_NAME);
> +		strbuf_addf(&buf_name, "th%02d:", ctx->thread_id);
> +	strbuf_addstr(&buf_name, thread_name);
> +	if (buf_name.len > TR2_MAX_THREAD_NAME)
> +		strbuf_setlen(&buf_name, TR2_MAX_THREAD_NAME);
> +
> +	ctx->thread_name = strbuf_detach(&buf_name, NULL);

This is not exactly a new problem, but if we use a mechanism to allow arbitrary long string (like composing with strbuf and detaching the resulting string as is), instead of having a fixed name[] array embedded in the ctx structure, I wonder if applying the maximum length this early makes sense. Such a truncation would allow more than one ctx structures to share the same name, which somehow feels error prone, inviting a mistake to use .thread_name member as an identifier, when its only intended use is to give a human-readable and not necessarily unique label. Of course, if the maximum is reasonably low, like a few dozen bytes, it may even make sense to embed an array of the fixed size and not worry about an extra pointer.

Show 10 quoted lines

> diff --git a/trace2/tr2_tls.h b/trace2/tr2_tls.h
> index a90bd639d48..d968da6a679 100644
> --- a/trace2/tr2_tls.h
> +++ b/trace2/tr2_tls.h
> @@ -9,7 +9,7 @@
>  #define TR2_MAX_THREAD_NAME (24)
>  
>  struct tr2tls_thread_ctx {
> -	struct strbuf thread_name;
> +	char *thread_name;

That is, something like

	char thread_name[TR2_MAX_THREAD_NAME + 1];

perhaps with moving it to the end of the struct to avoid padding waste, would make more sense than the posted patch, if we accept an early truncation and information loss.

The other extreme would also make equally more sense than the posted patch. Just grab strbuf_detach() result without truncation and point at it with "char *thread_name" here, and if the output layer wants to limit the names to some reasonable length, deal with the TR2_MAX_THREAD_NAME at that layer, without losing information too early. It might be a much bigger surgery, I am afraid, because the users of ctx->thread_name (and old ctx->thread_name.buf) all are relying on the string being shorter than TR2_MAX_THREAD_NAME.

>  	uint64_t *array_us_start;
>  	size_t alloc;
>  	size_t nr_open_regions; /* plays role of "nr" in ALLOC_GROW */