Re: [v6 PATCH] daemon: add systemd support

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 9, 2015, 03:51 UTC
Message-ID: <xmqqr3ruarhx.fsf@gitster.dls.corp.google.com>
In-Reply-To: <5523B933.8080302@ramsay1.demon.co.uk>

Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:

Show 92 quoted lines

> 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@churchofgit.com>
>> ---
>
> 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