On Wed, Oct 01, 2014 at 01:14:47PM +0200, Michael Haggerty wrote:
> 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.
> 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.
> -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.
> +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.
> + 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
