Re: [PATCH v4 4/4] maintenance: add support for systemd timers on Linux
From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2021-05-24 10:03:07
On Mon, May 24 2021, Lénaïc Huard wrote: If we're going to use ifdefs then...
quoted hunk ↗ jump to hunk
+static int is_systemd_timer_available(void) +{ + const char *cmd = "systemctl"; + int is_available; + static int cached_result = -1; +#ifdef __linux__ + struct child_process child = CHILD_PROCESS_INIT; +#endif + + if (cached_result != -1) + return cached_result; + + if (get_schedule_cmd(&cmd, &is_available)) + return is_available; + +#ifdef __linux__ + strvec_split(&child.args, cmd); + strvec_pushl(&child.args, "--user", "list-timers", NULL); + child.no_stdin = 1; + child.no_stdout = 1; + child.no_stderr = 1; + child.silent_exec_failure = 1; + + if (start_command(&child)) { + cached_result = 0; + return cached_result; + } + if (finish_command(&child)) { + cached_result = 0; + return cached_result; + } + cached_result = 1; + return cached_result; +#else + return 0; +#endif +}
This sort of function would, I think, be clearer if we just had two different functions in an ifdef/else, e.g. here cached_result" is checked under !__linux__, but is never set in that case. But aside from that, why we do we need all this is_available caching, isn't this all called as a one-off? If it's not, it seems much better to push that cache one level up the callstack. Have whatevere's keeping track of the struct cache is_available itself, or cache it in another (or the same, then tristate) field in the same struct.
[...] +test_expect_success 'start and stop Linux/systemd maintenance' ' + write_script print-args <<-\EOF && + printf "%s\n" "$*" >>args + EOF + + XDG_CONFIG_HOME="$PWD" && + export XDG_CONFIG_HOME && + rm -f args &&
If you're going to care about cleanup here, and personally I wouldn't,
just call that "args" by the name "expect" instead (as is convention)
and clobber it every time...
Anyway, a better way to do the cleanup is:
test_when_finished "rm args" &&
echo this is the first time you write the file >args
[the rest of the test code]
Then you don't need to re-rm it.