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

On Mon, Oct 18, 2010 at 6:31 PM, Jonathan Nieder [off-list ref] wrote:

A response to the general questions.

Thanks!

Erik Faye-Lund wrote:

quoted

On Fri, Oct 15, 2010 at 11:16 PM, Junio C Hamano [off-list ref] wrote:

quoted

quoted

Why does the signature even have to be different between the two to begin
with? I _think_ you have gid_t over there

We don't, so this is the primary reason.

Just to throw an idea out: you can also do something like

#ifndef NO_POSIX_GOODIES
struct credentials {
};
#else
struct credentials {
       struct passwd *pass;
       gid_t gid;
}
#endif

and pass a pointer to credentials around.

Yes, but that structure still needs to be filled somehow. I'm not sure
how this solves anything, really. Isn't it essentially another way of
wrapping an ifdef around the parameters inside main() (at least when
I've inlined serve() into main())?

quoted

quoted

This is especially
true if you are making the "drop-privileges" part a helper function, no?

I don't follow this part. What exactly becomes more true by having a
drop-privileges function?

(See linux-2.6.git:Documentation/SubmittingPatches, section "#ifdefs
are ugly".)

The ideal: never an #ifdef within a function.  (Well, the ideal is
no #ifdef-s in .c files, but that's harder to take seriously.)

#ifndef HAVE_POSIX_GOODIES
static int drop_privileges(...)
{
       return error("--user and --group not supported on this platform");
}
#endif
static int drop_privileges(...)
{
       ...
       do
       something
       ...
}
#endif

would make serve() look like

static int serve(...)
{
       int socknum, *socklist;

       ... setup socket ...

       if (want to drop privileges) {
               if (drop_privileges(...))
                       return -1;
       }

       return service_loop(socknum, socklist);
}

which should be quite readable even to a person only interested in the
!HAVE_POSIX_GOODIES case imho.  With some code rearrangement it could
be made nicer.  Now compare:

static int serve(...)
{
       int socknum, *socklist;

       ... setup socket ...

#ifdef HAVE_POSIX_GOODIES
       ...
       do
       things
       ...
#endif

       return service_loop(socknum, socklist);
}

Just my two cents.  Sorry I do not have something more substantive to
say.

You're leaving out the troublesome part, namely the glue between "if
(user_name)" in main(), and the "want to drop privileges"-stuff in
serve().

I could do a "struct credentials *cred = NULL;"  in main(), and assign
that inside "if (user_name)". But that'd leave a warning about
unreachable code in drop_privileges(), no?

I'm also getting the feeling that I'm being hinted at to implement
proper credential-dropping (ie filling out the windows-versions of the
code with something that makes sense for windows), but this isn't how
these things work on Windows. Daemons run as services on Windows, and
what user to run a service under is a system-administrator setting. In
fact, you can't even impersonate another user without having it's
password.

Turning git-daemon into a service is something that can be done later.
I've looked into it, and what seems to make the most sense is to have
a separate mode on git-daemon (or even another program), that starts
git-daemon as a subprocess. This is because of the way Windows
communicates with the service, requiring a message-loop that can be
terminated.