On Wed, Oct 05 2022, Junio C Hamano wrote:
> "Jeff Hostetler via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
>
>> From: Jeff Hostetler <jeffhost@xxxxxxxxxxxxx>
>>
>> Convert the `tr2tls_thread_ctx.thread_name` field from a `strbuf`
>> to a "flex array" at the end of the context structure.
>>
>> The `thread_name` field is a constant string that is constructed when
>> the context is created. Using a (non-const) `strbuf` structure for it
>> caused some confusion in the past because it implied that someone
>> could rename a thread after it was created. That usage was not
>> intended. Changing it to a "flex array" will hopefully make the
>> intent more clear.
>
> Surely, "const struct strbuf name;" member would be an oxymoron, and
> I agree that this should follow "use strbuf as an easy-to-work-with
> mechanism to come up with a string, and bake the final value into a
> struct as a member of type 'const char []'" pattern.
>
> I recall saying why I thought the flex array was overkill, though.
>
> You have been storing an up-to-24-byte human readable name by
> embedding a strbuf that has two size_t plus a pointer (i.e. 24-bytes
> even on Windows), and as TR2_MAX_THREAD_NAME is capped at 24 bytes
> anyway, an embedded fixed-size thread_name[TR2_MAX_THREAD_NAME+1]
> member may be the simplest thing to do, I suspect.
>
> If we were to allow arbitrarily long thread_name[], which may not be
> a bad thing to do (e.g. we do not have to worry about truncation
> making two names ambiguous, for example), then the flex array is the
> right direction to go in, though.
We don't even need that, AFAICT. My reply at [1] is rather long, but the
tl;dr is that the interface for this API is:
$ git grep '^\s+trace2_thread_start'
Documentation/technical/api-trace2.txt: trace2_thread_start("preload_thread");
builtin/fsmonitor--daemon.c: trace2_thread_start("fsm-health");
builtin/fsmonitor--daemon.c: trace2_thread_start("fsm-listen");
compat/simple-ipc/ipc-unix-socket.c: trace2_thread_start("ipc-worker");
compat/simple-ipc/ipc-unix-socket.c: trace2_thread_start("ipc-accept");
compat/simple-ipc/ipc-win32.c: trace2_thread_start("ipc-server");
t/helper/test-fsmonitor-client.c: trace2_thread_start("hammer");
t/helper/test-simple-ipc.c: trace2_thread_start("multiple");
trace2.h: trace2_thread_start_fl((thread_hint), __FILE__, __LINE__)
And we are taking e.g. "preload_thread" and turning it into strings like
these, and saving it into "struct tr2tls_thread_ctx".
"preload_thread", // main thread
"th01:preload_thread", // 1st thread
"th02:preload_thread" // 2nd thread
[...]
So, we don't need to strdup() and store that "preload_thread" anywhere.
It's already a constant string we have hardcoded in the program. We just
need to save a pointer to it.
Then we just format the "%s" or (if ".thread_id" == 0) or "th%02d:%s"
(if ".thread_id" > 0) on-the-fly, the two codepaths that end up using
this are already using strbuf_addf(), so just adding to the format there
is easy.
1. https://lore.kernel.org/git/221005.86y1tus9ps.gmgdl@xxxxxxxxxxxxxxxxxxx/
