Re: [PATCH 0/2] macos: safeguard git maintenance against highly concurrent operations
From: Johannes Schindelin <hidden>
Date: 2021-08-25 12:23:19
Hi Junio, On Tue, 24 Aug 2021, Junio C Hamano wrote:
"Johannes Schindelin via GitGitGadget" [off-list ref] writes:quoted
Please note that this patch series conflicts with lh/systemd-timers, although in a trivial way: the latter changes the signature of launchctl_schedule_plist() to lose its cmd parameter. The resolution is to adjust the conflicting code to lose the cmd parameter, and also drop it from launchctl_list_contains_plist() (and define it in the same way as launchctl_boot_plist() does). I assume that lh/systemd-timers will advance to next pretty soon; I plan on rebasing this patch series on top of it at that stage.Sounds like a plan. Here is my attempt to merge lh/systemd-timers into the result of applying these two to 'master', with focus on the top part of the launchctl_schedule_plist(). Sanity-checking is appreciated.
My local version (hence `git reset -hard`'ed away) looked almost precisely like yours, I only added the definition of `cmd` to the top of `launchctl_list_contains_plist()` and removed its `cmd` parameter (and adjusted the callers). But that's the only difference I can spot. Thanks, Dscho
quoted hunk ↗ jump to hunk
diff --cc builtin/gc.c index 22e670b508,6a57d0fde5..0000000000--- i/builtin/gc.c +++ w/builtin/gc.c@@@ -1593,48 -1678,26 +1678,50 @@@ static int launchctl_remove_plist(enum return result; } - static int launchctl_remove_plists(const char *cmd) + static int launchctl_remove_plists(void) { - return launchctl_remove_plist(SCHEDULE_HOURLY, cmd) || - launchctl_remove_plist(SCHEDULE_DAILY, cmd) || - launchctl_remove_plist(SCHEDULE_WEEKLY, cmd); + return launchctl_remove_plist(SCHEDULE_HOURLY) || + launchctl_remove_plist(SCHEDULE_DAILY) || + launchctl_remove_plist(SCHEDULE_WEEKLY); } +static int launchctl_list_contains_plist(const char *name, const char *cmd) +{ + int result; + struct child_process child = CHILD_PROCESS_INIT; + char *uid = launchctl_get_uid(); + + strvec_split(&child.args, cmd); + strvec_pushl(&child.args, "list", name, NULL); + + child.no_stderr = 1; + child.no_stdout = 1; + + if (start_command(&child)) + die(_("failed to start launchctl")); + + result = finish_command(&child); + + free(uid); + + /* Returns failure if 'name' doesn't exist. */ + return !result; +} + - static int launchctl_schedule_plist(const char *exec_path, enum schedule_priority schedule, const char *cmd) + static int launchctl_schedule_plist(const char *exec_path, enum schedule_priority schedule) { - FILE *plist; - int i; + int i, fd; const char *preamble, *repeat; const char *frequency = get_frequency(schedule); char *name = launchctl_service_name(frequency); char *filename = launchctl_service_filename(name); + struct lock_file lk = LOCK_INIT; + static unsigned long lock_file_timeout_ms = ULONG_MAX; + struct strbuf plist = STRBUF_INIT, plist2 = STRBUF_INIT; + struct stat st; ++ const char *cmd = "launchctl"; - if (safe_create_leading_directories(filename)) - die(_("failed to create directories for '%s'"), filename); - plist = xfopen(filename, "w"); - ++ get_schedule_cmd(&cmd, NULL); preamble = "<?xml version=\"1.0\"?>\n" "<!DOCTYPE plist PUBLIC \"-//Apple//DTD PLIST 1.0//EN\" \"http://www.apple.com/DTDs/PropertyList-1.0.dtd\">\n" "<plist version=\"1.0\">" @@@ -1687,38 -1750,13 +1774,38 @@@ /* unreachable */ break; } - fprintf(plist, "</array>\n</dict>\n</plist>\n"); - fclose(plist); + strbuf_addstr(&plist, "</array>\n</dict>\n</plist>\n"); - /* bootout might fail if not already running, so ignore */ - launchctl_boot_plist(0, filename); - if (launchctl_boot_plist(1, filename)) - die(_("failed to bootstrap service %s"), filename); + if (safe_create_leading_directories(filename)) + die(_("failed to create directories for '%s'"), filename); + + if ((long)lock_file_timeout_ms < 0 && + git_config_get_ulong("gc.launchctlplistlocktimeoutms", + &lock_file_timeout_ms)) + lock_file_timeout_ms = 150; + + fd = hold_lock_file_for_update_timeout(&lk, filename, LOCK_DIE_ON_ERROR, + lock_file_timeout_ms); + + /* + * Does this file already exist? With the intended contents? Is it + * registered already? Then it does not need to be re-registered. + */ + if (!stat(filename, &st) && st.st_size == plist.len && + strbuf_read_file(&plist2, filename, plist.len) == plist.len && + !strbuf_cmp(&plist, &plist2) && + launchctl_list_contains_plist(name, cmd)) + rollback_lock_file(&lk); + else { + if (write_in_full(fd, plist.buf, plist.len) < 0 || + commit_lock_file(&lk)) + die_errno(_("could not write '%s'"), filename); + + /* bootout might fail if not already running, so ignore */ - launchctl_boot_plist(0, filename, cmd); - if (launchctl_boot_plist(1, filename, cmd)) ++ launchctl_boot_plist(0, filename); ++ if (launchctl_boot_plist(1, filename)) + die(_("failed to bootstrap service %s"), filename); + } free(filename); free(name);