Re: [PATCH 0/2] macos: safeguard git maintenance against highly concurrent operations
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 25, 2021, 00:49 UTC
- Message-ID
- <xmqqwnoajql1.fsf@gitster.g>
- In-Reply-To
- <pull.1024.git.1629819840.gitgitgadget@gmail.com>
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 8 quoted lines
> 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.
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);