On Mon, May 24 2021, Lénaïc Huard wrote:
If we're going to use ifdefs then...
> +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.
