Re: [PATCH v5 23/35] lockfile: avoid transitory invalid states

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Sep 16, 2014, 22:45 UTC
Message-ID: <20140916224523.GW29050@google.com>
In-Reply-To: <1410896036-12750-24-git-send-email-mhagger@alum.mit.edu>

Michael Haggerty wrote:

Show 8 quoted lines

> We could probably continue to use the filename field to encode the
> state by being careful to write characters 1..N-1 of the filename
> first, and then overwrite the NUL at filename[0] with the first
> character of the filename, but that would be awkward and error-prone.
> 
> So, instead of using the filename field to determine whether the
> lock_file object is active, add a new field "lock_file::active" for
> this purpose.

Nice.

[...]

Show 10 quoted lines

> --- a/cache.h
> +++ b/cache.h
> @@ -576,6 +576,7 @@ extern int refresh_index(struct index_state *, unsigned int flags, const struct
>  
>  struct lock_file {
>  	struct lock_file *next;
> +	volatile sig_atomic_t active;
>  	int fd;
>  	pid_t owner;
>  	char on_list;

[...]

> +++ b/lockfile.c
> @@ -27,16 +27,19 @@

[...]

Show 7 quoted lines

> @@ -189,9 +198,14 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
>  		atexit(remove_lock_file);
>  	}
>  
> +	if (lk->active)
> +		die("BUG: lock_file(\"%s\") called with an active lock_file object",
> +		    path);

The error message doesn't make it entirely obvious to me what went wrong.

Maybe something like

		die("BUG: cannot lock_file(\"%s\") on active struct lock_file",
		    path);

lock_file already assumed on_list was initialized to zero but it wasn't particularly obvious since everything else is blindly scribbled over. Probably worth mentioning in the API docs that the lock_file should be zeroed before calling hold_lock_file_...

[...]

Show 6 quoted lines

> @@ -326,6 +341,7 @@ int commit_lock_file(struct lock_file *lk)
>  	if (rename(lk->filename, result_file))
>  		goto rollback;
>  
> +	lk->active = 0;
>  	lk->filename[0] = 0;

Is it useful to set filename[0] any more?

It seems potentially fragile to set both, since new code can appear that only sets or checks one or the other. Would it make sense to rename the filename field to make sure no new callers relying on the filename[0] convention sneak in (with the new convention being that the filename doesn't get cleared, to avoid problems)?

Jonathan