threads / patch / 48218

patchFix condition for redirecting stderr

Subject: [PATCH] Fix condition for redirecting stderr

## tl;dr

3 messages between Apr 3, 2018 and Apr 9, 2018. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

Lucas Werkmeister· Apr 3, 2018, 22:13 UTC · lore

Since the --log-destination option was added in 0c591cacb ("daemon: add --log-destination=(stderr|syslog|none)", 2018-02-04) with the explicit goal of allowing logging to stderr when running in inetd mode, we should not always redirect stderr to /dev/null in inetd mode, but rather only when stderr is not being used for logging.

Signed-off-by: Lucas Werkmeister <mail@lucaswerkmeister.de>
---

Notes:
    I have to apologize to the list here… even though I proposed this
    option with the goal of using it on my server, for some reason I
    decided to only use it there after the next Git release had come
    out, and didn’t test it anywhere else. The code looked okay, but I
    missed this part near the end of daemon.c that made it ineffective,
    rendering the feature broken in the 2.17.0 release and making me
    look like an idiot :/ sorry, everyone.
    
    For what it’s worth, with this fix the feature appears to work as
    intended (I *have* tested it on my server now and log messages from
    git-daemon show up in the journal as intended, attributed to the
    correct unit and everything).

 daemon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Show changes to daemon.c +1 −1

diff --git a/daemon.c b/daemon.c
index fe833ea7d..9d2e0d20e 100644
--- a/daemon.c
+++ b/daemon.c
@@ -1459,7 +1459,7 @@ int cmd_main(int argc, const char **argv)
 		die("base-path '%s' does not exist or is not a directory",
 		    base_path);
 
-	if (inetd_mode) {
+	if (log_destination != LOG_DESTINATION_STDERR) {
 		if (!freopen("/dev/null", "w", stderr))
 			die_errno("failed to redirect stderr to /dev/null");
 	}

-- 
2.16.3

Todd Zullinger· Apr 9, 2018, 02:23 UTC · re: Lucas Werkmeister · lore

Re: [PATCH] Fix condition for redirecting stderr

Lucas Werkmeister wrote:

Show 5 quoted lines

> Since the --log-destination option was added in 0c591cacb ("daemon: add
> --log-destination=(stderr|syslog|none)", 2018-02-04) with the explicit
> goal of allowing logging to stderr when running in inetd mode, we should
> not always redirect stderr to /dev/null in inetd mode, but rather only
> when stderr is not being used for logging.

Perhaps 's/^F/daemon: f/' on the subject? (Junio may well already have done so while queueing locally.)

The patch itself looks reasonable (to my relatively untrained eyes).

-- 
Todd
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Hardware:  the parts of a computer that can be kicked.
    -- Jeff Pesis

Lucas Werkmeister· Apr 9, 2018, 20:09 UTC · re: Todd Zullinger · lore

Re: [PATCH] Fix condition for redirecting stderr

On 09.04.2018 04:23, Todd Zullinger wrote:

Show 9 quoted lines

> Lucas Werkmeister wrote:
>> Since the --log-destination option was added in 0c591cacb ("daemon: add
>> --log-destination=(stderr|syslog|none)", 2018-02-04) with the explicit
>> goal of allowing logging to stderr when running in inetd mode, we should
>> not always redirect stderr to /dev/null in inetd mode, but rather only
>> when stderr is not being used for logging.
> 
> Perhaps 's/^F/daemon: f/' on the subject?  (Junio may well
> already have done so while queueing locally.)

Indeed, sorry! Looks like Junio fixed it on his end (going by the “what’s cooking” email), so I won’t reroll. (But I can at least set up a local commit-msg hook to make sure I don’t forget the subject area again.)