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@xxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html