Only write debug messages if descriptor is set

[mikedld]

fflush() flushes all open descriptors in case NULL is passed as argument. This patch prevents calls to fflush in case debug file descriptor is not initialized.

See also: https://trac.transmissionbt.com/ticket/4531.

[cfpp2p]

I think that I might see where an inconsistency with fflush() with NULL as argument might be originating from. Referencing http://www.opensource.apple.com and possibly older MAC systems the patch for "Fix a potential deadlock in _fwalk in a threaded environment." does not seem applied. So possibly when transmission-daemon compiled on MAC for the few transmissions users who report this bug is compiled with such faulty compiler.

http://www.opensource.apple.com/source/Libc/Libc-167/stdio.subproj/fwalk.c

http://www.opensource.apple.com/source/Libc/Libc-167/stdio.subproj/refill.c

The patch of _fwalk per:

committer Peter Avalos pavalos@dragonflybsd.org
Tue, 21 Apr 2009 06:20:06 +0000 (06:20 +0000)
http://gitweb.dragonflybsd.org/dragonfly.git/commitdiff/e0f95098eeba0176864b9cafe6d69b5b7bc0e73f

http://gitweb.dragonflybsd.org/dragonfly.git/commitdiff/e0f95098eeba0176864b9cafe6d69b5b7bc0e73f#patch52

http://gitweb.dragonflybsd.org/dragonfly.git/commitdiff/e0f95098eeba0176864b9cafe6d69b5b7bc0e73f#patch86

@@ -53,13 +49,17 @@ _fwalk(int (*function)(FILE *))

        ret = 0;
        /*
-        * It should be safe to walk  the list without locking it;
+        * It should be safe to walk the list without locking it;
         * new nodes are only added to the end and none are ever
         * removed.
+        *
+        * Avoid locking this list while walking it or else you will
+        * introduce a potential deadlock in [at least] refill.c.
         */
        for (g = &__sglue; g != NULL; g = g->next)
                for (fp = g->iobs, n = g->niobs; --n >= 0; fp++)
-                       if (fp->pub._flags != 0)
+                       if ((fp->pub._flags != 0) &&
+                           ((fp->pub._flags & __SIGN) == 0))
                                ret |= (*function)(fp);
        return (ret);
 }


@@ -114,10 +119,18 @@ __srefill(FILE *fp)
         * flush all line buffered output files, per the ANSI C
         * standard.
         */
-       if (fp->pub._flags & (__SLBF|__SNBF))
+       if (fp->pub._flags & (__SLBF|__SNBF)) {
+               /* Ignore this file in _fwalk to avoid potential deadlock. */
+               fp->pub._flags |= __SIGN;
                _fwalk(lflush);
+               fp->pub._flags &= ~__SIGN;
+
+               /* Now flush this file without locking it. */
+               if ((fp->pub._flags & (__SLBF|__SWR)) == (__SLBF|__SWR))
+                       __sflush(fp);
+       }
        fp->pub._p = fp->_bf._base;
-       fp->pub._r = (*fp->_read)(fp->_cookie, (char *)fp->pub._p, fp->_bf._size);
+       fp->pub._r = _sread(fp, (char *)fp->pub._p, fp->_bf._size);
        fp->pub._flags &= ~__SMOD;      /* buffer contents are again pristine */
        if (fp->pub._r <= 0) {
                if (fp->pub._r == 0)

[cfpp2p]

I don't see why call debugf() in the first place if we know there's nothing to print. Why not something like this:
cfpp2p/transmission@020f377

[mikedld]

For one thing, because I don't like having 47 identical checks scattered over the code when I could have just one (or close to one)... Wrapping the check with macro would've been a better idea if you want to get rid of unnecessary calls that much :) As to the idea itself, you may have a point here performance-wise.

[jech]

This patch prevents calls to fflush in case debug file descriptor is not initialized.

Applied as 6902710.

Wrapping the check with macro would've been a better idea if you want to get rid of unnecessary calls that much

Yes, that's what I do in usually (see for example https://github.com/jech/babeld/blob/master/util.h#L123) but since I'm trying to keep this code small (so people can easily include it in their projects), I didn't bother.

--jch