Re: [PATCH 3/7] fsmonitor: implement filesystem change listener for Linux

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 22837 bytes --]

On Thu, Feb 15, 2024 at 10:29:34AM +0000, Eric DeCosta via GitGitGadget wrote:
> From: Eric DeCosta <edecosta@mathworks.com>
> 
> Implement a filesystem change listener for Linux based on the inotify API:
> https://man7.org/linux/man-pages/man7/inotify.7.html
> 
> inotify requires registering a watch on every directory in the worktree and
> special handling of moves/renames.

I assume that fsmonitor is especially important in the context of repos
with large trees, and to the best of my knowledge inotify(7) does not
scale well when installing many watches. I thus have to wonder whether
fanotify(7) would be a better match to implement this nowadays, and what
the considerations were to pick one over the other.

> Signed-off-by: Eric DeCosta <edecosta@mathworks.com>
> ---
>  compat/fsmonitor/fsm-listen-linux.c | 676 ++++++++++++++++++++++++++++
>  1 file changed, 676 insertions(+)
>  create mode 100644 compat/fsmonitor/fsm-listen-linux.c
> 
> diff --git a/compat/fsmonitor/fsm-listen-linux.c b/compat/fsmonitor/fsm-listen-linux.c
> new file mode 100644
> index 00000000000..e8548e4e009
> --- /dev/null
> +++ b/compat/fsmonitor/fsm-listen-linux.c
> @@ -0,0 +1,676 @@
> +#include "cache.h"
> +#include "fsmonitor.h"
> +#include "fsm-listen.h"
> +#include "fsmonitor--daemon.h"
> +#include <dirent.h>
> +#include <fcntl.h>
> +#include <sys/inotify.h>
> +#include <sys/stat.h>
> +
> +/*
> + * Safe value to bitwise OR with rest of mask for
> + * kernels that do not support IN_MASK_CREATE
> + */
> +#ifndef IN_MASK_CREATE
> +#define IN_MASK_CREATE 0x00000000
> +#endif
> +
> +enum shutdown_reason {
> +	SHUTDOWN_CONTINUE = 0,
> +	SHUTDOWN_STOP,
> +	SHUTDOWN_ERROR,
> +	SHUTDOWN_FORCE
> +};
> +
> +struct watch_entry {
> +	struct hashmap_entry ent;
> +	int wd;
> +	uint32_t cookie;
> +	const char *dir;
> +};
> +
> +struct rename_entry {
> +	struct hashmap_entry ent;
> +	time_t whence;
> +	uint32_t cookie;
> +	const char *dir;
> +};
> +
> +struct fsm_listen_data {
> +	int fd_inotify;
> +	enum shutdown_reason shutdown;
> +	struct hashmap watches;
> +	struct hashmap renames;
> +	struct hashmap revwatches;
> +};
> +
> +static int watch_entry_cmp(const void *cmp_data,
> +			  const struct hashmap_entry *eptr,
> +			  const struct hashmap_entry *entry_or_key,
> +			  const void *keydata)
> +{
> +	const struct watch_entry *e1, *e2;
> +
> +	e1 = container_of(eptr, const struct watch_entry, ent);
> +	e2 = container_of(eptr, const struct watch_entry, ent);
> +	return e1->wd != e2->wd;
> +}
> +
> +static int revwatches_entry_cmp(const void *cmp_data,
> +			  const struct hashmap_entry *eptr,
> +			  const struct hashmap_entry *entry_or_key,
> +			  const void *keydata)
> +{
> +	const struct watch_entry *e1, *e2;
> +
> +	e1 = container_of(eptr, const struct watch_entry, ent);
> +	e2 = container_of(eptr, const struct watch_entry, ent);
> +	return strcmp(e1->dir, e2->dir);
> +}
> +
> +static int rename_entry_cmp(const void *cmp_data,
> +			  const struct hashmap_entry *eptr,
> +			  const struct hashmap_entry *entry_or_key,
> +			  const void *keydata)
> +{
> +	const struct rename_entry *e1, *e2;
> +
> +	e1 = container_of(eptr, const struct rename_entry, ent);
> +	e2 = container_of(eptr, const struct rename_entry, ent);
> +	return e1->cookie != e2->cookie;
> +}
> +
> +/*
> + * Register an inotify watch, add watch descriptor to path mapping
> + * and the reverse mapping.
> + */
> +static int add_watch(const char *path, struct fsm_listen_data *data)
> +{
> +	const char *interned = strintern(path);
> +	struct watch_entry *w1, *w2;
> +
> +	/* add the inotify watch, don't allow watches to be modified */
> +	int wd = inotify_add_watch(data->fd_inotify, interned,
> +				(IN_ALL_EVENTS | IN_ONLYDIR | IN_MASK_CREATE)
> +				^ IN_ACCESS ^ IN_CLOSE ^ IN_OPEN);
> +	if (wd < 0)
> +		return error_errno("inotify_add_watch('%s') failed", interned);
> +
> +	/* add watch descriptor -> directory mapping */
> +	CALLOC_ARRAY(w1, 1);
> +	w1->wd = wd;
> +	w1->dir = interned;
> +	hashmap_entry_init(&w1->ent, memhash(&w1->wd, sizeof(int)));
> +	hashmap_add(&data->watches, &w1->ent);
> +
> +	/* add directory -> watch descriptor mapping */
> +	CALLOC_ARRAY(w2, 1);
> +	w2->wd = wd;
> +	w2->dir = interned;
> +	hashmap_entry_init(&w2->ent, memhash(w2->dir, strlen(w2->dir)));
> +	hashmap_add(&data->revwatches, &w2->ent);
> +
> +	return 0;
> +}
> +
> +/*
> + * Remove the inotify watch, the watch descriptor to path mapping
> + * and the reverse mapping.
> + */
> +static void remove_watch(struct watch_entry *w,
> +	struct fsm_listen_data *data)
> +{
> +	struct watch_entry k1, k2, *w1, *w2;
> +
> +	/* remove watch, ignore error if kernel already did it */
> +	if (inotify_rm_watch(data->fd_inotify, w->wd) && errno != EINVAL)
> +		error_errno("inotify_rm_watch() failed");
> +
> +	hashmap_entry_init(&k1.ent, memhash(&w->wd, sizeof(int)));
> +	w1 = hashmap_remove_entry(&data->watches, &k1, ent, NULL);
> +	if (!w1)
> +		BUG("Double remove of watch for '%s'", w->dir);
> +
> +	if (w1->cookie)
> +		BUG("Removing watch for '%s' which has a pending rename", w1->dir);
> +
> +	hashmap_entry_init(&k2.ent, memhash(w->dir, strlen(w->dir)));
> +	w2 = hashmap_remove_entry(&data->revwatches, &k2, ent, NULL);
> +	if (!w2)
> +		BUG("Double remove of reverse watch for '%s'", w->dir);
> +
> +	/* w1->dir and w2->dir are interned strings, we don't own them */
> +	free(w1);
> +	free(w2);
> +}
> +
> +/*
> + * Check for stale directory renames.
> + *
> + * https://man7.org/linux/man-pages/man7/inotify.7.html
> + *
> + * Allow for some small timeout to account for the fact that insertion of the
> + * IN_MOVED_FROM+IN_MOVED_TO event pair is not atomic, and the possibility that
> + * there may not be any IN_MOVED_TO event.
> + *
> + * If the IN_MOVED_TO event is not received within the timeout then events have
> + * been missed and the monitor is in an inconsistent state with respect to the
> + * filesystem.
> + */
> +static int check_stale_dir_renames(struct hashmap *renames, time_t max_age)
> +{
> +	struct rename_entry *re;
> +	struct hashmap_iter iter;
> +
> +	hashmap_for_each_entry(renames, &iter, re, ent) {
> +		if (re->whence <= max_age)
> +			return -1;
> +	}
> +	return 0;
> +}
> +
> +/*
> + * Track pending renames.
> + *
> + * Tracking is done via a event cookie to watch descriptor mapping.
> + *
> + * A rename is not complete until matching a IN_MOVED_TO event is received
> + * for a corresponding IN_MOVED_FROM event.
> + */
> +static void add_dir_rename(uint32_t cookie, const char *path,
> +	struct fsm_listen_data *data)

