From: Jeff Hostetler <git@jeffhostetler.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
"Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, peff@peff.net, jrnieder@gmail.com,
steadmon@google.com, Junio C Hamano <gitster@pobox.com>,
Jeff Hostetler <jeffhost@microsoft.com>
Subject: Re: [PATCH 4/4] trace2: use system config for default trace2 settings
Date: Thu, 28 Mar 2019 14:50:08 -0400 [thread overview]
Message-ID: <f09d1d23-865f-fc35-250b-141a4d049029@jeffhostetler.com> (raw)
In-Reply-To: <87wokj9ic1.fsf@evledraar.gmail.com>
On 3/28/2019 10:36 AM, Ævar Arnfjörð Bjarmason wrote:
>
> On Thu, Mar 28 2019, Jeff Hostetler via GitGitGadget wrote:
>
> Thanks for working on this!
>
> Haven't given this any deep testing. Just some observations:
>
>> From: Jeff Hostetler <jeffhost@microsoft.com>
>>
>> Teach git to read the system config (usually "/etc/gitconfig") for
>> default Trace2 settings. This allows system-wide Trace2 settings to
>> be installed and inherited to make it easier to manage a collection of
>> systems.
[...]
>> diff --git a/trace2/tr2_sysenv.c b/trace2/tr2_sysenv.c
>> new file mode 100644
>> index 0000000000..656613e371
>> --- /dev/null
[...]
>> +/* clang-format off */
>> +static struct tr2_sysenv_entry tr2_sysenv_settings[] = {
>> + { "GIT_TR2_CONFIG_PARAMS", "trace2.configparams" },
>> +
>> + { "GIT_TR2_DST_DEBUG", "trace2.destinationdebug" },
>> +
>> + { "GIT_TR2", "trace2.normaltarget" },
>> + { "GIT_TR2_BRIEF", "trace2.normalbrief" },
>> +
>> + { "GIT_TR2_EVENT", "trace2.eventtarget" },
>> + { "GIT_TR2_EVENT_BRIEF", "trace2.eventbrief" },
>> + { "GIT_TR2_EVENT_NESTING", "trace2.eventnesting" },
>> +
>> + { "GIT_TR2_PERF", "trace2.perftarget" },
>> + { "GIT_TR2_PERF_BRIEF", "trace2.perfbrief" },
>> +};
>> +/* clang-format on */
>> +
>> +static int tr2_sysenv_cb(const char *key, const char *value, void *d)
>> +{
>> + int k;
>> +
>
> I added:
>
> if (!starts_with(key, "trace2."))
> return 0;
>
> Here, and everything works as expected. I think that's a good
> idea. Makes this O(n) over N config keys instead of O(n*x) where x = num
> entries in tr2_sysenv_settings.
Good idea. Thanks!
>
>> + for (k = 0; k < ARRAY_SIZE(tr2_sysenv_settings); k++) {
>> + if (!strcmp(key, tr2_sysenv_settings[k].git_config_name)) {
>> + free(tr2_sysenv_settings[k].value);
>> + tr2_sysenv_settings[k].value = xstrdup(value);
>> + return 0;
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +/*
>> + * Load Trace2 settings from the system config (usually "/etc/gitconfig"
>> + * unless we were built with a runtime-prefix). These are intended to
>> + * define the default values for Trace2 as requested by the administrator.
>> + */
>> +void tr2_sysenv_load(void)
>> +{
>> + const char *system_config_pathname;
>> + const char *test_pathname;
>> +
>> + system_config_pathname = git_etc_gitconfig();
>> +
>> + test_pathname = getenv("GIT_TEST_TR2_SYSTEM_CONFIG");
>> + if (test_pathname) {
>> + if (!*test_pathname || !strcmp(test_pathname, "0"))
>> + return; /* disable use of system config */
>> +
>> + /* mock it with given test file */
>> + system_config_pathname = test_pathname;
>> + }
>> +
>> + if (file_exists(system_config_pathname))
>> + git_config_from_file(tr2_sysenv_cb, system_config_pathname,
>> + NULL);
>
> Maybe this isn't worth it, but this "file_exists" thing is something we
> could abstract in the config machinery (or maybe passing via
> "config_options" makes more sense):
[...]
This is a good idea, but I think I'll save this for a future effort
rather than add it to the current patch series. It just seems outside
of my scope right now and adds to the footprint of this series.
[...]
>>
>> - nesting = getenv(TR2_ENVVAR_EVENT_NESTING);
>> + nesting = tr2_sysenv_get(TR2_SYSENV_EVENT_NESTING);
>> if (nesting && ((want_nesting = atoi(nesting)) > 0))
>> tr2env_event_nesting_wanted = want_nesting;
>>
>> - brief = getenv(TR2_ENVVAR_EVENT_BRIEF);
>> + brief = tr2_sysenv_get(TR2_SYSENV_EVENT_BRIEF);
>> if (brief && ((want_brief = atoi(brief)) > 0))
>> tr2env_event_brief = want_brief;
>
> A lot of this pre-dates this patch, but I wonder if the whole of trace2
> couldn't make more use of config.c's bool parsing for things like
> these. Maybe by having a "cfg_type" enum & parsed_value void* in
> tr2_sysenv_entry?
I converted the "brief" instances in the normal and perf targets to
use git_parse_maybe_bool() already, but I missed this one.
The nesting one above is actually an integer value rather than a bool.
I'll rename the variables in the re-roll to clarify that.
[...]
>> - brief = getenv(TR2_ENVVAR_NORMAL_BRIEF);
>> + brief = tr2_sysenv_get(TR2_SYSENV_NORMAL_BRIEF);
>> if (brief && *brief &&
>> ((want_brief = git_parse_maybe_bool(brief)) != -1))
>> tr2env_normal_brief = want_brief;
[...]
>> - brief = getenv(TR2_ENVVAR_PERF_BRIEF);
>> + brief = tr2_sysenv_get(TR2_SYSENV_PERF_BRIEF);
>> if (brief && *brief &&
>> ((want_brief = git_parse_maybe_bool(brief)) != -1))
>> tr2env_perf_brief = want_brief;
Thanks for the review.
I'll push up another version shortly.
Jeff
next prev parent reply other threads:[~2019-03-28 18:50 UTC|newest]
Thread overview: 68+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-03-28 13:30 [PATCH 0/4] trace2: load trace2 settings from system config Jeff Hostetler via GitGitGadget
2019-03-28 13:30 ` [PATCH 1/4] trace2: refactor setting process starting time Jeff Hostetler via GitGitGadget
2019-03-28 13:31 ` [PATCH 2/4] trace2: add absolute elapsed time to start event Jeff Hostetler via GitGitGadget
2019-03-28 13:31 ` [PATCH 3/4] trace2: find exec-dir before trace2 initialization Jeff Hostetler via GitGitGadget
2019-03-28 13:31 ` [PATCH 4/4] trace2: use system config for default trace2 settings Jeff Hostetler via GitGitGadget
2019-03-28 14:36 ` Ævar Arnfjörð Bjarmason
2019-03-28 18:50 ` Jeff Hostetler [this message]
2019-03-28 21:28 ` Josh Steadmon
2019-03-29 17:04 ` [PATCH v2 0/7] trace2: load trace2 settings from system config Jeff Hostetler via GitGitGadget
2019-03-29 17:04 ` [PATCH v2 1/7] trace2: refactor setting process starting time Jeff Hostetler via GitGitGadget
2019-03-29 17:04 ` [PATCH v2 2/7] trace2: add absolute elapsed time to start event Jeff Hostetler via GitGitGadget
2019-03-29 17:04 ` [PATCH v2 3/7] trace2: find exec-dir before trace2 initialization Jeff Hostetler via GitGitGadget
2019-03-29 17:04 ` [PATCH v2 4/7] trace2: use system config for default trace2 settings Jeff Hostetler via GitGitGadget
2019-04-01 21:00 ` Josh Steadmon
2019-04-01 21:06 ` Jeff Hostetler
2019-04-03 0:01 ` Jonathan Nieder
2019-04-03 0:00 ` Jonathan Nieder
2019-04-09 15:58 ` Jeff Hostetler
2019-03-29 17:04 ` [PATCH v2 5/7] trace2: report peak memory usage of the process Jeff Hostetler via GitGitGadget
2019-03-29 22:16 ` Ævar Arnfjörð Bjarmason
2019-04-01 21:05 ` Jeff Hostetler
2019-03-29 17:04 ` [PATCH v2 6/7] trace2: clarify UTC datetime formatting Jeff Hostetler via GitGitGadget
2019-03-29 17:04 ` [PATCH v2 7/7] trace2: make SIDs more unique Jeff Hostetler via GitGitGadget
2019-03-29 22:12 ` Ævar Arnfjörð Bjarmason
2019-04-01 21:16 ` Jeff Hostetler
2019-04-01 21:02 ` [PATCH v2 0/7] trace2: load trace2 settings from system config Josh Steadmon
2019-04-11 15:18 ` [PATCH v3 00/10] " Jeff Hostetler via GitGitGadget
2019-04-11 15:18 ` [PATCH v3 01/10] config: initialize opts structure in repo_read_config() Jeff Hostetler via GitGitGadget
2019-04-12 3:52 ` Jonathan Nieder
2019-04-15 14:34 ` Johannes Schindelin
2019-04-11 15:18 ` [PATCH v3 03/10] trace2: add absolute elapsed time to start event Jeff Hostetler via GitGitGadget
2019-04-11 15:18 ` [PATCH v3 02/10] trace2: refactor setting process starting time Jeff Hostetler via GitGitGadget
2019-04-11 15:18 ` [PATCH v3 04/10] trace2: find exec-dir before trace2 initialization Jeff Hostetler via GitGitGadget
2019-04-11 15:18 ` [PATCH v3 05/10] config: add read_very_early_config() Jeff Hostetler via GitGitGadget
2019-04-11 15:18 ` [PATCH v3 06/10] trace2: use system/global config for default trace2 settings Jeff Hostetler via GitGitGadget
2019-04-11 15:18 ` [PATCH v3 07/10] trace2: report peak memory usage of the process Jeff Hostetler via GitGitGadget
2019-04-11 15:18 ` [PATCH v3 08/10] trace2: clarify UTC datetime formatting Jeff Hostetler via GitGitGadget
2019-04-11 15:18 ` [PATCH v3 09/10] trace2: make SIDs more unique Jeff Hostetler via GitGitGadget
2019-04-11 15:18 ` [PATCH v3 10/10] trace2: update docs to describe system/global config settings Jeff Hostetler via GitGitGadget
2019-04-12 2:29 ` [PATCH v3 00/10] trace2: load trace2 settings from system config Junio C Hamano
2019-04-12 13:47 ` Jeff Hostetler
2019-04-15 20:39 ` [PATCH v4 " Jeff Hostetler via GitGitGadget
2019-04-15 20:39 ` [PATCH v4 01/10] config: initialize opts structure in repo_read_config() Jeff Hostetler via GitGitGadget
2019-04-15 20:39 ` [PATCH v4 02/10] trace2: refactor setting process starting time Jeff Hostetler via GitGitGadget
2019-04-15 20:39 ` [PATCH v4 03/10] trace2: add absolute elapsed time to start event Jeff Hostetler via GitGitGadget
2019-04-15 20:39 ` [PATCH v4 04/10] trace2: find exec-dir before trace2 initialization Jeff Hostetler via GitGitGadget
2019-04-15 20:39 ` [PATCH v4 05/10] config: add read_very_early_config() Jeff Hostetler via GitGitGadget
2019-04-15 20:39 ` [PATCH v4 06/10] trace2: use system/global config for default trace2 settings Jeff Hostetler via GitGitGadget
2019-04-27 13:43 ` SZEDER Gábor
2019-04-29 19:03 ` Jeff Hostetler
2019-04-15 20:39 ` [PATCH v4 08/10] trace2: clarify UTC datetime formatting Jeff Hostetler via GitGitGadget
2019-04-15 20:39 ` [PATCH v4 07/10] trace2: report peak memory usage of the process Jeff Hostetler via GitGitGadget
2019-04-15 20:39 ` [PATCH v4 09/10] trace2: make SIDs more unique Jeff Hostetler via GitGitGadget
2019-04-15 20:39 ` [PATCH v4 10/10] trace2: update docs to describe system/global config settings Jeff Hostetler via GitGitGadget
2019-04-29 20:14 ` [PATCH v5 00/11] trace2: load trace2 settings from system config Jeff Hostetler via GitGitGadget
2019-04-29 20:14 ` [PATCH v5 01/11] config: initialize opts structure in repo_read_config() Jeff Hostetler via GitGitGadget
2019-04-29 20:14 ` [PATCH v5 02/11] trace2: refactor setting process starting time Jeff Hostetler via GitGitGadget
2019-04-29 20:14 ` [PATCH v5 03/11] trace2: add absolute elapsed time to start event Jeff Hostetler via GitGitGadget
2019-04-29 20:14 ` [PATCH v5 04/11] trace2: find exec-dir before trace2 initialization Jeff Hostetler via GitGitGadget
2019-04-29 20:14 ` [PATCH v5 05/11] config: add read_very_early_config() Jeff Hostetler via GitGitGadget
2019-04-29 20:14 ` [PATCH v5 06/11] trace2: use system/global config for default trace2 settings Jeff Hostetler via GitGitGadget
2019-04-29 20:14 ` [PATCH v5 07/11] trace2: report peak memory usage of the process Jeff Hostetler via GitGitGadget
2019-04-29 20:14 ` [PATCH v5 08/11] trace2: clarify UTC datetime formatting Jeff Hostetler via GitGitGadget
2019-04-29 20:14 ` [PATCH v5 09/11] trace2: make SIDs more unique Jeff Hostetler via GitGitGadget
2019-04-29 20:14 ` [PATCH v5 10/11] trace2: update docs to describe system/global config settings Jeff Hostetler via GitGitGadget
2019-04-29 20:14 ` [PATCH v5 11/11] trace2: fixup access problem on /etc/gitconfig in read_very_early_config Jeff Hostetler via GitGitGadget
2019-04-29 20:21 ` [PATCH v5 00/11] trace2: load trace2 settings from system config Jeff Hostetler
2019-05-07 1:18 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f09d1d23-865f-fc35-250b-141a4d049029@jeffhostetler.com \
--to=git@jeffhostetler.com \
--cc=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=gitster@pobox.com \
--cc=jeffhost@microsoft.com \
--cc=jrnieder@gmail.com \
--cc=peff@peff.net \
--cc=steadmon@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).