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

From: Jeff King <peff@peff.net>
Date: Oct 1, 2014, 12:48 UTC
Message-ID: <20141001124847.GB10772@peff.net>
In-Reply-To: <1412162089-3233-2-git-send-email-mhagger@alum.mit.edu>

On Wed, Oct 01, 2014 at 01:14:47PM +0200, Michael Haggerty wrote:

Show 6 quoted lines

> I thought about adding second stdio-oriented entrance points analogous
> to hold_lock_file_for_update(), hold_lock_file_for_append(), and
> reopen_lock_file(), but it seemed simpler to add just the one new
> function instead of three or four. 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.

Show 7 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. The exact value of errno is not usually that important after the open() has succeeded.

Show 11 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.

Show 6 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.

Show 8 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);

-Peff