Re: [PATCH 1/3] fdopen_lock_file(): access a lockfile using stdio

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 1, 2014, 21:20 UTC
Message-ID: <xmqq8ukzpjw1.fsf@gitster.dls.corp.google.com>
In-Reply-To: <20141001124847.GB10772@peff.net>

Jeff King <peff@peff.net> writes:

Show 8 quoted lines

> On Wed, Oct 01, 2014 at 01:14:47PM +0200, Michael Haggerty wrote:
>
>> ... If using stdio on lockfiles becomes
>> more popular, we might want to add some helper functions to make it a
>> bit more convenient.
>
> I think it makes sense to wait until we see more potential callers crop
> up.

Yup.

Show 12 quoted lines

>> In close_lock_file(), if ferror() returns an error, then errno is not
>> necessarily still set in a way that reflects the original error. I
>> don't see a way to ensure that errno is set correctly in this
>> situation. But hopefully, callers are monitoring their calls to
>> fwrite()/fprintf() etc and will have noticed write errors on their own
>> already. If anybody can suggest an improvement here, please let me
>> know.
>
> I was careful in the packed-refs stdio caller to check all of my fprintf
> calls (because I was using fclose myself). I wonder if it would be nicer
> to back off from that and just depend on the ferror() call at
> commit-time.

That's a thought (I think the same can be said for "close-time").

Show 15 quoted lines

>> -static void remove_lock_files(void)
>> +static void remove_lock_files(int skip_fclose)
>>  {
>>  	pid_t me = getpid();
>>  
>>  	while (lock_file_list) {
>> -		if (lock_file_list->owner == me)
>> +		if (lock_file_list->owner == me) {
>> +			/* fclose() is not safe to call in a signal handler */
>> +			if (skip_fclose)
>> +				lock_file_list->fp = NULL;
>
> I wondered when reading the commit message if it should mention this
> signal-handling case (which you brought up in the cover letter). This
> comment is probably enough, though.

No strong opinion either way.

Show 12 quoted lines

>> +FILE *fdopen_lock_file(struct lock_file *lk, const char *mode)
>> +{
>> +	if (!lk->active)
>> +		die("BUG: fdopen_lock_file() called for unlocked object");
>> +	if (lk->fp)
>> +		die("BUG: fdopen_lock_file() called twice for file '%s'", lk->filename.buf);
>
> I would have expected calling this twice to be a noop (i.e., make the
> function more "give me the associated filehandle, and create one if
> necessary"). But I don't think any current callers should need that, so
> it probably makes sense to play it safe and die("BUG"), at least for
> now.

Interesting. One could imagine a sane call-chain whose top-level creates a lockfile, associates a FILE * with it to write into it itself, then calls set of helpers. You could pass only FILE * to such helpers that does nothing other than writing to lk->fp to the lockfile, but it would be cumbersome if a helper wants to have access to the lockfile itself and FILE * (i.e. it writes and then either commits or rolls back; name it foo_finish() or something).

Such a call-chain certainly would want a way to ask "I know this lk is already associated with a FILE *; give me that". But that still does not require "I do not know if this lk already has FILE * or not, but I want a FILE * associated with it now. Peek or create."

So I think this is OK.

Show 13 quoted lines

>> +	if (fp) {
>> +		lk->fp = NULL;
>> +
>> +		/*
>> +		 * Note: no short-circuiting here; we want to fclose()
>> +		 * in any case!
>> +		 */
>> +		err = ferror(fp) | fclose(fp);
>
> Would this be more clear as:
>
> 	err = ferror(fp);
> 	err |= fclose(fp);

No strong opinion either way.

Thanks, both.