{"thread":{"id":"47669","subject":"[PATCH] daemon: add --no-syslog to undo implicit --syslog","startedAt":"2018-01-22T23:31:43Z","lastAt":"2018-02-05T20:09:42Z","messageCount":21,"participants":["Lucas Werkmeister","Ævar Arnfjörð Bjarmason","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"337121","messageId":"20180122232304.4863-1-mail@lucaswerkmeister.de","threadId":"47669","inReplyTo":null,"subject":"[PATCH] daemon: add --no-syslog to undo implicit --syslog","fromName":"Lucas Werkmeister","fromEmail":"mail@lucaswerkmeister.de","sentAt":"2018-01-22T23:23:04Z","receivedAt":"2018-01-22T23:31:43Z","isPatch":true,"sender":{"key":"mail@lucaswerkmeister.de","avatar":"https://avatars.githubusercontent.com/u/2346599?v=4"},"body":"Several options imply --syslog, without there being a way to disable it\nagain. This commit adds that option.\n\nThis is useful, for instance, when running `git daemon` as a systemd\nservice with --inetd. systemd connects stderr to the journal by default,\nso logging to stderr is useful. On the other hand, log messages sent via\nsyslog also reach the journal eventually, but run the risk of being\nprocessed at a time when the `git daemon` process has already exited\n(especially if the process was very short-lived, e.g. due to client\nerror), so that the journal can no longer read its cgroup and attach the\nmessage to the correct systemd unit. See systemd/systemd#2913 [1].\n\n[1]: https://github.com/systemd/systemd/issues/2913\n\nSigned-off-by: Lucas Werkmeister \n---\n\nNotes:\n I decided not to add the option to git-daemon's --help output, since\n the similar --no-informative-errors option is also not listed there.\n Let me know if it should be added.\n \n Feel free to remove the part about systemd from the commit message\n if you feel it doesn't need to be included.\n\n Documentation/git-daemon.txt | 6 ++++--\n daemon.c | 4 ++++\n 2 files changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt\nindex 3c91db7be..dfd6ce03c 100644\n--- a/Documentation/git-daemon.txt\n+++ b/Documentation/git-daemon.txt\n@@ -8,7 +8,7 @@ git-daemon - A really simple server for Git repositories\n SYNOPSIS\n --------\n [verse]\n-'git daemon' [--verbose] [--syslog] [--export-all]\n+'git daemon' [--verbose] [--[no-]syslog] [--export-all]\n \t [--timeout=] [--init-timeout=] [--max-connections=]\n \t [--strict-paths] [--base-path=] [--base-path-relaxed]\n \t [--user-path | --user-path=]\n@@ -109,9 +109,11 @@ OPTIONS\n \tMaximum number of concurrent clients, defaults to 32. Set it to\n \tzero for no limit.\n \n---syslog::\n+--[no-]syslog::\n \tLog to syslog instead of stderr. Note that this option does not imply\n \t--verbose, thus by default only error conditions will be logged.\n+\t`--no-syslog` is the default, but may be given explicitly to override\n+\tthe implicit `--syslog` of an earlier `--inetd` or `--detach` option.\n \n --user-path::\n --user-path=::\ndiff --git a/daemon.c b/daemon.c\nindex e37e343d0..d59fef6d6 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -1300,6 +1300,10 @@ int cmd_main(int argc, const char **argv)\n \t\t\tlog_syslog = 1;\n \t\t\tcontinue;\n \t\t}\n+\t\tif (!strcmp(arg, \"--no-syslog\")) {\n+\t\t\tlog_syslog = 0;\n+\t\t\tcontinue;\n+\t\t}\n \t\tif (!strcmp(arg, \"--export-all\")) {\n \t\t\texport_all_trees = 1;\n \t\t\tcontinue;\n-- \n2.16.0\n\n"},{"id":"337127","messageId":"87shaxh2b9.fsf@evledraar.gmail.com","threadId":"47669","inReplyTo":"20180122232304.4863-1-mail@lucaswerkmeister.de","subject":"Re: [PATCH] daemon: add --no-syslog to undo implicit --syslog","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-01-23T00:00:10Z","receivedAt":"2018-01-23T00:00:19Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Jan 22 2018, Lucas Werkmeister jotted:\n\n> Several options imply --syslog, without there being a way to disable it\n> again. This commit adds that option.\n\nJust two options imply --syslog, --detach & --inetd, unless I've missed\nsomething, anyway 2 != several, so maybe just say \"The --detach and\n--inetd options imply --syslog ...\".\n\n> This is useful, for instance, when running `git daemon` as a systemd\n> service with --inetd. systemd connects stderr to the journal by default,\n> so logging to stderr is useful. On the other hand, log messages sent via\n> syslog also reach the journal eventually, but run the risk of being\n> processed at a time when the `git daemon` process has already exited\n> (especially if the process was very short-lived, e.g. due to client\n> error), so that the journal can no longer read its cgroup and attach the\n> message to the correct systemd unit. See systemd/systemd#2913 [1].\n>\n> [1]: https://github.com/systemd/systemd/issues/2913\n\nThis patch looks good, but I wonder if with the rise of systemd there's\na good reason to flip the default around to not having other stuff imply\n--syslog, and have users specify this implictly, then we won't need a\n--no-syslog option.\n\nBut maybe that'll break too much stuff.\n"},{"id":"337168","messageId":"xmqqh8rcig0x.fsf@gitster.mtv.corp.google.com","threadId":"47669","inReplyTo":"87shaxh2b9.fsf@evledraar.gmail.com","subject":"Re: [PATCH] daemon: add --no-syslog to undo implicit --syslog","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-01-23T18:30:54Z","receivedAt":"2018-01-23T18:31:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> On Mon, Jan 22 2018, Lucas Werkmeister jotted:\n>\n>> Several options imply --syslog, without there being a way to disable it\n>> again. This commit adds that option.\n>\n> Just two options imply --syslog, --detach & --inetd, unless I've missed\n> something, anyway 2 != several, so maybe just say \"The --detach and\n> --inetd options imply --syslog ...\".\n\nCorrect. Moreover, --detach completely dissociates the process from\nthe original set of standard file descriptors by first closing them\nand then connecting it to \"/dev/null\", so it will be nonsense to use\nthis new option with it.\n"},{"id":"337185","messageId":"18ef807d-a642-17b2-fc32-af0c3b963a71@lucaswerkmeister.de","threadId":"47669","inReplyTo":"xmqqh8rcig0x.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] daemon: add --no-syslog to undo implicit --syslog","fromName":"Lucas Werkmeister","fromEmail":"mail@lucaswerkmeister.de","sentAt":"2018-01-23T22:06:18Z","receivedAt":"2018-01-23T22:06:25Z","isPatch":true,"sender":{"key":"mail@lucaswerkmeister.de","avatar":"https://avatars.githubusercontent.com/u/2346599?v=4"},"body":"Thanks for your responses!\n\nOn 23.01.2018 01:00, Ævar Arnfjörð Bjarmason wrote:\n> This patch looks good, but I wonder if with the rise of systemd\n> there's a good reason to flip the default around to not having other\n> stuff imply --syslog, and have users specify this implictly, then we\n> won't need a --no-syslog option.\n> \n> But maybe that'll break too much stuff.\n> \n\nThat seems risky to me – even with systemd, the StandardError directive\nby default inherits StandardOutput, so if you set StandardOutput=socket\nwithout StandardError=journal, log output in stderr clobbers regular\noutput. (Also, stderr is apparently closed with --detach, see below.)\n\nOn 23.01.2018 19:30, Junio C Hamano wrote:\n> Ævar Arnfjörð Bjarmason writes:\n> \n>> On Mon, Jan 22 2018, Lucas Werkmeister jotted:\n>> \n>>> Several options imply --syslog, without there being a way to\n>>> disable it again. This commit adds that option.\n>> \n>> Just two options imply --syslog, --detach & --inetd, unless I've\n>> missed something, anyway 2 != several, so maybe just say \"The\n>> --detach and --inetd options imply --syslog ...\".\n> \n> Correct.\n\nI respectfully disagree on “2 != several”, but sure, I can repeat the\ntwo options in the message instead :)\n\n> Moreover, --detach completely dissociates the process from the\n> original set of standard file descriptors by first closing them and\n> then connecting it to \"/dev/null\", so it will be nonsense to use this\n> new option with it.\n> \n\nAh, I wasn’t aware of that – so with --detach, --no-syslog would be\nbetter described as “disables all logging” rather than “log to stderr\ninstead”. IMHO it would still make sense to have the --no-syslog option\n(then mainly for use with --inetd) as long as its interaction with\n--inetd is properly documented… do you agree? If yes, I’ll be glad to\nsubmit another version of the patch.\n\nBest regards,\nLucas\n\n"},{"id":"337225","messageId":"b9bde80b-5da8-0d5b-f0a9-4ca1f1a74b23@lucaswerkmeister.de","threadId":"47669","inReplyTo":"18ef807d-a642-17b2-fc32-af0c3b963a71@lucaswerkmeister.de","subject":"Re: [PATCH] daemon: add --no-syslog to undo implicit --syslog","fromName":"Lucas Werkmeister","fromEmail":"mail@lucaswerkmeister.de","sentAt":"2018-01-24T10:05:26Z","receivedAt":"2018-01-24T10:05:35Z","isPatch":true,"sender":{"key":"mail@lucaswerkmeister.de","avatar":"https://avatars.githubusercontent.com/u/2346599?v=4"},"body":"On 23.01.2018 23:06, Lucas Werkmeister wrote:\n> Ah, I wasn’t aware of that – so with --detach, --no-syslog would be\n> better described as “disables all logging” rather than “log to stderr\n> instead”. IMHO it would still make sense to have the --no-syslog option\n> (then mainly for use with --inetd) as long as its interaction with\n> --inetd is properly documented… do you agree? If yes, I’ll be glad to\n> submit another version of the patch.\n\nOne alternative idea would be to instead make syslog and stderr logging\northogonal by adding not just --no-syslog, but also --[no-]stderr. For\nbackwards compatibility, --syslog would imply --no-stderr, but you could\nalso enable logging to both channels with --syslog --stderr, or disable\nall logging with --no-syslog --no-stderr. But that doesn’t really solve\nthe interaction with --detach – --detach --stderr would still be a\nnonsensical combination.\n\n"},{"id":"337278","messageId":"xmqqtvvbds42.fsf@gitster.mtv.corp.google.com","threadId":"47669","inReplyTo":"18ef807d-a642-17b2-fc32-af0c3b963a71@lucaswerkmeister.de","subject":"Re: [PATCH] daemon: add --no-syslog to undo implicit --syslog","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-01-24T18:33:17Z","receivedAt":"2018-01-24T18:33:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lucas Werkmeister writes:\n\n>> Moreover, --detach completely dissociates the process from the\n>> original set of standard file descriptors by first closing them and\n>> then connecting it to \"/dev/null\", so it will be nonsense to use this\n>> new option with it.\n>\n> Ah, I wasn’t aware of that – so with --detach, --no-syslog would be\n> better described as “disables all logging” rather than “log to stderr\n> instead”. IMHO it would still make sense to have the --no-syslog option\n> (then mainly for use with --inetd) as long as its interaction with\n> --inetd is properly documented.\n\nBecause \"--detach --no-syslog\" is a roundabout way to ask for\nsending the log to _nowhere_, I actually would say that \"nonsense\"\nis a bit too strong a word for the combination of your thing with\n\"--detach\".\n\nIt might make more sense to introduce a new \"--send-log-to=\"\noption, where the destination can be one of: syslog, stderr, none.\n\nThe you can make the current \"--syslog\" option a synonym to\n\"--send-log-to=syslog\". The internal variable log_syslog would\nprobably become\n\n\tenum log_destination { \n\t\tLOG_TO_NONE = -1,\n\t\tLOG_TO_STDERR = 0,\n\t\tLOG_TO_SYSLOG = 1,\n\t} log_destination;\n\nand wherever the current code assigns 1 to log_syslog, you would be\nsetting it LOG_TO_SYSLOG.\n\nThen those who want no log can express that wish in a more direct\nway, i.e. \"daemon --send-log-to=none\", perhaps.\n\nSuch an approach leaves open room for future enhancement. It is not\ntoo far-fetched to imagine something like:\n\n\tgit daemon --send-log-to=/var/log/git-daemon.log\n\nby introducing the fourth value to \"enum log_destination\"; perhaps\nthe file is opened and connected to stderr to accept the logs,\ncombined with a new feature that tells the daemon to close and\nreopen the log file when it receives a HUP or something like that.\n\n\n\n\n\n\n"},{"id":"337289","messageId":"740d5b61-884d-76df-a226-9d8a1245f528@lucaswerkmeister.de","threadId":"47669","inReplyTo":"xmqqtvvbds42.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] daemon: add --no-syslog to undo implicit --syslog","fromName":"Lucas Werkmeister","fromEmail":"mail@lucaswerkmeister.de","sentAt":"2018-01-24T19:48:20Z","receivedAt":"2018-01-24T19:48:29Z","isPatch":true,"sender":{"key":"mail@lucaswerkmeister.de","avatar":"https://avatars.githubusercontent.com/u/2346599?v=4"},"body":"On 24.01.2018 19:33, Junio C Hamano wrote:\n> Lucas Werkmeister writes:\n> \n>>> Moreover, --detach completely dissociates the process from the\n>>> original set of standard file descriptors by first closing them and\n>>> then connecting it to \"/dev/null\", so it will be nonsense to use this\n>>> new option with it.\n>>\n>> Ah, I wasn’t aware of that – so with --detach, --no-syslog would be\n>> better described as “disables all logging” rather than “log to stderr\n>> instead”. IMHO it would still make sense to have the --no-syslog option\n>> (then mainly for use with --inetd) as long as its interaction with\n>> --inetd is properly documented.\n> \n> Because \"--detach --no-syslog\" is a roundabout way to ask for\n> sending the log to _nowhere_, I actually would say that \"nonsense\"\n> is a bit too strong a word for the combination of your thing with\n> \"--detach\".\n> \n> It might make more sense to introduce a new \"--send-log-to=\"\n> option, where the destination can be one of: syslog, stderr, none.\n> \n> The you can make the current \"--syslog\" option a synonym to\n> \"--send-log-to=syslog\". The internal variable log_syslog would\n> probably become\n> \n> \tenum log_destination { \n> \t\tLOG_TO_NONE = -1,\n> \t\tLOG_TO_STDERR = 0,\n> \t\tLOG_TO_SYSLOG = 1,\n> \t} log_destination;\n> \n> and wherever the current code assigns 1 to log_syslog, you would be\n> setting it LOG_TO_SYSLOG.\n> \n> Then those who want no log can express that wish in a more direct\n> way, i.e. \"daemon --send-log-to=none\", perhaps.\n> \n> Such an approach leaves open room for future enhancement. It is not\n> too far-fetched to imagine something like:\n> \n> \tgit daemon --send-log-to=/var/log/git-daemon.log\n> \n> by introducing the fourth value to \"enum log_destination\"; perhaps\n> the file is opened and connected to stderr to accept the logs,\n> combined with a new feature that tells the daemon to close and\n> reopen the log file when it receives a HUP or something like that.\n\nSounds interesting… do you think it would be worth it supporting\nmultiple destinations? Right now this could be implemented fairly easily\nby making log_destination a bit field (and --syslog would then imply\n--send-log-to=syslog --no-send-log-to=stderr or something like that). On\nthe other hand, that doesn’t allow for this nice trick of reusing the\nstderr fd for a log file in case of future enhancement.\n"},{"id":"337577","messageId":"20180127183132.19724-1-mail@lucaswerkmeister.de","threadId":"47669","inReplyTo":"xmqqtvvbds42.fsf@gitster.mtv.corp.google.com","subject":"[PATCH v2] daemon: add --send-log-to=(stderr|syslog|none)","fromName":"Lucas Werkmeister","fromEmail":"mail@lucaswerkmeister.de","sentAt":"2018-01-27T18:31:32Z","receivedAt":"2018-01-27T18:32:10Z","isPatch":true,"sender":{"key":"mail@lucaswerkmeister.de","avatar":"https://avatars.githubusercontent.com/u/2346599?v=4"},"body":"This makes it possible to use --inetd while still logging to standard\nerror. --syslog is retained as an alias for --send-log-to=syslog. A mode\nto disable logging explicitly is also provided.\n\nThe combination of --inetd with --send-log-to=stderr is useful, for\ninstance, when running `git daemon` as an instanced systemd service\n(with associated socket unit). In this case, log messages sent via\nsyslog are received by the journal daemon, but run the risk of being\nprocessed at a time when the `git daemon` process has already exited\n(especially if the process was very short-lived, e.g. due to client\nerror), so that the journal daemon can no longer read its cgroup and\nattach the message to the correct systemd unit (see systemd/systemd#2913\n[1]). Logging to stderr instead can solve this problem, because systemd\ncan connect stderr directly to the journal daemon, which then already\nknows which unit is associated with this stream.\n\n[1]: https://github.com/systemd/systemd/issues/2913\n\nSigned-off-by: Lucas Werkmeister \nHelped-by: Ævar Arnfjörð Bjarmason \nHelped-by: Junio C Hamano \n---\n\nNotes:\n This was originally “daemon: add --no-syslog to undo implicit\n --syslog”, but Junio pointed out that combining --no-syslog with\n --detach isn’t especially useful and suggested --send-log-to=\n instead. Is Helped-by: the right credit for this or should it be\n something else?\n \n I’m also not quite sure if the systemd part of the commit message is\n accurate – see my comment on the linked issue [2]. TL;DR: this might\n no longer be necessary on systemd v235. (I’m experiencing the\n problem on Debian Stretch, systemd v232.) As in the last patch, feel\n free to remove that part of the commit message.\n \n [2]: https://github.com/systemd/systemd/issues/2913#issuecomment-361002589\n\n Documentation/git-daemon.txt | 23 +++++++++++++++++++++--\n daemon.c | 43 ++++++++++++++++++++++++++++++++++++-------\n 2 files changed, 57 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt\nindex 3c91db7be..e973f4390 100644\n--- a/Documentation/git-daemon.txt\n+++ b/Documentation/git-daemon.txt\n@@ -20,6 +20,7 @@ SYNOPSIS\n \t [--inetd |\n \t [--listen=] [--port=]\n \t [--user= [--group=]]]\n+\t [--send-log-to=(stderr|syslog|none)]\n \t [...]\n \n DESCRIPTION\n@@ -110,8 +111,26 @@ OPTIONS\n \tzero for no limit.\n \n --syslog::\n-\tLog to syslog instead of stderr. Note that this option does not imply\n-\t--verbose, thus by default only error conditions will be logged.\n+\tShort for `--send-log-to=syslog`.\n+\n+--send-log-to=::\n+\tSend log messages to the specified destination.\n+\tNote that this option does not imply --verbose,\n+\tthus by default only error conditions will be logged.\n+\tThe defaults to `stderr`, and must be one of:\n++\n+--\n+stderr::\n+\tWrite to standard error.\n+\tNote that if `--detach` is specified,\n+\tthe process disconnects from the real standard error,\n+\tmaking this destination effectively equivalent to `none`.\n+syslog::\n+\tWrite to syslog, using the `git-daemon` identifier.\n+none::\n+\tDisable all logging.\n+--\n++\n \n --user-path::\n --user-path=::\ndiff --git a/daemon.c b/daemon.c\nindex e37e343d0..3d8e16ede 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -9,7 +9,11 @@\n #define initgroups(x, y) (0) /* nothing */\n #endif\n \n-static int log_syslog;\n+static enum log_destination {\n+\tLOG_TO_NONE = -1,\n+\tLOG_TO_STDERR = 0,\n+\tLOG_TO_SYSLOG = 1,\n+} log_destination;\n static int verbose;\n static int reuseaddr;\n static int informative_errors;\n@@ -25,6 +29,7 @@ static const char daemon_usage[] =\n \" [--access-hook=]\\n\"\n \" [--inetd | [--listen=] [--port=]\\n\"\n \" [--detach] [--user= [--group=]]\\n\"\n+\" [--send-log-to=(stderr|syslog|none)]\\n\"\n \" [...]\";\n \n /* List of acceptable pathname prefixes */\n@@ -74,11 +79,14 @@ static const char *get_ip_address(struct hostinfo *hi)\n \n static void logreport(int priority, const char *err, va_list params)\n {\n-\tif (log_syslog) {\n+\tswitch (log_destination) {\n+\tcase LOG_TO_SYSLOG: {\n \t\tchar buf[1024];\n \t\tvsnprintf(buf, sizeof(buf), err, params);\n \t\tsyslog(priority, \"%s\", buf);\n-\t} else {\n+\t\tbreak;\n+\t}\n+\tcase LOG_TO_STDERR: {\n \t\t/*\n \t\t * Since stderr is set to buffered mode, the\n \t\t * logging of different processes will not overlap\n@@ -88,6 +96,11 @@ static void logreport(int priority, const char *err, va_list params)\n \t\tvfprintf(stderr, err, params);\n \t\tfputc('\\n', stderr);\n \t\tfflush(stderr);\n+\t\tbreak;\n+\t}\n+\tcase LOG_TO_NONE: {\n+\t\tbreak;\n+\t}\n \t}\n }\n \n@@ -1289,7 +1302,7 @@ int cmd_main(int argc, const char **argv)\n \t\t}\n \t\tif (!strcmp(arg, \"--inetd\")) {\n \t\t\tinetd_mode = 1;\n-\t\t\tlog_syslog = 1;\n+\t\t\tlog_destination = LOG_TO_SYSLOG;\n \t\t\tcontinue;\n \t\t}\n \t\tif (!strcmp(arg, \"--verbose\")) {\n@@ -1297,9 +1310,25 @@ int cmd_main(int argc, const char **argv)\n \t\t\tcontinue;\n \t\t}\n \t\tif (!strcmp(arg, \"--syslog\")) {\n-\t\t\tlog_syslog = 1;\n+\t\t\tlog_destination = LOG_TO_SYSLOG;\n \t\t\tcontinue;\n \t\t}\n+\t\tif (skip_prefix(arg, \"--send-log-to=\", &v)) {\n+\t\t\tif (!strcmp(v, \"syslog\")) {\n+\t\t\t\tlog_destination = LOG_TO_SYSLOG;\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\t\t\telse if (!strcmp(v, \"stderr\")) {\n+\t\t\t\tlog_destination = LOG_TO_STDERR;\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\t\t\telse if (!strcmp(v, \"none\")) {\n+\t\t\t\tlog_destination = LOG_TO_NONE;\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\t\t\telse\n+\t\t\t\tdie(\"Unknown log destination %s\", v);\n+\t\t}\n \t\tif (!strcmp(arg, \"--export-all\")) {\n \t\t\texport_all_trees = 1;\n \t\t\tcontinue;\n@@ -1356,7 +1385,7 @@ int cmd_main(int argc, const char **argv)\n \t\t}\n \t\tif (!strcmp(arg, \"--detach\")) {\n \t\t\tdetach = 1;\n-\t\t\tlog_syslog = 1;\n+\t\t\tlog_destination = LOG_TO_SYSLOG;\n \t\t\tcontinue;\n \t\t}\n \t\tif (skip_prefix(arg, \"--user=\", &v)) {\n@@ -1402,7 +1431,7 @@ int cmd_main(int argc, const char **argv)\n \t\tusage(daemon_usage);\n \t}\n \n-\tif (log_syslog) {\n+\tif (log_destination == LOG_TO_SYSLOG) {\n \t\topenlog(\"git-daemon\", LOG_PID, LOG_DAEMON);\n \t\tset_die_routine(daemon_die);\n \t} else\n-- \n2.16.0\n\n"},{"id":"337597","messageId":"CAPig+cTetjQ9LSH68Fe5OTcj9TwQ9GSbGzdrjzHOhTAVFvrPxw@mail.gmail.com","threadId":"47669","inReplyTo":"20180127183132.19724-1-mail@lucaswerkmeister.de","subject":"Re: [PATCH v2] daemon: add --send-log-to=(stderr|syslog|none)","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-01-28T06:40:43Z","receivedAt":"2018-01-28T06:40:50Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Jan 27, 2018 at 1:31 PM, Lucas Werkmeister\n wrote:\n> This makes it possible to use --inetd while still logging to standard\n> error. --syslog is retained as an alias for --send-log-to=syslog. A mode\n> to disable logging explicitly is also provided.\n>\n> The combination of --inetd with --send-log-to=stderr is useful, for\n> instance, when running `git daemon` as an instanced systemd service\n> (with associated socket unit). In this case, log messages sent via\n> syslog are received by the journal daemon, but run the risk of being\n> processed at a time when the `git daemon` process has already exited\n> (especially if the process was very short-lived, e.g. due to client\n> error), so that the journal daemon can no longer read its cgroup and\n> attach the message to the correct systemd unit (see systemd/systemd#2913\n> [1]). Logging to stderr instead can solve this problem, because systemd\n> can connect stderr directly to the journal daemon, which then already\n> knows which unit is associated with this stream.\n\nThe purpose of this patch would be easier to fathom if the problem was\npresented first (systemd race condition), followed by the solution\n(ability to log to stderr even when using --inetd), followed finally\nby incidental notes (\"--syslog is retained as an alias...\" and ability\nto disable logging).\n\nNot sure, though, if it's worth a re-roll.\n\n> Signed-off-by: Lucas Werkmeister \n> Helped-by: Ævar Arnfjörð Bjarmason \n> Helped-by: Junio C Hamano \n> ---\n>\n> Notes:\n> This was originally “daemon: add --no-syslog to undo implicit\n> --syslog”, but Junio pointed out that combining --no-syslog with\n> --detach isn’t especially useful and suggested --send-log-to=\n> instead. Is Helped-by: the right credit for this or should it be\n> something else?\n\nHelped-by: is fine, though typically your Signed-off-by: would be last.\n\nI understand that Junio suggested the name --send-log-to=, but I\nwonder if the more concise --log= would be an possibility.\n\nMore below...\n\n> diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt\n> @@ -110,8 +111,26 @@ OPTIONS\n> +--send-log-to=::\n> + Send log messages to the specified destination.\n> + Note that this option does not imply --verbose,\n> + thus by default only error conditions will be logged.\n> + The defaults to `stderr`, and must be one of:\n\nPerhaps also update the documentation of --inetd to mention that its\nimplied --syslog can be overridden by --send-log-to=.\n\n> diff --git a/daemon.c b/daemon.c\n> @@ -74,11 +79,14 @@ static const char *get_ip_address(struct hostinfo *hi)\n>\n> static void logreport(int priority, const char *err, va_list params)\n> {\n> - if (log_syslog) {\n> + switch (log_destination) {\n> + case LOG_TO_SYSLOG: {\n> char buf[1024];\n> vsnprintf(buf, sizeof(buf), err, params);\n> syslog(priority, \"%s\", buf);\n> - } else {\n> + break;\n> + }\n> + case LOG_TO_STDERR: {\n\nThere aren't many instances of:\n\n case FOO: {\n\nin the code-base, but those that exist don't use braces around cases\nwhich don't need it, so perhaps drop it from the STDERR and NONE\ncases. (Probably not worth a re-roll, though.)\n\n> /*\n> * Since stderr is set to buffered mode, the\n> * logging of different processes will not overlap\n> @@ -88,6 +96,11 @@ static void logreport(int priority, const char *err, va_list params)\n> vfprintf(stderr, err, params);\n> fputc('\\n', stderr);\n> fflush(stderr);\n> + break;\n> + }\n> + case LOG_TO_NONE: {\n> + break;\n> + }\n> }\n\nConsecutive lines with braces at the same indentation level is rather\nodd (but see previous comment).\n\n> }\n>\n> @@ -1289,7 +1302,7 @@ int cmd_main(int argc, const char **argv)\n> }\n> if (!strcmp(arg, \"--inetd\")) {\n> inetd_mode = 1;\n> - log_syslog = 1;\n> + log_destination = LOG_TO_SYSLOG;\n\nHmm, so an invocation \"--inetd --send-log-to=stderr\" works as\nexpected, but \"--send-log-to=stderr --inetd\" doesn't; output goes to\nsyslog despite the explicit request for stderr. Counterintuitive. This\nshould probably distinguish between 'log_destination' being unset and\nset explicitly; if unset, then, and only then, have --inetd imply\nsyslog. Perhaps something like this:\n\n static enum log_destination {\n LOG_TO_UNSET = -1\n LOG_TO_NONE,\n LOG_TO_STDERR,\n LOG_TO_SYSLOG,\n } log_destination = LOG_TO_UNSET;\n\n if (!strcmp(arg, \"--inetd\")) {\n inetd_mode = 1;\n if (log_destination == LOG_TO_UNSET)\n log_destination = LOG_TO_SYSLOG;\n ...\n }\n ...\n if (log_destination == LOG_TO_UNSET)\n log_destination = LOG_TO_STDERR\n\n> continue;\n> }\n> @@ -1297,9 +1310,25 @@ int cmd_main(int argc, const char **argv)\n> + if (skip_prefix(arg, \"--send-log-to=\", &v)) {\n> + if (!strcmp(v, \"syslog\")) {\n> + log_destination = LOG_TO_SYSLOG;\n> + continue;\n> + }\n> + else if (!strcmp(v, \"stderr\")) {\n\nStyle: cuddle 'else' with the braces: } else if (...) {\n\n> + log_destination = LOG_TO_STDERR;\n> + continue;\n> + }\n> + else if (!strcmp(v, \"none\")) {\n> + log_destination = LOG_TO_NONE;\n> + continue;\n> + }\n> + else\n> + die(\"Unknown log destination %s\", v);\n\nEven though there is a mix of capitalized and lowercase-only error\nmessages in daemon.c, newly written code avoids capitalization so\nperhaps downcase \"unknown\".\n"},{"id":"337645","messageId":"1d98ac06-7cfd-c006-d3c5-9dcb385a17d8@lucaswerkmeister.de","threadId":"47669","inReplyTo":"CAPig+cTetjQ9LSH68Fe5OTcj9TwQ9GSbGzdrjzHOhTAVFvrPxw@mail.gmail.com","subject":"Re: [PATCH v2] daemon: add --send-log-to=(stderr|syslog|none)","fromName":"Lucas Werkmeister","fromEmail":"mail@lucaswerkmeister.de","sentAt":"2018-01-28T22:58:06Z","receivedAt":"2018-01-28T22:58:20Z","isPatch":true,"sender":{"key":"mail@lucaswerkmeister.de","avatar":"https://avatars.githubusercontent.com/u/2346599?v=4"},"body":"On 28.01.2018 07:40, Eric Sunshine wrote:\n> On Sat, Jan 27, 2018 at 1:31 PM, Lucas Werkmeister\n> wrote:\n>> This makes it possible to use --inetd while still logging to standard\n>> error. --syslog is retained as an alias for --send-log-to=syslog. A mode\n>> to disable logging explicitly is also provided.\n>>\n>> The combination of --inetd with --send-log-to=stderr is useful, for\n>> instance, when running `git daemon` as an instanced systemd service\n>> (with associated socket unit). In this case, log messages sent via\n>> syslog are received by the journal daemon, but run the risk of being\n>> processed at a time when the `git daemon` process has already exited\n>> (especially if the process was very short-lived, e.g. due to client\n>> error), so that the journal daemon can no longer read its cgroup and\n>> attach the message to the correct systemd unit (see systemd/systemd#2913\n>> [1]). Logging to stderr instead can solve this problem, because systemd\n>> can connect stderr directly to the journal daemon, which then already\n>> knows which unit is associated with this stream.\n> \n> The purpose of this patch would be easier to fathom if the problem was\n> presented first (systemd race condition), followed by the solution\n> (ability to log to stderr even when using --inetd), followed finally\n> by incidental notes (\"--syslog is retained as an alias...\" and ability\n> to disable logging).\n> \n> Not sure, though, if it's worth a re-roll.\n\nI didn’t want to sound like I was just scratching my own itch ;) I hope\nthis option is useful for other use-cases as well?\n\n> \n>> Signed-off-by: Lucas Werkmeister \n>> Helped-by: Ævar Arnfjörð Bjarmason \n>> Helped-by: Junio C Hamano \n>> ---\n>>\n>> Notes:\n>> This was originally “daemon: add --no-syslog to undo implicit\n>> --syslog”, but Junio pointed out that combining --no-syslog with\n>> --detach isn’t especially useful and suggested --send-log-to=\n>> instead. Is Helped-by: the right credit for this or should it be\n>> something else?\n> \n> Helped-by: is fine, though typically your Signed-off-by: would be last.\n> \n> I understand that Junio suggested the name --send-log-to=, but I\n> wonder if the more concise --log= would be an possibility.\n\n--log sounds to me like it could also indicate *what* to log (e. g. “log\nverbosely” or “don’t log client IPs”). But perhaps --log-to= ?\n\n> \n> More below...\n> \n>> diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt\n>> @@ -110,8 +111,26 @@ OPTIONS\n>> +--send-log-to=::\n>> + Send log messages to the specified destination.\n>> + Note that this option does not imply --verbose,\n>> + thus by default only error conditions will be logged.\n>> + The defaults to `stderr`, and must be one of:\n> \n> Perhaps also update the documentation of --inetd to mention that its\n> implied --syslog can be overridden by --send-log-to=.\n\nVery good idea!\n\n> \n>> diff --git a/daemon.c b/daemon.c\n>> @@ -74,11 +79,14 @@ static const char *get_ip_address(struct hostinfo *hi)\n>>\n>> static void logreport(int priority, const char *err, va_list params)\n>> {\n>> - if (log_syslog) {\n>> + switch (log_destination) {\n>> + case LOG_TO_SYSLOG: {\n>> char buf[1024];\n>> vsnprintf(buf, sizeof(buf), err, params);\n>> syslog(priority, \"%s\", buf);\n>> - } else {\n>> + break;\n>> + }\n>> + case LOG_TO_STDERR: {\n> \n> There aren't many instances of:\n> \n> case FOO: {\n> \n> in the code-base, but those that exist don't use braces around cases\n> which don't need it, so perhaps drop it from the STDERR and NONE\n> cases. (Probably not worth a re-roll, though.)\n\nGood point, forgot that part of the coding guidelines. Only the syslog\ncase needs it because the buf declaration can’t be labeled directly.\n\n> \n>> /*\n>> * Since stderr is set to buffered mode, the\n>> * logging of different processes will not overlap\n>> @@ -88,6 +96,11 @@ static void logreport(int priority, const char *err, va_list params)\n>> vfprintf(stderr, err, params);\n>> fputc('\\n', stderr);\n>> fflush(stderr);\n>> + break;\n>> + }\n>> + case LOG_TO_NONE: {\n>> + break;\n>> + }\n>> }\n> \n> Consecutive lines with braces at the same indentation level is rather\n> odd (but see previous comment).\n> \n>> }\n>>\n>> @@ -1289,7 +1302,7 @@ int cmd_main(int argc, const char **argv)\n>> }\n>> if (!strcmp(arg, \"--inetd\")) {\n>> inetd_mode = 1;\n>> - log_syslog = 1;\n>> + log_destination = LOG_TO_SYSLOG;\n> \n> Hmm, so an invocation \"--inetd --send-log-to=stderr\" works as\n> expected, but \"--send-log-to=stderr --inetd\" doesn't; output goes to\n> syslog despite the explicit request for stderr. Counterintuitive. This\n> should probably distinguish between 'log_destination' being unset and\n> set explicitly; if unset, then, and only then, have --inetd imply\n> syslog. Perhaps something like this:\n> \n> static enum log_destination {\n> LOG_TO_UNSET = -1\n> LOG_TO_NONE,\n> LOG_TO_STDERR,\n> LOG_TO_SYSLOG,\n> } log_destination = LOG_TO_UNSET;\n> \n> if (!strcmp(arg, \"--inetd\")) {\n> inetd_mode = 1;\n> if (log_destination == LOG_TO_UNSET)\n> log_destination = LOG_TO_SYSLOG;\n> ...\n> }\n> ...\n> if (log_destination == LOG_TO_UNSET)\n> log_destination = LOG_TO_STDERR\n> \n\nI’m not sure if that’s worth the extra complication… some existing\noptions behave the same way already, e. g. in `git rebase --stat\n--quiet`, the `--stat` is ignored.\n\n>> continue;\n>> }\n>> @@ -1297,9 +1310,25 @@ int cmd_main(int argc, const char **argv)\n>> + if (skip_prefix(arg, \"--send-log-to=\", &v)) {\n>> + if (!strcmp(v, \"syslog\")) {\n>> + log_destination = LOG_TO_SYSLOG;\n>> + continue;\n>> + }\n>> + else if (!strcmp(v, \"stderr\")) {\n> \n> Style: cuddle 'else' with the braces: } else if (...) {\n>\n\nIs that a general rule? I couldn’t find anything about it in\nCodingGuidelines and daemon.c seemed to use both styles about evenly, so\nI wasn’t sure what to use.\n\n>> + log_destination = LOG_TO_STDERR;\n>> + continue;\n>> + }\n>> + else if (!strcmp(v, \"none\")) {\n>> + log_destination = LOG_TO_NONE;\n>> + continue;\n>> + }\n>> + else\n>> + die(\"Unknown log destination %s\", v);\n> \n> Even though there is a mix of capitalized and lowercase-only error\n> messages in daemon.c, newly written code avoids capitalization so\n> perhaps downcase \"unknown\".\n> \n\nOkay, thanks!\n\nI’ll prepare a new version of the patch, but will wait a bit before\nsending it in case there’s more feedback. But thanks for your reply :)\n"},{"id":"337646","messageId":"CAPig+cR1VXtc-wZxv3mZGsbTAzmtPbhnRPtBX97-8Rm9b6rpHQ@mail.gmail.com","threadId":"47669","inReplyTo":"1d98ac06-7cfd-c006-d3c5-9dcb385a17d8@lucaswerkmeister.de","subject":"Re: [PATCH v2] daemon: add --send-log-to=(stderr|syslog|none)","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-01-29T00:10:29Z","receivedAt":"2018-01-29T00:10:35Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Jan 28, 2018 at 5:58 PM, Lucas Werkmeister\n wrote:\n> On 28.01.2018 07:40, Eric Sunshine wrote:\n>> On Sat, Jan 27, 2018 at 1:31 PM, Lucas Werkmeister\n>> wrote:\n>>> This makes it possible to use --inetd while still logging to standard\n>>> error. --syslog is retained as an alias for --send-log-to=syslog. A mode\n>>> to disable logging explicitly is also provided.\n>>>\n>>> The combination of --inetd with --send-log-to=stderr is useful, for\n>>> instance, when running `git daemon` as an instanced systemd service\n>>> (with associated socket unit). In this case, log messages sent via\n>>> syslog are received by the journal daemon, but run the risk of being\n>>> processed at a time when the `git daemon` process has already exited\n>>> (especially if the process was very short-lived, e.g. due to client\n>>> error), so that the journal daemon can no longer read its cgroup and\n>>> attach the message to the correct systemd unit (see systemd/systemd#2913\n>>> [1]). Logging to stderr instead can solve this problem, because systemd\n>>> can connect stderr directly to the journal daemon, which then already\n>>> knows which unit is associated with this stream.\n>>\n>> The purpose of this patch would be easier to fathom if the problem was\n>> presented first (systemd race condition), followed by the solution\n>> (ability to log to stderr even when using --inetd), followed finally\n>> by incidental notes (\"--syslog is retained as an alias...\" and ability\n>> to disable logging).\n>>\n>> Not sure, though, if it's worth a re-roll.\n>\n> I didn’t want to sound like I was just scratching my own itch ;) I hope\n> this option is useful for other use-cases as well?\n\nIf the reader does not know that --inetd implies --syslog, then\n\n This makes it possible to use --inetd while still logging to\n standard error.\n\nleads to a \"Huh?\" moment since it is not self-contained. Had it said\n\n Add new option --send-log-to=(stderr|syslog|none) which\n allows the implied --syslog by --inetd to be overridden.\n\nit would have provided enough information to understand the purpose of\nthe patch at a glance. Talking about the systemd race-condition first\nwould also have explained the patch's purpose, and since the proposed\nsolution is general (not specific to your use-case), scratching an\nitch is not a point against it.\n\nAnyhow, it's not that big of a deal, but it did give me a bit of a\npause when reading the first paragraph since it's customary on this\nproject to start by explaining the problem.\n\n>> I understand that Junio suggested the name --send-log-to=, but I\n>> wonder if the more concise --log= would be an possibility.\n>\n> --log sounds to me like it could also indicate *what* to log (e. g. “log\n> verbosely” or “don’t log client IPs”). But perhaps --log-to= ?\n\nPerhaps we can take into consideration precedent by other (non-Git)\ndaemon-like commands when naming this option. (None come to my mind\nimmediately, but I'm sure they're out there.)\n\n>>> if (!strcmp(arg, \"--inetd\")) {\n>>> inetd_mode = 1;\n>>> - log_syslog = 1;\n>>> + log_destination = LOG_TO_SYSLOG;\n>>\n>> Hmm, so an invocation \"--inetd --send-log-to=stderr\" works as\n>> expected, but \"--send-log-to=stderr --inetd\" doesn't; output goes to\n>> syslog despite the explicit request for stderr. Counterintuitive. This\n>> should probably distinguish between 'log_destination' being unset and\n>> set explicitly; if unset, then, and only then, have --inetd imply\n>> syslog. Perhaps something like this:\n>>\n>> static enum log_destination {\n>> LOG_TO_UNSET = -1\n>> LOG_TO_NONE,\n>> LOG_TO_STDERR,\n>> LOG_TO_SYSLOG,\n>> } log_destination = LOG_TO_UNSET;\n>>\n>> if (!strcmp(arg, \"--inetd\")) {\n>> inetd_mode = 1;\n>> if (log_destination == LOG_TO_UNSET)\n>> log_destination = LOG_TO_SYSLOG;\n>> ...\n>> }\n>> ...\n>> if (log_destination == LOG_TO_UNSET)\n>> log_destination = LOG_TO_STDERR\n>>\n>\n> I’m not sure if that’s worth the extra complication… some existing\n> options behave the same way already, e. g. in `git rebase --stat\n> --quiet`, the `--stat` is ignored.\n\nI took \"last one wins\" into consideration when writing the above but\nwas not convinced that it applies to this case since --inetd and\n--send-log-to= have no obvious relation to one another (unlike, say,\n--verbose and --quiet or other similar combinations). Unless one reads\nthe documentation very closely, output ending up in syslog despite\n\"--send-log-to=stderr --inetd\" is just way too counterintuitive and\nmay well lead to bug reports later on. Therefore, doing the additional\nwork now to stave off such bug reports is likely worthwhile.\n\n>>> + }\n>>> + else if (!strcmp(v, \"stderr\")) {\n>>\n>> Style: cuddle 'else' with the braces: } else if (...) {\n>>\n>\n> Is that a general rule? I couldn’t find anything about it in\n> CodingGuidelines and daemon.c seemed to use both styles about evenly, so\n> I wasn’t sure what to use.\n\nIt's not stated explicitly in CodingGuidelines, but there is one\nexample of cuddling 'else' with braces in the section talking about\nbraces and 'if' statements.\n"},{"id":"338154","messageId":"20180203230801.30345-1-mail@lucaswerkmeister.de","threadId":"47669","inReplyTo":"CAPig+cR1VXtc-wZxv3mZGsbTAzmtPbhnRPtBX97-8Rm9b6rpHQ@mail.gmail.com","subject":"[PATCH v3] daemon: add --log-destination=(stderr|syslog|none)","fromName":"Lucas Werkmeister","fromEmail":"mail@lucaswerkmeister.de","sentAt":"2018-02-03T23:08:01Z","receivedAt":"2018-02-03T23:08:38Z","isPatch":true,"sender":{"key":"mail@lucaswerkmeister.de","avatar":"https://avatars.githubusercontent.com/u/2346599?v=4"},"body":"This new option can be used to override the implicit --syslog of\n--inetd, or to disable all logging. (While --detach also implies\n--syslog, --log-destination=stderr with --detach is useless since\n--detach disassociates the process from the original stderr.) --syslog\nis retained as an alias for --log-destination=syslog.\n\n--log-destination always overrides implicit --syslog regardless of\noption order. This is different than the “last one wins” logic that\napplies to some implicit options elsewhere in Git, but should hopefully\nbe less confusing. (I also don’t know if *all* implicit options in Git\nfollow “last one wins”.)\n\nThe combination of --inetd with --log-destination=stderr is useful, for\ninstance, when running `git daemon` as an instanced systemd service\n(with associated socket unit). In this case, log messages sent via\nsyslog are received by the journal daemon, but run the risk of being\nprocessed at a time when the `git daemon` process has already exited\n(especially if the process was very short-lived, e.g. due to client\nerror), so that the journal daemon can no longer read its cgroup and\nattach the message to the correct systemd unit (see systemd/systemd#2913\n[1]). Logging to stderr instead can solve this problem, because systemd\ncan connect stderr directly to the journal daemon, which then already\nknows which unit is associated with this stream.\n\n[1]: https://github.com/systemd/systemd/issues/2913\n\nHelped-by: Ævar Arnfjörð Bjarmason \nHelped-by: Junio C Hamano \nHelped-by: Eric Sunshine \nSigned-off-by: Lucas Werkmeister \n---\n\nNotes:\n Next version on my quest to make git-daemon logging more reliable :)\n many thanks to Eric Sunshine for review. Main changes this version:\n \n - Rename --send-log-to to --log-destination, following the\n postgresql option. A cursory search didn’t turn up any other\n daemons with a similar option – suggestions welcome!\n - Make explicit --log-destination always override implicit --syslog,\n regardless of option order.\n\n Documentation/git-daemon.txt | 26 ++++++++++++++++++++++---\n daemon.c | 45 +++++++++++++++++++++++++++++++++++++-------\n 2 files changed, 61 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt\nindex 3c91db7be..a0106e6aa 100644\n--- a/Documentation/git-daemon.txt\n+++ b/Documentation/git-daemon.txt\n@@ -20,6 +20,7 @@ SYNOPSIS\n \t [--inetd |\n \t [--listen=] [--port=]\n \t [--user= [--group=]]]\n+\t [--log-destination=(stderr|syslog|none)]\n \t [...]\n \n DESCRIPTION\n@@ -80,7 +81,8 @@ OPTIONS\n \tdo not have the 'git-daemon-export-ok' file.\n \n --inetd::\n-\tHave the server run as an inetd service. Implies --syslog.\n+\tHave the server run as an inetd service. Implies --syslog (may be\n+\toverridden with `--log-destination=`).\n \tIncompatible with --detach, --port, --listen, --user and --group\n \toptions.\n \n@@ -110,8 +112,26 @@ OPTIONS\n \tzero for no limit.\n \n --syslog::\n-\tLog to syslog instead of stderr. Note that this option does not imply\n-\t--verbose, thus by default only error conditions will be logged.\n+\tShort for `--log-destination=syslog`.\n+\n+--log-destination=::\n+\tSend log messages to the specified destination.\n+\tNote that this option does not imply --verbose,\n+\tthus by default only error conditions will be logged.\n+\tThe defaults to `stderr`, and must be one of:\n++\n+--\n+stderr::\n+\tWrite to standard error.\n+\tNote that if `--detach` is specified,\n+\tthe process disconnects from the real standard error,\n+\tmaking this destination effectively equivalent to `none`.\n+syslog::\n+\tWrite to syslog, using the `git-daemon` identifier.\n+none::\n+\tDisable all logging.\n+--\n++\n \n --user-path::\n --user-path=::\ndiff --git a/daemon.c b/daemon.c\nindex e37e343d0..f28400e3f 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -9,7 +9,12 @@\n #define initgroups(x, y) (0) /* nothing */\n #endif\n \n-static int log_syslog;\n+static enum log_destination {\n+\tLOG_DESTINATION_UNSET = -1,\n+\tLOG_DESTINATION_NONE = 0,\n+\tLOG_DESTINATION_STDERR = 1,\n+\tLOG_DESTINATION_SYSLOG = 2,\n+} log_destination;\n static int verbose;\n static int reuseaddr;\n static int informative_errors;\n@@ -25,6 +30,7 @@ static const char daemon_usage[] =\n \" [--access-hook=]\\n\"\n \" [--inetd | [--listen=] [--port=]\\n\"\n \" [--detach] [--user= [--group=]]\\n\"\n+\" [--log-destination=(stderr|syslog|none)]\\n\"\n \" [...]\";\n \n /* List of acceptable pathname prefixes */\n@@ -74,11 +80,14 @@ static const char *get_ip_address(struct hostinfo *hi)\n \n static void logreport(int priority, const char *err, va_list params)\n {\n-\tif (log_syslog) {\n+\tswitch (log_destination) {\n+\tcase LOG_DESTINATION_SYSLOG: {\n \t\tchar buf[1024];\n \t\tvsnprintf(buf, sizeof(buf), err, params);\n \t\tsyslog(priority, \"%s\", buf);\n-\t} else {\n+\t\tbreak;\n+\t}\n+\tcase LOG_DESTINATION_STDERR:\n \t\t/*\n \t\t * Since stderr is set to buffered mode, the\n \t\t * logging of different processes will not overlap\n@@ -88,6 +97,10 @@ static void logreport(int priority, const char *err, va_list params)\n \t\tvfprintf(stderr, err, params);\n \t\tfputc('\\n', stderr);\n \t\tfflush(stderr);\n+\t\tbreak;\n+\tcase LOG_DESTINATION_NONE:\n+\tcase LOG_DESTINATION_UNSET:\n+\t\tbreak;\n \t}\n }\n \n@@ -1289,7 +1302,6 @@ int cmd_main(int argc, const char **argv)\n \t\t}\n \t\tif (!strcmp(arg, \"--inetd\")) {\n \t\t\tinetd_mode = 1;\n-\t\t\tlog_syslog = 1;\n \t\t\tcontinue;\n \t\t}\n \t\tif (!strcmp(arg, \"--verbose\")) {\n@@ -1297,9 +1309,22 @@ int cmd_main(int argc, const char **argv)\n \t\t\tcontinue;\n \t\t}\n \t\tif (!strcmp(arg, \"--syslog\")) {\n-\t\t\tlog_syslog = 1;\n+\t\t\tlog_destination = LOG_DESTINATION_SYSLOG;\n \t\t\tcontinue;\n \t\t}\n+\t\tif (skip_prefix(arg, \"--log-destination=\", &v)) {\n+\t\t\tif (!strcmp(v, \"syslog\")) {\n+\t\t\t\tlog_destination = LOG_DESTINATION_SYSLOG;\n+\t\t\t\tcontinue;\n+\t\t\t} else if (!strcmp(v, \"stderr\")) {\n+\t\t\t\tlog_destination = LOG_DESTINATION_STDERR;\n+\t\t\t\tcontinue;\n+\t\t\t} else if (!strcmp(v, \"none\")) {\n+\t\t\t\tlog_destination = LOG_DESTINATION_NONE;\n+\t\t\t\tcontinue;\n+\t\t\t} else\n+\t\t\t\tdie(\"Unknown log destination %s\", v);\n+\t\t}\n \t\tif (!strcmp(arg, \"--export-all\")) {\n \t\t\texport_all_trees = 1;\n \t\t\tcontinue;\n@@ -1356,7 +1381,6 @@ int cmd_main(int argc, const char **argv)\n \t\t}\n \t\tif (!strcmp(arg, \"--detach\")) {\n \t\t\tdetach = 1;\n-\t\t\tlog_syslog = 1;\n \t\t\tcontinue;\n \t\t}\n \t\tif (skip_prefix(arg, \"--user=\", &v)) {\n@@ -1402,7 +1426,14 @@ int cmd_main(int argc, const char **argv)\n \t\tusage(daemon_usage);\n \t}\n \n-\tif (log_syslog) {\n+\tif (log_destination == LOG_DESTINATION_UNSET) {\n+\t\tif (inetd_mode || detach)\n+\t\t\tlog_destination = LOG_DESTINATION_SYSLOG;\n+\t\telse\n+\t\t\tlog_destination = LOG_DESTINATION_STDERR;\n+\t}\n+\n+\tif (log_destination == LOG_DESTINATION_SYSLOG) {\n \t\topenlog(\"git-daemon\", LOG_PID, LOG_DAEMON);\n \t\tset_die_routine(daemon_die);\n \t} else\n-- \n2.16.1\n\n"},{"id":"338159","messageId":"CAPig+cS8hqaqwrzw9GXY9x0rgksPrCm_deHHw-AL6Tj8-bf1Og@mail.gmail.com","threadId":"47669","inReplyTo":"20180203230801.30345-1-mail@lucaswerkmeister.de","subject":"Re: [PATCH v3] daemon: add --log-destination=(stderr|syslog|none)","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-02-04T06:36:17Z","receivedAt":"2018-02-04T06:38:14Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Feb 3, 2018 at 6:08 PM, Lucas Werkmeister\n wrote:\n> This new option can be used to override the implicit --syslog of\n> --inetd, or to disable all logging. (While --detach also implies\n> --syslog, --log-destination=stderr with --detach is useless since\n> --detach disassociates the process from the original stderr.) --syslog\n> is retained as an alias for --log-destination=syslog.\n> [...]\n> Signed-off-by: Lucas Werkmeister \n\nThanks for the re-roll. There are a few comments below. Except for one\napparent bug, I'm not sure the others are worth a re-roll...\n\n> diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt\n> @@ -110,8 +112,26 @@ OPTIONS\n> +--log-destination=::\n> + Send log messages to the specified destination.\n> + Note that this option does not imply --verbose,\n> + thus by default only error conditions will be logged.\n> + The defaults to `stderr`, and must be one of:\n\nI wonder if this should say instead:\n\n The default destination is `stderr` unless `syslog`\n is implied by `--inetd` or `--detach`, ...\n\n> diff --git a/daemon.c b/daemon.c\n> @@ -9,7 +9,12 @@\n> -static int log_syslog;\n> +static enum log_destination {\n> + LOG_DESTINATION_UNSET = -1,\n> + LOG_DESTINATION_NONE = 0,\n> + LOG_DESTINATION_STDERR = 1,\n> + LOG_DESTINATION_SYSLOG = 2,\n> +} log_destination;\n\nDoesn't log_destination need to be initialized to\nLOG_DESTINATION_UNSET (see [1])? As it stands, being static, it's\ninitialized automatically to 0 (LOG_DESTINATION_NONE), which borks the\nlogic below.\n\n> @@ -74,11 +80,14 @@ static const char *get_ip_address(struct hostinfo *hi)\n> static void logreport(int priority, const char *err, va_list params)\n> {\n> + switch (log_destination) {\n> + case LOG_DESTINATION_SYSLOG: {\n> char buf[1024];\n> vsnprintf(buf, sizeof(buf), err, params);\n> syslog(priority, \"%s\", buf);\n> + break;\n> + }\n> + case LOG_DESTINATION_STDERR:\n> /*\n> * Since stderr is set to buffered mode, the\n> * logging of different processes will not overlap\n> @@ -88,6 +97,10 @@ static void logreport(int priority, const char *err, va_list params)\n> vfprintf(stderr, err, params);\n> fputc('\\n', stderr);\n> fflush(stderr);\n> + break;\n> + case LOG_DESTINATION_NONE:\n> + case LOG_DESTINATION_UNSET:\n> + break;\n\nSince LOG_DESTINATION_UNSET should never occur, perhaps this should be\nwritten as:\n\n case LOG_DESTINATION_NONE:\n break;\n case LOG_DESTINATION_UNSET:\n BUG(\"impossible destination: %d\", log_destination);\n\n> @@ -1297,9 +1309,22 @@ int cmd_main(int argc, const char **argv)\n> + if (skip_prefix(arg, \"--log-destination=\", &v)) {\n> + if (!strcmp(v, \"syslog\")) {\n> + log_destination = LOG_DESTINATION_SYSLOG;\n> + continue;\n> + } else if (!strcmp(v, \"stderr\")) {\n> + log_destination = LOG_DESTINATION_STDERR;\n> + continue;\n> + } else if (!strcmp(v, \"none\")) {\n> + log_destination = LOG_DESTINATION_NONE;\n> + continue;\n> + } else\n> + die(\"Unknown log destination %s\", v);\n\nMentioned previously[1], this probably ought to start with lowercase.\nIt also would be more readable to set off the unknown value with a\ncolon or quotes:\n\n die(\"unknown log destination '%s', v);\n\n> @@ -1402,7 +1426,14 @@ int cmd_main(int argc, const char **argv)\n> - if (log_syslog) {\n> + if (log_destination == LOG_DESTINATION_UNSET) {\n> + if (inetd_mode || detach)\n> + log_destination = LOG_DESTINATION_SYSLOG;\n> + else\n> + log_destination = LOG_DESTINATION_STDERR;\n> + }\n> +\n> + if (log_destination == LOG_DESTINATION_SYSLOG) {\n> openlog(\"git-daemon\", LOG_PID, LOG_DAEMON);\n> set_die_routine(daemon_die);\n\n[1]: https://public-inbox.org/git/CAPig+cTetjQ9LSH68Fe5OTcj9TwQ9GSbGzdrjzHOhTAVFvrPxw@mail.gmail.com/\n"},{"id":"338168","messageId":"772ae5fc-5f92-3494-613a-a44c0073e8d7@lucaswerkmeister.de","threadId":"47669","inReplyTo":"CAPig+cS8hqaqwrzw9GXY9x0rgksPrCm_deHHw-AL6Tj8-bf1Og@mail.gmail.com","subject":"Re: [PATCH v3] daemon: add --log-destination=(stderr|syslog|none)","fromName":"Lucas Werkmeister","fromEmail":"mail@lucaswerkmeister.de","sentAt":"2018-02-04T18:29:26Z","receivedAt":"2018-02-04T18:29:33Z","isPatch":true,"sender":{"key":"mail@lucaswerkmeister.de","avatar":"https://avatars.githubusercontent.com/u/2346599?v=4"},"body":"On 04.02.2018 07:36, Eric Sunshine wrote:\n> On Sat, Feb 3, 2018 at 6:08 PM, Lucas Werkmeister\n> wrote:\n>> This new option can be used to override the implicit --syslog of\n>> --inetd, or to disable all logging. (While --detach also implies\n>> --syslog, --log-destination=stderr with --detach is useless since\n>> --detach disassociates the process from the original stderr.) --syslog\n>> is retained as an alias for --log-destination=syslog.\n>> [...]\n>> Signed-off-by: Lucas Werkmeister \n> \n> Thanks for the re-roll. There are a few comments below. Except for one\n> apparent bug, I'm not sure the others are worth a re-roll...\n> \n>> diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt\n>> @@ -110,8 +112,26 @@ OPTIONS\n>> +--log-destination=::\n>> + Send log messages to the specified destination.\n>> + Note that this option does not imply --verbose,\n>> + thus by default only error conditions will be logged.\n>> + The defaults to `stderr`, and must be one of:\n> \n> I wonder if this should say instead:\n> \n> The default destination is `stderr` unless `syslog`\n> is implied by `--inetd` or `--detach`, ...\n> \n>> diff --git a/daemon.c b/daemon.c\n>> @@ -9,7 +9,12 @@\n>> -static int log_syslog;\n>> +static enum log_destination {\n>> + LOG_DESTINATION_UNSET = -1,\n>> + LOG_DESTINATION_NONE = 0,\n>> + LOG_DESTINATION_STDERR = 1,\n>> + LOG_DESTINATION_SYSLOG = 2,\n>> +} log_destination;\n> \n> Doesn't log_destination need to be initialized to\n> LOG_DESTINATION_UNSET (see [1])? As it stands, being static, it's\n> initialized automatically to 0 (LOG_DESTINATION_NONE), which borks the\n> logic below.\n\nThanks, I knew I’d forgotten something :)\n\n> \n>> @@ -74,11 +80,14 @@ static const char *get_ip_address(struct hostinfo *hi)\n>> static void logreport(int priority, const char *err, va_list params)\n>> {\n>> + switch (log_destination) {\n>> + case LOG_DESTINATION_SYSLOG: {\n>> char buf[1024];\n>> vsnprintf(buf, sizeof(buf), err, params);\n>> syslog(priority, \"%s\", buf);\n>> + break;\n>> + }\n>> + case LOG_DESTINATION_STDERR:\n>> /*\n>> * Since stderr is set to buffered mode, the\n>> * logging of different processes will not overlap\n>> @@ -88,6 +97,10 @@ static void logreport(int priority, const char *err, va_list params)\n>> vfprintf(stderr, err, params);\n>> fputc('\\n', stderr);\n>> fflush(stderr);\n>> + break;\n>> + case LOG_DESTINATION_NONE:\n>> + case LOG_DESTINATION_UNSET:\n>> + break;\n> \n> Since LOG_DESTINATION_UNSET should never occur, perhaps this should be\n> written as:\n> \n> case LOG_DESTINATION_NONE:\n> break;\n> case LOG_DESTINATION_UNSET:\n> BUG(\"impossible destination: %d\", log_destination);\n\nGood point, I didn’t know about the BUG() macro. But putting the\ndestination in the message seems unnecessary if it can only be a single\nvalue – or would you make this a default: case?\n\n> \n>> @@ -1297,9 +1309,22 @@ int cmd_main(int argc, const char **argv)\n>> + if (skip_prefix(arg, \"--log-destination=\", &v)) {\n>> + if (!strcmp(v, \"syslog\")) {\n>> + log_destination = LOG_DESTINATION_SYSLOG;\n>> + continue;\n>> + } else if (!strcmp(v, \"stderr\")) {\n>> + log_destination = LOG_DESTINATION_STDERR;\n>> + continue;\n>> + } else if (!strcmp(v, \"none\")) {\n>> + log_destination = LOG_DESTINATION_NONE;\n>> + continue;\n>> + } else\n>> + die(\"Unknown log destination %s\", v);\n> \n> Mentioned previously[1], this probably ought to start with lowercase.\n> It also would be more readable to set off the unknown value with a\n> colon or quotes:\n> \n> die(\"unknown log destination '%s', v);\n> \n>> @@ -1402,7 +1426,14 @@ int cmd_main(int argc, const char **argv)\n>> - if (log_syslog) {\n>> + if (log_destination == LOG_DESTINATION_UNSET) {\n>> + if (inetd_mode || detach)\n>> + log_destination = LOG_DESTINATION_SYSLOG;\n>> + else\n>> + log_destination = LOG_DESTINATION_STDERR;\n>> + }\n>> +\n>> + if (log_destination == LOG_DESTINATION_SYSLOG) {\n>> openlog(\"git-daemon\", LOG_PID, LOG_DAEMON);\n>> set_die_routine(daemon_die);\n> \n> [1]: https://public-inbox.org/git/CAPig+cTetjQ9LSH68Fe5OTcj9TwQ9GSbGzdrjzHOhTAVFvrPxw@mail.gmail.com/\n> \n\nI’ll send a new version shortly, also addressing your other comments\nwhich I didn’t reply to here.\n\nCheers,\nLucas\n\n"},{"id":"338169","messageId":"20180204183037.23162-1-mail@lucaswerkmeister.de","threadId":"47669","inReplyTo":"772ae5fc-5f92-3494-613a-a44c0073e8d7@lucaswerkmeister.de","subject":"[PATCH v4] daemon: add --log-destination=(stderr|syslog|none)","fromName":"Lucas Werkmeister","fromEmail":"mail@lucaswerkmeister.de","sentAt":"2018-02-04T18:30:37Z","receivedAt":"2018-02-04T18:31:28Z","isPatch":true,"sender":{"key":"mail@lucaswerkmeister.de","avatar":"https://avatars.githubusercontent.com/u/2346599?v=4"},"body":"This new option can be used to override the implicit --syslog of\n--inetd, or to disable all logging. (While --detach also implies\n--syslog, --log-destination=stderr with --detach is useless since\n--detach disassociates the process from the original stderr.) --syslog\nis retained as an alias for --log-destination=syslog.\n\n--log-destination always overrides implicit --syslog regardless of\noption order. This is different than the “last one wins” logic that\napplies to some implicit options elsewhere in Git, but should hopefully\nbe less confusing. (I also don’t know if *all* implicit options in Git\nfollow “last one wins”.)\n\nThe combination of --inetd with --log-destination=stderr is useful, for\ninstance, when running `git daemon` as an instanced systemd service\n(with associated socket unit). In this case, log messages sent via\nsyslog are received by the journal daemon, but run the risk of being\nprocessed at a time when the `git daemon` process has already exited\n(especially if the process was very short-lived, e.g. due to client\nerror), so that the journal daemon can no longer read its cgroup and\nattach the message to the correct systemd unit (see systemd/systemd#2913\n[1]). Logging to stderr instead can solve this problem, because systemd\ncan connect stderr directly to the journal daemon, which then already\nknows which unit is associated with this stream.\n\n[1]: https://github.com/systemd/systemd/issues/2913\n\nHelped-by: Ævar Arnfjörð Bjarmason \nHelped-by: Junio C Hamano \nHelped-by: Eric Sunshine \nSigned-off-by: Lucas Werkmeister \n---\n\nNotes:\n Fixes log_destination not being initialized correctly and some minor\n style issues.\n\n Documentation/git-daemon.txt | 28 ++++++++++++++++++++++++---\n daemon.c | 46 +++++++++++++++++++++++++++++++++++++-------\n 2 files changed, 64 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt\nindex 3c91db7be..56d54a489 100644\n--- a/Documentation/git-daemon.txt\n+++ b/Documentation/git-daemon.txt\n@@ -20,6 +20,7 @@ SYNOPSIS\n \t [--inetd |\n \t [--listen=] [--port=]\n \t [--user= [--group=]]]\n+\t [--log-destination=(stderr|syslog|none)]\n \t [...]\n \n DESCRIPTION\n@@ -80,7 +81,8 @@ OPTIONS\n \tdo not have the 'git-daemon-export-ok' file.\n \n --inetd::\n-\tHave the server run as an inetd service. Implies --syslog.\n+\tHave the server run as an inetd service. Implies --syslog (may be\n+\toverridden with `--log-destination=`).\n \tIncompatible with --detach, --port, --listen, --user and --group\n \toptions.\n \n@@ -110,8 +112,28 @@ OPTIONS\n \tzero for no limit.\n \n --syslog::\n-\tLog to syslog instead of stderr. Note that this option does not imply\n-\t--verbose, thus by default only error conditions will be logged.\n+\tShort for `--log-destination=syslog`.\n+\n+--log-destination=::\n+\tSend log messages to the specified destination.\n+\tNote that this option does not imply --verbose,\n+\tthus by default only error conditions will be logged.\n+\tThe must be one of:\n++\n+--\n+stderr::\n+\tWrite to standard error.\n+\tNote that if `--detach` is specified,\n+\tthe process disconnects from the real standard error,\n+\tmaking this destination effectively equivalent to `none`.\n+syslog::\n+\tWrite to syslog, using the `git-daemon` identifier.\n+none::\n+\tDisable all logging.\n+--\n++\n+The default destination is `syslog` if `--inetd` or `--detach` is specified,\n+otherwise `stderr`.\n \n --user-path::\n --user-path=::\ndiff --git a/daemon.c b/daemon.c\nindex e37e343d0..fb538e367 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -9,7 +9,12 @@\n #define initgroups(x, y) (0) /* nothing */\n #endif\n \n-static int log_syslog;\n+static enum log_destination {\n+\tLOG_DESTINATION_UNSET = -1,\n+\tLOG_DESTINATION_NONE = 0,\n+\tLOG_DESTINATION_STDERR = 1,\n+\tLOG_DESTINATION_SYSLOG = 2,\n+} log_destination = LOG_DESTINATION_UNSET;\n static int verbose;\n static int reuseaddr;\n static int informative_errors;\n@@ -25,6 +30,7 @@ static const char daemon_usage[] =\n \" [--access-hook=]\\n\"\n \" [--inetd | [--listen=] [--port=]\\n\"\n \" [--detach] [--user= [--group=]]\\n\"\n+\" [--log-destination=(stderr|syslog|none)]\\n\"\n \" [...]\";\n \n /* List of acceptable pathname prefixes */\n@@ -74,11 +80,14 @@ static const char *get_ip_address(struct hostinfo *hi)\n \n static void logreport(int priority, const char *err, va_list params)\n {\n-\tif (log_syslog) {\n+\tswitch (log_destination) {\n+\tcase LOG_DESTINATION_SYSLOG: {\n \t\tchar buf[1024];\n \t\tvsnprintf(buf, sizeof(buf), err, params);\n \t\tsyslog(priority, \"%s\", buf);\n-\t} else {\n+\t\tbreak;\n+\t}\n+\tcase LOG_DESTINATION_STDERR:\n \t\t/*\n \t\t * Since stderr is set to buffered mode, the\n \t\t * logging of different processes will not overlap\n@@ -88,6 +97,11 @@ static void logreport(int priority, const char *err, va_list params)\n \t\tvfprintf(stderr, err, params);\n \t\tfputc('\\n', stderr);\n \t\tfflush(stderr);\n+\t\tbreak;\n+\tcase LOG_DESTINATION_NONE:\n+\t\tbreak;\n+\tcase LOG_DESTINATION_UNSET:\n+\t\tBUG(\"log destination not initialized correctly\");\n \t}\n }\n \n@@ -1289,7 +1303,6 @@ int cmd_main(int argc, const char **argv)\n \t\t}\n \t\tif (!strcmp(arg, \"--inetd\")) {\n \t\t\tinetd_mode = 1;\n-\t\t\tlog_syslog = 1;\n \t\t\tcontinue;\n \t\t}\n \t\tif (!strcmp(arg, \"--verbose\")) {\n@@ -1297,9 +1310,22 @@ int cmd_main(int argc, const char **argv)\n \t\t\tcontinue;\n \t\t}\n \t\tif (!strcmp(arg, \"--syslog\")) {\n-\t\t\tlog_syslog = 1;\n+\t\t\tlog_destination = LOG_DESTINATION_SYSLOG;\n \t\t\tcontinue;\n \t\t}\n+\t\tif (skip_prefix(arg, \"--log-destination=\", &v)) {\n+\t\t\tif (!strcmp(v, \"syslog\")) {\n+\t\t\t\tlog_destination = LOG_DESTINATION_SYSLOG;\n+\t\t\t\tcontinue;\n+\t\t\t} else if (!strcmp(v, \"stderr\")) {\n+\t\t\t\tlog_destination = LOG_DESTINATION_STDERR;\n+\t\t\t\tcontinue;\n+\t\t\t} else if (!strcmp(v, \"none\")) {\n+\t\t\t\tlog_destination = LOG_DESTINATION_NONE;\n+\t\t\t\tcontinue;\n+\t\t\t} else\n+\t\t\t\tdie(\"unknown log destination '%s'\", v);\n+\t\t}\n \t\tif (!strcmp(arg, \"--export-all\")) {\n \t\t\texport_all_trees = 1;\n \t\t\tcontinue;\n@@ -1356,7 +1382,6 @@ int cmd_main(int argc, const char **argv)\n \t\t}\n \t\tif (!strcmp(arg, \"--detach\")) {\n \t\t\tdetach = 1;\n-\t\t\tlog_syslog = 1;\n \t\t\tcontinue;\n \t\t}\n \t\tif (skip_prefix(arg, \"--user=\", &v)) {\n@@ -1402,7 +1427,14 @@ int cmd_main(int argc, const char **argv)\n \t\tusage(daemon_usage);\n \t}\n \n-\tif (log_syslog) {\n+\tif (log_destination == LOG_DESTINATION_UNSET) {\n+\t\tif (inetd_mode || detach)\n+\t\t\tlog_destination = LOG_DESTINATION_SYSLOG;\n+\t\telse\n+\t\t\tlog_destination = LOG_DESTINATION_STDERR;\n+\t}\n+\n+\tif (log_destination == LOG_DESTINATION_SYSLOG) {\n \t\topenlog(\"git-daemon\", LOG_PID, LOG_DAEMON);\n \t\tset_die_routine(daemon_die);\n \t} else\n-- \n2.16.1\n\n"},{"id":"338170","messageId":"871si0mvo0.fsf@evledraar.gmail.com","threadId":"47669","inReplyTo":"20180204183037.23162-1-mail@lucaswerkmeister.de","subject":"Re: [PATCH v4] daemon: add --log-destination=(stderr|syslog|none)","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-02-04T18:55:43Z","receivedAt":"2018-02-04T18:55:52Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Feb 04 2018, Lucas Werkmeister jotted:\n\n> \t [--inetd |\n> \t [--listen=] [--port=]\n> \t [--user= [--group=]]]\n> +\t [--log-destination=(stderr|syslog|none)]\n\nI micronit, but maybe worthwhile to have a preceeding commit to fix up\nthat indentation of --listen and --user.\n\n> +--log-destination=::\n> +\tSend log messages to the specified destination.\n> +\tNote that this option does not imply --verbose,\n\nShould `` quote --verbose, although I see similar to the WS change I\nnoted above there's plenty of existing stuff in that doc doing it wrong.\n> +\t\t\t} else\n> +\t\t\t\tdie(\"unknown log destination '%s'\", v);\n\nShould be die(_(\"unknown..., i.e. use the _() macro.\n\nAnyway, this looks fine to be with our without these proposed\nbikeshedding changes above. Thanks.\n"},{"id":"338171","messageId":"0306739c-3fba-2528-f08f-831a31c202b0@lucaswerkmeister.de","threadId":"47669","inReplyTo":"871si0mvo0.fsf@evledraar.gmail.com","subject":"Re: [PATCH v4] daemon: add --log-destination=(stderr|syslog|none)","fromName":"Lucas Werkmeister","fromEmail":"mail@lucaswerkmeister.de","sentAt":"2018-02-04T18:58:57Z","receivedAt":"2018-02-04T18:59:06Z","isPatch":true,"sender":{"key":"mail@lucaswerkmeister.de","avatar":"https://avatars.githubusercontent.com/u/2346599?v=4"},"body":"On 04.02.2018 19:55, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Sun, Feb 04 2018, Lucas Werkmeister jotted:\n> \n>> \t [--inetd |\n>> \t [--listen=] [--port=]\n>> \t [--user= [--group=]]]\n>> +\t [--log-destination=(stderr|syslog|none)]\n> \n> I micronit, but maybe worthwhile to have a preceeding commit to fix up\n> that indentation of --listen and --user.\n\nI thought the indentation here is intentional, since we’re still inside\nthe [] pair (either --inetd or --listen, --port, …).\n\n> \n>> +--log-destination=::\n>> +\tSend log messages to the specified destination.\n>> +\tNote that this option does not imply --verbose,\n> \n> Should `` quote --verbose, although I see similar to the WS change I\n> noted above there's plenty of existing stuff in that doc doing it wrong.\n\nI could send a follow-up to consistently ``-quote all options in\ngit-daemon.txt… or would that be rejected as unnecessarily cluttering\nthe history or the `git blame`?\n\n>> +\t\t\t} else\n>> +\t\t\t\tdie(\"unknown log destination '%s'\", v);\n> \n> Should be die(_(\"unknown..., i.e. use the _() macro.\n> \n> Anyway, this looks fine to be with our without these proposed\n> bikeshedding changes above. Thanks.\n>\n\n"},{"id":"338173","messageId":"CAPig+cSOH-V9aZDALOEX56ENXChexOVWzsgKgtvDULryqP0Kyw@mail.gmail.com","threadId":"47669","inReplyTo":"20180204183037.23162-1-mail@lucaswerkmeister.de","subject":"Re: [PATCH v4] daemon: add --log-destination=(stderr|syslog|none)","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-02-04T19:36:14Z","receivedAt":"2018-02-04T19:36:20Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Feb 4, 2018 at 1:30 PM, Lucas Werkmeister\n wrote:\n> This new option can be used to override the implicit --syslog of\n> --inetd, or to disable all logging. (While --detach also implies\n> --syslog, --log-destination=stderr with --detach is useless since\n> --detach disassociates the process from the original stderr.) --syslog\n> is retained as an alias for --log-destination=syslog.\n> [...]\n> Signed-off-by: Lucas Werkmeister \n> ---\n> Notes:\n> Fixes log_destination not being initialized correctly and some minor\n> style issues.\n\nThanks. With the 'log_destination' initialization bug fixed, this\nversion looks good; I didn't find anything else worth commenting upon.\nÆvar's micronits[1] could be addressed by a follow-up patch (if\ndesirable), but probably needn't hold up this patch.\n\n[1]: https://public-inbox.org/git/871si0mvo0.fsf@evledraar.gmail.com/\n"},{"id":"338174","messageId":"CAPig+cQrG4MchSrgCYeyFf9__XxeEfZ-QGy7Gpzufe0abmbEug@mail.gmail.com","threadId":"47669","inReplyTo":"871si0mvo0.fsf@evledraar.gmail.com","subject":"Re: [PATCH v4] daemon: add --log-destination=(stderr|syslog|none)","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-02-04T19:44:20Z","receivedAt":"2018-02-04T19:44:26Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Feb 4, 2018 at 1:55 PM, Ævar Arnfjörð Bjarmason\n wrote:\n> On Sun, Feb 04 2018, Lucas Werkmeister jotted:\n>> [--inetd |\n>> [--listen=] [--port=]\n>> [--user= [--group=]]]\n>> + [--log-destination=(stderr|syslog|none)]\n>\n> I micronit, but maybe worthwhile to have a preceeding commit to fix up\n> that indentation of --listen and --user.\n\nThe '--listen' and '--user' lines are in the \"alternate\" ('|') branch\nof '--inetd' so, as Lucas points out, this indentation appears\nintentional, thus seems okay as-is.\n\n>> +--log-destination=::\n>> + Send log messages to the specified destination.\n>> + Note that this option does not imply --verbose,\n>\n> Should `` quote --verbose, although I see similar to the WS change I\n> noted above there's plenty of existing stuff in that doc doing it wrong.\n\nAs you mention, there are plenty of existing offenders already in this\nfile, so probably not worth a re-roll (perhaps Junio can fix this new\ninstance locally), but certainly good fodder for a follow-up patch.\n\n>> + } else\n>> + die(\"unknown log destination '%s'\", v);\n>\n> Should be die(_(\"unknown..., i.e. use the _() macro.\n\nNo message in this source file use _(...) yet so probably not worth a\nre-roll, but definitely something for a follow-up patch (by someone).\n"},{"id":"338226","messageId":"xmqqlgg7b855.fsf@gitster-ct.c.googlers.com","threadId":"47669","inReplyTo":"CAPig+cSOH-V9aZDALOEX56ENXChexOVWzsgKgtvDULryqP0Kyw@mail.gmail.com","subject":"Re: [PATCH v4] daemon: add --log-destination=(stderr|syslog|none)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-05T18:31:34Z","receivedAt":"2018-02-05T18:34:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Sun, Feb 4, 2018 at 1:30 PM, Lucas Werkmeister\n> wrote:\n>> This new option can be used to override the implicit --syslog of\n>> ...\n> Thanks. With the 'log_destination' initialization bug fixed, this\n> version looks good; I didn't find anything else worth commenting upon.\n> Ævar's micronits[1] could be addressed by a follow-up patch (if\n> desirable), but probably needn't hold up this patch.\n>\n> [1]: https://public-inbox.org/git/871si0mvo0.fsf@evledraar.gmail.com/\n\nNicely done. Thanks, all.\n\nWill queue.\n"},{"id":"338243","messageId":"87vafbkxkx.fsf@evledraar.gmail.com","threadId":"47669","inReplyTo":"0306739c-3fba-2528-f08f-831a31c202b0@lucaswerkmeister.de","subject":"Re: [PATCH v4] daemon: add --log-destination=(stderr|syslog|none)","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-02-05T20:09:34Z","receivedAt":"2018-02-05T20:09:42Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Feb 04 2018, Lucas Werkmeister jotted:\n\n> On 04.02.2018 19:55, Ævar Arnfjörð Bjarmason wrote:\n>> \n>> On Sun, Feb 04 2018, Lucas Werkmeister jotted:\n>> \n>>> \t [--inetd |\n>>> \t [--listen=] [--port=]\n>>> \t [--user= [--group=]]]\n>>> +\t [--log-destination=(stderr|syslog|none)]\n>> \n>> I micronit, but maybe worthwhile to have a preceeding commit to fix up\n>> that indentation of --listen and --user.\n>\n> I thought the indentation here is intentional, since we’re still inside\n> the [] pair (either --inetd or --listen, --port, …).\n\nYes, sorry. Nevrmind.\n\n>> \n>>> +--log-destination=::\n>>> +\tSend log messages to the specified destination.\n>>> +\tNote that this option does not imply --verbose,\n>> \n>> Should `` quote --verbose, although I see similar to the WS change I\n>> noted above there's plenty of existing stuff in that doc doing it wrong.\n>\n> I could send a follow-up to consistently ``-quote all options in\n> git-daemon.txt… or would that be rejected as unnecessarily cluttering\n> the history or the `git blame`?\n\nI don't think anyone would mind. Tiny doc cleanups are neat-o.\n\n>>> +\t\t\t} else\n>>> +\t\t\t\tdie(\"unknown log destination '%s'\", v);\n>> \n>> Should be die(_(\"unknown..., i.e. use the _() macro.\n>> \n>> Anyway, this looks fine to be with our without these proposed\n>> bikeshedding changes above. Thanks.\n>>\n\n"}]}