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

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Sep 17, 2014, 15:39 UTC
Message-ID: <5419AB44.1070109@alum.mit.edu>
In-Reply-To: <20140916204853.GH29050@google.com>

On 09/16/2014 10:48 PM, Jonathan Nieder wrote:

Show 21 quoted lines

> 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
mhagger@alum.mit.edu