Re: [PATCH v4 4/4] maintenance: add support for systemd timers on Linux
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- May 24, 2021, 09:55 UTC
- Message-ID
- <87r1hwo3e3.fsf@evledraar.gmail.com>
- In-Reply-To
- <20210524071538.46862-5-lenaic@lhuard.fr>
On Mon, May 24 2021, Lénaïc Huard wrote:
If we're going to use ifdefs then...
Show 37 quoted lines
> +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.
Show 9 quoted lines
> [...] > +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.