On Tue, Mar 01 2022, Jeff Hostetler via GitGitGadget wrote:
> From: Jeff Hostetler <jeffhost@xxxxxxxxxxxxx>
> [...]
> +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?
> + /*
> + * 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?
> +
> + /*
> + * 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?
> + }
> +
> + 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.
