Re: [PATCH v4 15/15] daemon: opt-out on features that require posix

[Erik Faye-Lund <kusmabite@gmail.com>]

On Thu, Oct 14, 2010 at 1:02 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Erik Faye-Lund <kusmabite@gmail.com> writes:
>
>> Windows does not supply the POSIX-functions fork(), setuuid(), setgid(),
>> setsid() and initgroups(). Disable support for --user, --group and
>> --detach if the NO_POSIX_GOODIES flag is set.
>>
>> MinGW doesn't have prototypes and headers for inet_ntop and inet_pton,
>> so include our implementation instead. MSVC does have, so avoid doing
>> so there.
>>
>> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
>> ---
>> diff --git a/daemon.c b/daemon.c
>> index 9b97b58..aa580f6 100644
>> --- a/daemon.c
>> +++ b/daemon.c
>> @@ -965,7 +969,12 @@ static void store_pid(const char *path)
>>               die_errno("failed to write pid file '%s'", path);
>>  }
>>
>> -static int serve(struct string_list *listen_addr, int listen_port, struct passwd *pass, gid_t gid)
>> +#ifndef NO_POSIX_GOODIES
>> +static struct passwd *pass;
>> +static gid_t gid;
>> +#endif
>> +
>> +static int serve(struct string_list *listen_addr, int listen_port)
>>  {
>>       struct socketlist socklist = { NULL, 0, 0 };
>>
>
> This is ugly.  Why did you need to make the arguments file-scope static?
>

To avoid having different signatures for the serve-function dependent
on NO_POSIX_GOODIES.

Do you have any other suggestions on how to do this? Perhaps I should
just move the logic in serve() to the end of main()? It's the only
call-site for the function, and would remove the need for a function
prototype all-together...

>> @@ -974,10 +983,12 @@ static int serve(struct string_list *listen_addr, int listen_port, struct passwd
>>               die("unable to allocate any listen sockets on port %u",
>>                   listen_port);
>>
>> +#ifndef NO_POSIX_GOODIES
>>       if (pass && gid &&
>>           (initgroups(pass->pw_name, gid) || setgid (gid) ||
>>            setuid(pass->pw_uid)))
>>               die("cannot drop privileges");
>> +#endif
>
> It would be cleaner to make a helper (e.g. "drop-privileges") that is a
> no-op on NO_POSIX_GOODIES platform, and call that without #ifdef here.
>

Sure, makes sense.

> The same aversion to too many #ifdef's apply to the rest of the patch.
>

I can remove some of them, like keeping the variables in main()
around, even though they'll be constant. That might cause some
compile-time warnings, though.