Subject: Re: [PATCH v5 07/35] hold_lock_file_for_append(): release lock on errors

From: Michael Haggerty <[email-masked]>
Date: Wed, 17 Sep 2014 17:39:48 +0200

On 09/16/2014 10:48 PM, Jonathan Nieder wrote:
> Michael Haggerty wrote:
> 
>> --- a/lockfile.c
>> +++ b/lockfile.c
>> @@ -219,13 +219,13 @@ int hold_lock_file_for_append(struct lock_file *lk, const char *path, int flags)
>>  		if (errno != ENOENT) {
>>  			if (flags & LOCK_DIE_ON_ERROR)
>>  				die("cannot open '%s' for copying", path);
>> -			close(fd);
>> +			rollback_lock_file(lk);
>>  			return error("cannot open '%s' for copying", path);
> 
> Makes sense.
> 
> Now that I'm here, I wonder a little at the error convention.  If the
> caller doesn't pass LOCK_DIE_ON_ERROR, are they supposed to be able to
> use unable_to_lock_message?  What errno would they pass in the err
> parameter?  Would callers want handle failure to acquire a lock
> differently from other errors (e.g., by sleeping and trying again),
> and if not, what is the optionally-die behavior in hold_lock_file
> about?

The same applies to hold_lock_file_for_update(), so I'll discuss both at
once:

Most callers do pass LOCK_DIE_ON_ERROR. Of the ones that don't, a couple
appear to want to emit more meaningful error messages. A couple don't
die at all but return an error code to their caller. At least one
(add_to_alternates_file()) calls die_errno().

As it happens, hold_lock_file_for_append() sometimes overwrites errno
before it returns. I will add a patch on top of this series that fixes that.

I don't see any callers that retry, though I've thought about
implementing that in some places. But it's outside of the scope of this
patch series.

Michael

-- 
Michael Haggerty
[email-masked]