Ramsay Jones <ramsay@xxxxxxxxxxxxxxxxxxx> writes: > On 07/04/15 03:03, Shawn Landden wrote: >> systemd supports git-daemon's existing --inetd mode as well. >> --systemd allows git-daemon has the advantage of allowing one git-daemon >> to listen to multiple interfaces as well as the system one(s), >> and more allow git-daemon to not be spawned on every connection. >> >> Signed-off-by: Shawn Landden <shawn@xxxxxxxxxxxxxxx> >> --- > > I have not been following this patch review, but I just happened to > notice something while skimming the patch as this email floated by ... > >> Respond to review by Eric Sunshine here: >> http://marc.info/?l=git&m=142836529908207&w=2 >> > > [snip] > >> diff --git a/daemon.c b/daemon.c >> index 9ee2187..9880858 100644 >> --- a/daemon.c >> +++ b/daemon.c >> @@ -1,3 +1,7 @@ >> +#ifdef HAVE_SYSTEMD >> +# include <systemd/sd-daemon.h> >> +#endif >> + >> #include "cache.h" >> #include "pkt-line.h" >> #include "exec_cmd.h" >> @@ -28,7 +32,11 @@ static const char daemon_usage[] = >> " [--(enable|disable|allow-override|forbid-override)=<service>]\n" >> " [--access-hook=<path>]\n" >> " [--inetd | [--listen=<host_or_ipaddr>] [--port=<n>]\n" >> +#ifdef HAVE_SYSTEMD >> +" [--systemd | [--detach] [--user=<user> [--group=<group>]]]\n" /* exactly 80 characters */ >> +#else >> " [--detach] [--user=<user> [--group=<group>]]\n" >> +#endif >> " [<directory>...]"; >> >> /* List of acceptable pathname prefixes */ >> @@ -1176,11 +1184,23 @@ static void store_pid(const char *path) >> } >> >> static int serve(struct string_list *listen_addr, int listen_port, >> - struct credentials *cred) >> + struct credentials *cred, int systemd_mode) >> { >> struct socketlist socklist = { NULL, 0, 0 }; >> > > ... this hunk splits a statement in two ... > >> - socksetup(listen_addr, listen_port, &socklist); >> +#ifdef HAVE_SYSTEMD >> + if (systemd_mode) { >> + int i, n; >> + >> + n = sd_listen_fds(0); >> + ALLOC_GROW(socklist.list, socklist.nr + n, socklist.alloc); >> + for (i = 0; i < n; i++) >> + socklist.list[socklist.nr++] = SD_LISTEN_FDS_START + i; >> + } >> + >> + if (listen_addr->nr > 0 || !systemd_mode) >> +#endif >> + socksetup(listen_addr, listen_port, &socklist); > > ... here. I have not considered any possible subtlety in the code, but simply > considering the patch text, I think the following may be easier to read: > > #ifdef HAVE_SYSTEMD > if (systemd_mode) { > ... > } > > if (listen_addr->nr > 0 || !systemd_mode) > socksetup(listen_addr, listen_port, &socklist); > #else > socksetup(listen_addr, listen_port, &socklist); > #endif > > Or, maybe: > > #if !defined(HAVE_SYSTEMD) > socksetup(listen_addr, listen_port, &socklist); > #else > ... > #endif > > Or, ... ;-) Yes, I'd really prefer to avoid #if/#else#endif in the main codeflow. It is vastly prefereable, if you can arrange so, to have a set of helper functions #if USE_SYSTEMD static void enumerate_sockets(struct socklist *slist) { ... /* do systemd specific enumeration */ } #else static void enumerate_sockets(struct socklist *slist) { ... /* something else */ } #endif and then just call that helper from the main codeline. enumerate_sockets(&socklist); socksetup(...); without #ifdef/#else/#endif -- 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