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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux