Ramsay Jones <[email protected]> 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 <[email protected]>
>> ---
>
> 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