Nit: `add_dir_rename()` and the below `rename_dir()` sound as if we
actually perform the rename ourselves. How about `track_dir_rename()`
and `finalize_dir_rename()`?

> +{
> +	struct watch_entry k, *w;
> +	struct rename_entry *re;
> +
> +	/* lookup the watch descriptor for the given path */
> +	hashmap_entry_init(&k.ent, memhash(path, strlen(path)));
> +	w = hashmap_get_entry(&data->revwatches, &k, ent, NULL);
> +	if (!w) /* should never happen */
> +		BUG("No watch for '%s'", path);

Error message should start with lower-case character.

> +	w->cookie = cookie;
> +
> +	/* add the pending rename to match against later */
> +	CALLOC_ARRAY(re, 1);
> +	re->dir = w->dir;
> +	re->cookie = w->cookie;
> +	re->whence = time(NULL);
> +	hashmap_entry_init(&re->ent, memhash(&re->cookie, sizeof(uint32_t)));
> +	hashmap_add(&data->renames, &re->ent);
> +}
> +
> +/*
> + * Handle directory renames
> + *
> + * Once a IN_MOVED_TO event is received, lookup the rename tracking information
> + * via the event cookie and use this information to update the watch.
> + */
> +static void rename_dir(uint32_t cookie, const char *path,
> +	struct fsm_listen_data *data)
> +{
> +	struct rename_entry rek, *re;
> +	struct watch_entry k, *w;
> +
> +	/* lookup a pending rename to match */
> +	rek.cookie = cookie;
> +	hashmap_entry_init(&rek.ent, memhash(&rek.cookie, sizeof(uint32_t)));
> +	re = hashmap_get_entry(&data->renames, &rek, ent, NULL);
> +	if (re) {
> +		k.dir = re->dir;
> +		hashmap_entry_init(&k.ent, memhash(k.dir, strlen(k.dir)));
> +		w = hashmap_get_entry(&data->revwatches, &k, ent, NULL);
> +		if (w) {
> +			w->cookie = 0; /* rename handled */
> +			remove_watch(w, data);
> +			add_watch(path, data);
> +		} else {
> +			BUG("No matching watch");
> +		}
> +	} else {
> +		BUG("No matching cookie");

The above two bugs should start with a lower-case letter.

> +	}
> +}
> +
> +/*
> + * Recursively add watches to every directory under path
> + */
> +static int register_inotify(const char *path,
> +	struct fsmonitor_daemon_state *state,
> +	struct fsmonitor_batch *batch)
> +{
> +	DIR *dir;
> +	const char *rel;
> +	struct strbuf current = STRBUF_INIT;
> +	struct dirent *de;
> +	struct stat fs;
> +	int ret = -1;
> +
> +	dir = opendir(path);
> +	if (!dir)
> +		return error_errno("opendir('%s') failed", path);
> +
> +	while ((de = readdir_skip_dot_and_dotdot(dir)) != NULL) {
> +		strbuf_reset(&current);
> +		strbuf_addf(&current, "%s/%s", path, de->d_name);
> +		if (lstat(current.buf, &fs)) {
> +			error_errno("lstat('%s') failed", current.buf);

Missing `_()` translation marker.

> +			goto failed;
> +		}
> +
> +		/* recurse into directory */
> +		if (S_ISDIR(fs.st_mode)) {
> +			if (add_watch(current.buf, state->listen_data))
> +				goto failed;
> +			if (register_inotify(current.buf, state, batch))
> +				goto failed;
> +		} else if (batch) {
> +			rel = current.buf + state->path_worktree_watch.len + 1;
> +			trace_printf_key(&trace_fsmonitor, "explicitly adding '%s'", rel);
> +			fsmonitor_batch__add_path(batch, rel);
> +		}
> +	}
> +	ret = 0;
> +
> +failed:
> +	strbuf_release(&current);
> +	if (closedir(dir) < 0)
> +		return error_errno("closedir('%s') failed", path);

Missing `_()` translation marker.

> +	return ret;
> +}
> +
> +static int em_rename_dir_from(u_int32_t mask)
> +{
> +	return ((mask & IN_ISDIR) && (mask & IN_MOVED_FROM));
> +}
> +
> +static int em_rename_dir_to(u_int32_t mask)
> +{
> +	return ((mask & IN_ISDIR) && (mask & IN_MOVED_TO));
> +}
> +
> +static int em_remove_watch(u_int32_t mask)
> +{
> +	return (mask & IN_DELETE_SELF);
> +}
> +
> +static int em_dir_renamed(u_int32_t mask)
> +{
> +	return ((mask & IN_ISDIR) && (mask & IN_MOVE));
> +}
> +
> +static int em_dir_created(u_int32_t mask)
> +{
> +	return ((mask & IN_ISDIR) && (mask & IN_CREATE));
> +}
> +
> +static int em_dir_deleted(uint32_t mask)
> +{
> +	return ((mask & IN_ISDIR) && (mask & IN_DELETE));
> +}
> +
> +static int em_force_shutdown(u_int32_t mask)
> +{
> +	return (mask & IN_UNMOUNT) || (mask & IN_Q_OVERFLOW);
> +}
> +
> +static int em_ignore(u_int32_t mask)
> +{
> +	return (mask & IN_IGNORED) || (mask & IN_MOVE_SELF);
> +}
> +
> +static void log_mask_set(const char *path, u_int32_t mask)
> +{
> +	struct strbuf msg = STRBUF_INIT;
> +
> +	if (mask & IN_ACCESS)
> +		strbuf_addstr(&msg, "IN_ACCESS|");
> +	if (mask & IN_MODIFY)
> +		strbuf_addstr(&msg, "IN_MODIFY|");
> +	if (mask & IN_ATTRIB)
> +		strbuf_addstr(&msg, "IN_ATTRIB|");
> +	if (mask & IN_CLOSE_WRITE)
> +		strbuf_addstr(&msg, "IN_CLOSE_WRITE|");
> +	if (mask & IN_CLOSE_NOWRITE)
> +		strbuf_addstr(&msg, "IN_CLOSE_NOWRITE|");
> +	if (mask & IN_OPEN)
> +		strbuf_addstr(&msg, "IN_OPEN|");
> +	if (mask & IN_MOVED_FROM)
> +		strbuf_addstr(&msg, "IN_MOVED_FROM|");
> +	if (mask & IN_MOVED_TO)
> +		strbuf_addstr(&msg, "IN_MOVED_TO|");
> +	if (mask & IN_CREATE)
> +		strbuf_addstr(&msg, "IN_CREATE|");
> +	if (mask & IN_DELETE)
> +		strbuf_addstr(&msg, "IN_DELETE|");
> +	if (mask & IN_DELETE_SELF)
> +		strbuf_addstr(&msg, "IN_DELETE_SELF|");
> +	if (mask & IN_MOVE_SELF)
> +		strbuf_addstr(&msg, "IN_MOVE_SELF|");
> +	if (mask & IN_UNMOUNT)
> +		strbuf_addstr(&msg, "IN_UNMOUNT|");
> +	if (mask & IN_Q_OVERFLOW)
> +		strbuf_addstr(&msg, "IN_Q_OVERFLOW|");
> +	if (mask & IN_IGNORED)
> +		strbuf_addstr(&msg, "IN_IGNORED|");
> +	if (mask & IN_ISDIR)
> +		strbuf_addstr(&msg, "IN_ISDIR|");

Doesn't this end up with one trailing '|' in `msg`? You could use
`strbuf_strip_suffix(msg, "|")` to drop it.

> +	trace_printf_key(&trace_fsmonitor, "inotify_event: '%s', mask=%#8.8x %s",
> +				path, mask, msg.buf);
> +
> +	strbuf_release(&msg);
> +}
> +
> +int fsm_listen__ctor(struct fsmonitor_daemon_state *state)
> +{
> +	int fd;
> +	int ret = 0;
> +	struct fsm_listen_data *data;
> +
> +	CALLOC_ARRAY(data, 1);
> +	state->listen_data = data;
> +	state->listen_error_code = -1;
> +	data->shutdown = SHUTDOWN_ERROR;
> +
> +	fd = inotify_init1(O_NONBLOCK);
> +	if (fd < 0)
> +		return error_errno("inotify_init1() failed");
> +
> +	data->fd_inotify = fd;
> +
> +	hashmap_init(&data->watches, watch_entry_cmp, NULL, 0);
> +	hashmap_init(&data->renames, rename_entry_cmp, NULL, 0);
> +	hashmap_init(&data->revwatches, revwatches_entry_cmp, NULL, 0);
> +
> +	if (add_watch(state->path_worktree_watch.buf, data))
> +		ret = -1;
> +	else if (register_inotify(state->path_worktree_watch.buf, state, NULL))
> +		ret = -1;
> +	else if (state->nr_paths_watching > 1) {
> +		if (add_watch(state->path_gitdir_watch.buf, data))
> +			ret = -1;
> +		else if (register_inotify(state->path_gitdir_watch.buf, state, NULL))
> +			ret = -1;
> +	}

Style: if one of the branches requires braces then all of them should.

> +
> +	if (!ret) {
> +		state->listen_error_code = 0;
> +		data->shutdown = SHUTDOWN_CONTINUE;
> +	}
> +
> +	return ret;
> +}
> +
> +void fsm_listen__dtor(struct fsmonitor_daemon_state *state)
> +{
> +	struct fsm_listen_data *data;
> +	struct hashmap_iter iter;
> +	struct watch_entry *w;
> +	int fd;
> +
> +	if (!state || !state->listen_data)
> +		return;
> +
> +	data = state->listen_data;
> +	fd = data->fd_inotify;
> +
> +	hashmap_for_each_entry(&data->watches, &iter, w, ent) {
> +		w->cookie = 0; /* ignore any pending renames */
> +		remove_watch(w, data);
> +	}
> +	hashmap_clear(&data->watches);
> +
> +	hashmap_clear(&data->revwatches); /* remove_watch freed the entries */
> +
> +	hashmap_clear_and_free(&data->renames, struct rename_entry, ent);
> +
> +	FREE_AND_NULL(state->listen_data);

The empty lines between all these cleanups  can probably be removed.

> +
> +	if (fd && (close(fd) < 0))
> +		error_errno(_("closing inotify file descriptor failed"));
> +}
> +
> +void fsm_listen__stop_async(struct fsmonitor_daemon_state *state)
> +{
> +	if (!state->listen_data->shutdown)
> +		state->listen_data->shutdown = SHUTDOWN_STOP;
> +}
> +
> +/*
> + * Process a single inotify event and queue for publication.
> + */
> +static int process_event(const char *path,
> +	const struct inotify_event *event,
> +	struct fsmonitor_batch *batch,
> +	struct string_list *cookie_list,
> +	struct fsmonitor_daemon_state *state)
> +{
> +	const char *rel;
> +	const char *last_sep;
> +
> +	switch (fsmonitor_classify_path_absolute(state, path)) {
> +		case IS_INSIDE_DOT_GIT_WITH_COOKIE_PREFIX:
> +		case IS_INSIDE_GITDIR_WITH_COOKIE_PREFIX:
> +			/* Use just the filename of the cookie file. */
> +			last_sep = find_last_dir_sep(path);
> +			string_list_append(cookie_list,
> +					last_sep ? last_sep + 1 : path);
> +			break;
> +		case IS_INSIDE_DOT_GIT:
> +		case IS_INSIDE_GITDIR:
> +			break;
> +		case IS_DOT_GIT:
> +		case IS_GITDIR:
> +			/*
> +			* If .git directory is deleted or renamed away,
> +			* we have to quit.
> +			*/
> +			if (em_dir_deleted(event->mask)) {
> +				trace_printf_key(&trace_fsmonitor,
> +						"event: gitdir removed");
> +				state->listen_data->shutdown = SHUTDOWN_FORCE;
> +				goto done;
> +			}
> +
> +			if (em_dir_renamed(event->mask)) {
> +				trace_printf_key(&trace_fsmonitor,
> +						"event: gitdir renamed");
> +				state->listen_data->shutdown = SHUTDOWN_FORCE;
> +				goto done;
> +			}
> +			break;
> +		case IS_WORKDIR_PATH:
> +			/* normal events in the working directory */
> +			if (trace_pass_fl(&trace_fsmonitor))
> +				log_mask_set(path, event->mask);
> +
> +			rel = path + state->path_worktree_watch.len + 1;
> +			fsmonitor_batch__add_path(batch, rel);
> +
> +			if (em_dir_deleted(event->mask))
> +				break;
> +
> +			/* received IN_MOVE_FROM, add tracking for expected IN_MOVE_TO */
> +			if (em_rename_dir_from(event->mask))
> +				add_dir_rename(event->cookie, path, state->listen_data);
> +
> +			/* received IN_MOVE_TO, update watch to reflect new path */
> +			if (em_rename_dir_to(event->mask)) {
> +				rename_dir(event->cookie, path, state->listen_data);
> +				if (register_inotify(path, state, batch)) {
> +					state->listen_data->shutdown = SHUTDOWN_ERROR;
> +					goto done;
> +				}
> +			}
> +
> +			if (em_dir_created(event->mask)) {
> +				if (add_watch(path, state->listen_data)) {
> +					state->listen_data->shutdown = SHUTDOWN_ERROR;
> +					goto done;
> +				}
> +				if (register_inotify(path, state, batch)) {
> +					state->listen_data->shutdown = SHUTDOWN_ERROR;
> +					goto done;
> +				}
> +			}
> +			break;
> +		case IS_OUTSIDE_CONE:
> +		default:
> +			trace_printf_key(&trace_fsmonitor,
> +					"ignoring '%s'", path);
> +			break;
> +	}
> +	return 0;
> +done:
> +	return -1;
> +}
> +
> +/*
> + * Read the inotify event stream and pre-process events before further
> + * processing and eventual publishing.
> + */
> +static void handle_events(struct fsmonitor_daemon_state *state)
> +{
> +	 /* See https://man7.org/linux/man-pages/man7/inotify.7.html */
> +	char buf[4096]
> +		__attribute__ ((aligned(__alignof__(struct inotify_event))));
> +
> +	struct hashmap watches = state->listen_data->watches;
> +	struct fsmonitor_batch *batch = NULL;
> +	struct string_list cookie_list = STRING_LIST_INIT_DUP;
> +	struct watch_entry k, *w;
> +	struct strbuf path;
> +	const struct inotify_event *event;
> +	int fd = state->listen_data->fd_inotify;
> +	ssize_t len;
> +	char *ptr, *p;

I think many of the variables could be moved into deeper scopes. Makes
it easier to assess what is being used where.

> +
> +	strbuf_init(&path, PATH_MAX);
> +
> +	for(;;) {
> +		len = read(fd, buf, sizeof(buf));
> +		if (len == -1 && errno != EAGAIN) {

Wouldn't we also have to handle EINTR here?

> +			error_errno(_("reading inotify message stream failed"));
> +			state->listen_data->shutdown = SHUTDOWN_ERROR;
> +			goto done;
> +		}
> +
> +		/* nothing to read */
> +		if (len <= 0)
> +			goto done;
> +
> +		/* Loop over all events in the buffer. */
> +		for (ptr = buf; ptr < buf + len;
> +			 ptr += sizeof(struct inotify_event) + event->len) {

Nit: there's an additional whitespace here.

> +
> +			event = (const struct inotify_event *) ptr;
> +
> +			if (em_ignore(event->mask))
> +				continue;
> +
> +			/* File system was unmounted or event queue overflowed */
> +			if (em_force_shutdown(event->mask)) {
> +				if (trace_pass_fl(&trace_fsmonitor))
> +					log_mask_set("Forcing shutdown", event->mask);
> +				state->listen_data->shutdown = SHUTDOWN_FORCE;
> +				goto done;
> +			}
> +
> +			hashmap_entry_init(&k.ent, memhash(&event->wd, sizeof(int)));
> +			k.wd = event->wd;
> +
> +			w = hashmap_get_entry(&watches, &k, ent, NULL);
> +			if (!w) /* should never happen */
> +				BUG("No watch for '%s'", event->name);

Error messages should start with a lower-case letter.

> +
> +			/* directory watch was removed */
> +			if (em_remove_watch(event->mask)) {
> +				remove_watch(w, state->listen_data);
> +				continue;
> +			}
> +
> +			strbuf_reset(&path);
> +			strbuf_add(&path, w->dir, strlen(w->dir));
> +			strbuf_addch(&path, '/');
> +			strbuf_addstr(&path, event->name);

The above three lines can be simplified to:

```
strbuf_addf("%s/%s", w->dir, event->name);
```

> +
> +			p = fsmonitor__resolve_alias(path.buf, &state->alias);
> +			if (!p)
> +				p = strbuf_detach(&path, NULL);
> +
> +			if (!batch)
> +				batch = fsmonitor_batch__new();
> +
> +			if (process_event(p, event, batch, &cookie_list, state)) {
> +				free(p);
> +				goto done;
> +			}
> +			free(p);
> +		}
> +		strbuf_reset(&path);
> +		fsmonitor_publish(state, batch, &cookie_list);
> +		string_list_clear(&cookie_list, 0);
> +		batch = NULL;
> +	}
> +done:
> +	strbuf_release(&path);
> +	fsmonitor_batch__free_list(batch);
> +	string_list_clear(&cookie_list, 0);
> +}
> +
> +/*
> + * Non-blocking read of the inotify events stream. The inotify fd is polled
> + * frequently to help minimize the number of queue overflows.
> + */
> +void fsm_listen__loop(struct fsmonitor_daemon_state *state)
> +{
> +	int poll_num;
> +	const int interval = 1000;

You could rename the variable to `interval_ms` to clarify its unit.

> +	time_t checked = time(NULL);
> +	struct pollfd fds[1];
> +	fds[0].fd = state->listen_data->fd_inotify;
> +	fds[0].events = POLLIN;
> +
> +	for(;;) {

Nit: `while (1)`

> +		switch (state->listen_data->shutdown) {
> +			case SHUTDOWN_CONTINUE:
> +				poll_num = poll(fds, 1, 1);

Why do you pick a timeout of 1 millisecond here? I'd have expected us to
instruct poll(3p) to either not block at all (0) or to block for
`interval`.

> +				if (poll_num == -1) {
> +					if (errno == EINTR)
> +						continue;

Wouldn't we also have to handle EAGAIN here?

Patrick

> +					error_errno(_("polling inotify message stream failed"));
> +					state->listen_data->shutdown = SHUTDOWN_ERROR;
> +					continue;
> +				}
> +
> +				if ((time(NULL) - checked) >= interval) {
> +					checked = time(NULL);
> +					if (check_stale_dir_renames(&state->listen_data->renames,
> +						checked - interval)) {
> +						trace_printf_key(&trace_fsmonitor,
> +							"Missed IN_MOVED_TO events, forcing shutdown");
> +						state->listen_data->shutdown = SHUTDOWN_FORCE;
> +						continue;
> +					}
> +				}
> +
> +				if (poll_num > 0 && (fds[0].revents & POLLIN))
> +					handle_events(state);
> +
> +				continue;
> +			case SHUTDOWN_ERROR:
> +				state->listen_error_code = -1;
> +				ipc_server_stop_async(state->ipc_server_data);
> +				break;
> +			case SHUTDOWN_FORCE:
> +				state->listen_error_code = 0;
> +				ipc_server_stop_async(state->ipc_server_data);
> +				break;
> +			case SHUTDOWN_STOP:
> +			default:
> +				state->listen_error_code = 0;
> +				break;
> +		}
> +		return;
> +	}
> +}
> -- 
> gitgitgadget
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]