Jeff King <peff@xxxxxxxx> writes:
> 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.
>> 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").
>> -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.
>> +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.
>> + 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html