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

[Michael Haggerty <mhagger@alum.mit.edu>]

On 09/17/2014 12:45 AM, Jonathan Nieder wrote:
> Michael Haggerty wrote:
> 
>> 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.
> 
> [...]
>> --- 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 @@
> [...]
>> @@ -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);

This is an internal sanity check that users should never see, and if
they do the first thing a developer will do is grep the source code for
the error message in the bug report and then he/she will see exactly
what went wrong. So I don't think it is very important that this error
message be super self-explanatory.

But it doesn't hurt, so I'll make the change you suggest.

> 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_...

Good point. I will document this better.

> [...]
>> @@ -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)?

I admit that nobody should be relying on filename being cleared anymore.
And I can see your point that somebody might come to rely on this
implementation detail. But I don't like leaving valid filenames around
where a bug might cause them to be accessed accidentally. I would rather
set the filename to the empty string so that any attempt to use it
causes an immediate error message from the OS rather than accessing the
wrong file.

I will note in the lock_file docstring that client code should not rely
on the filename being empty when in the 'unlocked' state.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu