| Message ID | 37fdce5ec3afaa9aae5001c648fced0675dae0c4.1625150864.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Builtin FSMonitor Feature | expand |
On Thu, Jul 01 2021, Jeff Hostetler via GitGitGadget wrote: > + if (!test_env_value) { > + struct timeval tv; > + struct tm tm; > + time_t secs; > + > + gettimeofday(&tv, NULL); > + secs = tv.tv_sec; > + gmtime_r(&secs, &tm); > + > + strbuf_addf(&token->token_id, > + "%"PRIu64".%d.%4d%02d%02dT%02d%02d%02d.%06ldZ", > + flush_count++, > + getpid(), > + tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday, > + tm.tm_hour, tm.tm_min, tm.tm_sec, > + (long)tv.tv_usec); Just bikeshedding, but can we have tokens that mostly sort numeric-wise by time order? So time at the start, not the flush_count/getpid. Maybe I'm missing something, but couldn't we just re-use the trace2 SID + a more trivial trailer? It would have the nice property that you could find the trace2 SID whenever you looked at such a token (could e.g. split them by "/" too), and add the tv_usec, flush_count+whatever else is needed to make it unique after the "/", no?
On 7/1/21 6:58 PM, Ævar Arnfjörð Bjarmason wrote: > > On Thu, Jul 01 2021, Jeff Hostetler via GitGitGadget wrote: > >> + if (!test_env_value) { >> + struct timeval tv; >> + struct tm tm; >> + time_t secs; >> + >> + gettimeofday(&tv, NULL); >> + secs = tv.tv_sec; >> + gmtime_r(&secs, &tm); >> + >> + strbuf_addf(&token->token_id, >> + "%"PRIu64".%d.%4d%02d%02dT%02d%02d%02d.%06ldZ", >> + flush_count++, >> + getpid(), >> + tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday, >> + tm.tm_hour, tm.tm_min, tm.tm_sec, >> + (long)tv.tv_usec); > > Just bikeshedding, but can we have tokens that mostly sort numeric-wise > by time order? So time at the start, not the flush_count/getpid. As I described in a rather large comment in the code, tokens are opaque strings -- without a less-than / greater-than relationship -- just a random string that the daemon can use (along with a sequence number) to ensure that a later request is well-defined. Here I'm using a counter, pid, and date-stamp. I'd prefer using a GUID or UUID just to drive that home, but I didn't want to add a new .lib or .a to the build if not necessary. Perhaps I should compute this portion as hex(hash(time())) to remove the temptation to look inside my opaque token ?? > > Maybe I'm missing something, but couldn't we just re-use the trace2 SID > + a more trivial trailer? It would have the nice property that you could > find the trace2 SID whenever you looked at such a token (could > e.g. split them by "/" too), and add the tv_usec, flush_count+whatever > else is needed to make it unique after the "/", no? > I would rather keep Trace2 out of this. The SID is another opaque string and I don't want to reach inside it. Jeff
On Tue, Jul 13 2021, Jeff Hostetler wrote: > On 7/1/21 6:58 PM, Ævar Arnfjörð Bjarmason wrote: >> On Thu, Jul 01 2021, Jeff Hostetler via GitGitGadget wrote: >> >>> + if (!test_env_value) { >>> + struct timeval tv; >>> + struct tm tm; >>> + time_t secs; >>> + >>> + gettimeofday(&tv, NULL); >>> + secs = tv.tv_sec; >>> + gmtime_r(&secs, &tm); >>> + >>> + strbuf_addf(&token->token_id, >>> + "%"PRIu64".%d.%4d%02d%02dT%02d%02d%02d.%06ldZ", >>> + flush_count++, >>> + getpid(), >>> + tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday, >>> + tm.tm_hour, tm.tm_min, tm.tm_sec, >>> + (long)tv.tv_usec); >> Just bikeshedding, but can we have tokens that mostly sort >> numeric-wise >> by time order? So time at the start, not the flush_count/getpid. > > As I described in a rather large comment in the code, tokens are opaque > strings -- without a less-than / greater-than relationship -- just a > random string that the daemon can use (along with a sequence number) to > ensure that a later request is well-defined. > > Here I'm using a counter, pid, and date-stamp. I'd prefer using a GUID > or UUID just to drive that home, but I didn't want to add a new .lib or > .a to the build if not necessary. > > Perhaps I should compute this portion as hex(hash(time())) to remove the > temptation to look inside my opaque token ?? Why does it matter if someone looks inside your opaque token if the code is treating it as opaque by just doing a strcmp(old,new) on it? I just suggested this as a debugging aid, i.e. when you the human (as opposed to the program) are looking at this behavior it's handy to look at the token and see that your cookies don't match, and that they look to be N seconds apart. And furthermore, if git crashes or whatever you can now easily look up what process crashed if you've got the leftover cookie, if you've also got trace2 logs. >> Maybe I'm missing something, but couldn't we just re-use the trace2 >> SID >> + a more trivial trailer? It would have the nice property that you could >> find the trace2 SID whenever you looked at such a token (could >> e.g. split them by "/" too), and add the tv_usec, flush_count+whatever >> else is needed to make it unique after the "/", no? >> > > I would rather keep Trace2 out of this. The SID is another opaque > string and I don't want to reach inside it. For the purposes of the git.git codebase it's fine to reach inside of it, especially for a "I'd like a near-enough-UUID, and I know the trace2 SID already does that per-program", so you just need e.g. a sequence counter within the program to ensure global uniqueness with other git processes for such a cookie.
diff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c index e942f7c5840..e991925fafc 100644 --- a/builtin/fsmonitor--daemon.c +++ b/builtin/fsmonitor--daemon.c @@ -93,6 +93,120 @@ static int do_as_client__status(void) } } +/* + * Requests to and from a FSMonitor Protocol V2 provider use an opaque + * "token" as a virtual timestamp. Clients can request a summary of all + * created/deleted/modified files relative to a token. In the response, + * clients receive a new token for the next (relative) request. + * + * + * Token Format + * ============ + * + * The contents of the token are private and provider-specific. + * + * For the built-in fsmonitor--daemon, we define a token as follows: + * + * "builtin" ":" <token_id> ":" <sequence_nr> + * + * The "builtin" prefix is used as a namespace to avoid conflicts + * with other providers (such as Watchman). + * + * The <token_id> is an arbitrary OPAQUE string, such as a GUID, + * UUID, or {timestamp,pid}. It is used to group all filesystem + * events that happened while the daemon was monitoring (and in-sync + * with the filesystem). + * + * Unlike FSMonitor Protocol V1, it is not defined as a timestamp + * and does not define less-than/greater-than relationships. + * (There are too many race conditions to rely on file system + * event timestamps.) + * + * The <sequence_nr> is a simple integer incremented whenever the + * daemon needs to make its state public. For example, if 1000 file + * system events come in, but no clients have requested the data, + * the daemon can continue to accumulate file changes in the same + * bin and does not need to advance the sequence number. However, + * as soon as a client does arrive, the daemon needs to start a new + * bin and increment the sequence number. + * + * The sequence number serves as the boundary between 2 sets + * of bins -- the older ones that the client has already seen + * and the newer ones that it hasn't. + * + * When a new <token_id> is created, the <sequence_nr> is reset to + * zero. + * + * + * About Token Ids + * =============== + * + * A new token_id is created: + * + * [1] each time the daemon is started. + * + * [2] any time that the daemon must re-sync with the filesystem + * (such as when the kernel drops or we miss events on a very + * active volume). + * + * [3] in response to a client "flush" command (for dropped event + * testing). + * + * When a new token_id is created, the daemon is free to discard all + * cached filesystem events associated with any previous token_ids. + * Events associated with a non-current token_id will never be sent + * to a client. A token_id change implicitly means that the daemon + * has gap in its event history. + * + * Therefore, clients that present a token with a stale (non-current) + * token_id will always be given a trivial response. + */ +struct fsmonitor_token_data { + struct strbuf token_id; + struct fsmonitor_batch *batch_head; + struct fsmonitor_batch *batch_tail; + uint64_t client_ref_count; +}; + +static struct fsmonitor_token_data *fsmonitor_new_token_data(void) +{ + static int test_env_value = -1; + static uint64_t flush_count = 0; + struct fsmonitor_token_data *token; + + CALLOC_ARRAY(token, 1); + + strbuf_init(&token->token_id, 0); + token->batch_head = NULL; + token->batch_tail = NULL; + token->client_ref_count = 0; + + if (test_env_value < 0) + test_env_value = git_env_bool("GIT_TEST_FSMONITOR_TOKEN", 0); + + if (!test_env_value) { + struct timeval tv; + struct tm tm; + time_t secs; + + gettimeofday(&tv, NULL); + secs = tv.tv_sec; + gmtime_r(&secs, &tm); + + strbuf_addf(&token->token_id, + "%"PRIu64".%d.%4d%02d%02dT%02d%02d%02d.%06ldZ", + flush_count++, + getpid(), + tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday, + tm.tm_hour, tm.tm_min, tm.tm_sec, + (long)tv.tv_usec); + } else { + strbuf_addf(&token->token_id, "test_%08x", test_env_value++); + } + + return token; +} + static ipc_server_application_cb handle_client; static int handle_client(void *data, @@ -283,7 +397,7 @@ static int fsmonitor_run_daemon(void) pthread_mutex_init(&state.main_lock, NULL); state.error_code = 0; - state.current_token_data = NULL; + state.current_token_data = fsmonitor_new_token_data(); /* Prepare to (recursively) watch the <worktree-root> directory. */ strbuf_init(&state.path_worktree_watch, 0);