Re: [PATCH v6 27/30] fsmonitor--daemon: use a cookie file to sync with file system

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Mar 7, 2022, 11:15 UTC
Message-ID: <220307.86r17e0zv0.gmgdl@evledraar.gmail.com>
In-Reply-To: <5b06eb5d0e60c7d23987d915609c07364e4f7372.1646160213.git.gitgitgadget@gmail.com>

On Tue, Mar 01 2022, Jeff Hostetler via GitGitGadget wrote:

Show 5 quoted lines

> From: Jeff Hostetler <jeffhost@microsoft.com>
> [...]
> +enum fsmonitor_cookie_item_result {
> +	FCIR_ERROR = -1, /* could not create cookie file ? */
> +	FCIR_INIT = 0,

nit: redundant = 0 assignment, i.e....

> +	FCIR_SEEN,
> +	FCIR_ABORT,

If we're going to make these implicit (which we usually do) let's do the same for FCIR_INIT.

> +	strbuf_addf(&cookie_filename, "%i-%i", getpid(), my_cookie_seq);

For trace2 we encoded getpid() in the SID using a fixed-width encoding format, maybe do the same here?

Show 7 quoted lines

> +	/*
> +	 * Create the cookie file on disk and then wait for a notification
> +	 * that the listener thread has seen it.
> +	 */
> +	fd = open(cookie_pathname.buf, O_WRONLY | O_CREAT | O_EXCL, 0600);
> +	if (fd >= 0) {
> +		close(fd);

Also check the return value of close()...

> +		unlink(cookie_pathname.buf);

...and unlink()? If we explicitly prefer not to, note why?

Show 21 quoted lines

> +
> +		/*
> +		 * Technically, this is an infinite wait (well, unless another
> +		 * thread sends us an abort).  I'd like to change this to
> +		 * use `pthread_cond_timedwait()` and return an error/timeout
> +		 * and let the caller do the trivial response thing, but we
> +		 * don't have that routine in our thread-utils.
> +		 *
> +		 * After extensive beta testing I'm not really worried about
> +		 * this.  Also note that the above open() and unlink() calls
> +		 * will cause at least two FS events on that path, so the odds
> +		 * of getting stuck are pretty slim.
> +		 */
> +		while (cookie->result == FCIR_INIT)
> +			pthread_cond_wait(&state->cookies_cond,
> +					  &state->main_lock);
> +	} else {
> +		error_errno(_("could not create fsmonitor cookie '%s'"),
> +			    cookie->name);
> +
> +		cookie->result = FCIR_ERROR;

...more readable if we just:

    if (open(...) < 0) {
        ...;
        goto cleanup;

So no need to indent the comment/main logic of the function above?

Show 7 quoted lines

> +	}
> +
> +	hashmap_remove(&state->cookies, &cookie->entry, NULL);
> +
> +	result = cookie->result;
> +
> +	free((char*)cookie->name);

Since the "name" here is always an allocated string we should just make it "char *" in the struct and avoid these casts.

In other places we play this casting game because the struct is a public API, but in this case it's all ours.