Patch which adds syslog support to git-shell

Patch which adds syslog support to git-shell

[Gerhard Gappmeier]

[-- Attachment #1.1: Type: text/plain, Size: 717 bytes --]

Hi

I'm not sure if this is the right list, but here is my first GIT patch.

I had a problem with git-shell and wanted to analyze it.
Unfortunately it does not contain any trace capabilities.
So I cloned git and added some basic syslog support.
After that I recognized that the current git version just works ;-)
but the syslog functionality is always a nice thing I think.
So here is the patch.

Merry X-Mas.

-- 

mit freundlichen Grüßen / best regards

Gerhard Gappmeier
ascolab GmbH - automation system communication laboratory
Tel.: +49 9131 691 123
Fax: +49 9131 691 128
Web: http://www.ascolab.com
GPG Key-Id: 5AAC50C4
GPG Fingerprint: 967A 15F1 2788 164D CCA3 6C46 07CD 6F82 5AAC 50C4

[-- Attachment #1.2: 0001-Added-syslog-functionality-to-git-shell.patch --]
[-- Type: text/x-patch, Size: 6536 bytes --]

From 0683e3a249419620abcf4363086cd53ec34972e8 Mon Sep 17 00:00:00 2001
From: Gerhard Gappmeier <gerhard.gappmeier@ascolab.com>
Date: Wed, 23 Dec 2009 17:19:34 +0100
Subject: [PATCH] Added syslog functionality to git-shell.

---
 shell.c |  192 ++++++++++++++++++++++++++++++++++++++-------------------------
 1 files changed, 115 insertions(+), 77 deletions(-)

diff --git a/shell.c b/shell.c
index e4864e0..4cdf385 100644
--- a/shell.c
+++ b/shell.c
@@ -1,103 +1,141 @@
+#include <syslog.h>
+#include <errno.h>
 #include "cache.h"
 #include "quote.h"
 #include "exec_cmd.h"
 #include "strbuf.h"
 
+/* Syslog defines */
+#define GIT_SYSLOG_IDENT    "git-shell"
+#define GIT_SYSLOG_OPTION   0
+#define GIT_SYSLOG_FACILITY LOG_LOCAL0
+
 static int do_generic_cmd(const char *me, char *arg)
 {
-	const char *my_argv[4];
+    const char *my_argv[4];
+    int ret = 0;
+    
+    setup_path();
+    if (!arg || !(arg = sq_dequote(arg))) {
+        syslog(LOG_INFO, "bad argument");
+        die("bad argument");
+    }
+    if (prefixcmp(me, "git-")) {
+        syslog(LOG_INFO, "bad command");
+        die("bad command");
+    }
+
+    my_argv[0] = me + 4;
+    my_argv[1] = arg;
+    my_argv[2] = NULL;
 
-	setup_path();
-	if (!arg || !(arg = sq_dequote(arg)))
-		die("bad argument");
-	if (prefixcmp(me, "git-"))
-		die("bad command");
+    syslog(LOG_INFO, "Executing '%s' '%s'.", my_argv[0], my_argv[1]);
 
-	my_argv[0] = me + 4;
-	my_argv[1] = arg;
-	my_argv[2] = NULL;
+    ret =  execv_git_cmd(my_argv);
+    if (ret == -1) {
+        syslog(LOG_ERR, " execv_git_cmd failed: %s\n", strerror(errno));
+    }
 
-	return execv_git_cmd(my_argv);
+    return ret;
 }
 
 static int do_cvs_cmd(const char *me, char *arg)
 {
-	const char *cvsserver_argv[3] = {
-		"cvsserver", "server", NULL
-	};
+    const char *cvsserver_argv[3] = {
+        "cvsserver", "server", NULL
+    };
+    int ret = 0;
+
+    if (!arg || strcmp(arg, "server")) {
+        syslog(LOG_INFO, "git-cvsserver only handles server: %s", arg);
+        die("git-cvsserver only handles server: %s", arg);
+    }
+
+    setup_path();
 
-	if (!arg || strcmp(arg, "server"))
-		die("git-cvsserver only handles server: %s", arg);
+    syslog(LOG_INFO, "Executing '%s' '%s'.", cvsserver_argv[0], cvsserver_argv[1]);
 
-	setup_path();
-	return execv_git_cmd(cvsserver_argv);
+    ret = execv_git_cmd(cvsserver_argv);
+    if (ret == -1) {
+        syslog(LOG_ERR, " execv_git_cmd failed: %s\n", strerror(errno));
+    }
+
+    return ret;
 }
 
 
 static struct commands {
-	const char *name;
-	int (*exec)(const char *me, char *arg);
+    const char *name;
+    int (*exec)(const char *me, char *arg);
 } cmd_list[] = {
-	{ "git-receive-pack", do_generic_cmd },
-	{ "git-upload-pack", do_generic_cmd },
-	{ "git-upload-archive", do_generic_cmd },
-	{ "cvs", do_cvs_cmd },
-	{ NULL },
+    { "git-receive-pack", do_generic_cmd },
+    { "git-upload-pack", do_generic_cmd },
+    { "git-upload-archive", do_generic_cmd },
+    { "cvs", do_cvs_cmd },
+    { NULL },
 };
 
 int main(int argc, char **argv)
 {
-	char *prog;
-	struct commands *cmd;
-	int devnull_fd;
-
-	/*
-	 * Always open file descriptors 0/1/2 to avoid clobbering files
-	 * in die().  It also avoids not messing up when the pipes are
-	 * dup'ed onto stdin/stdout/stderr in the child processes we spawn.
-	 */
-	devnull_fd = open("/dev/null", O_RDWR);
-	while (devnull_fd >= 0 && devnull_fd <= 2)
-		devnull_fd = dup(devnull_fd);
-	if (devnull_fd == -1)
-		die_errno("opening /dev/null failed");
-	close (devnull_fd);
-
-	/*
-	 * Special hack to pretend to be a CVS server
-	 */
-	if (argc == 2 && !strcmp(argv[1], "cvs server"))
-		argv--;
-
-	/*
-	 * We do not accept anything but "-c" followed by "cmd arg",
-	 * where "cmd" is a very limited subset of git commands.
-	 */
-	else if (argc != 3 || strcmp(argv[1], "-c"))
-		die("What do you think I am? A shell?");
-
-	prog = argv[2];
-	if (!strncmp(prog, "git", 3) && isspace(prog[3]))
-		/* Accept "git foo" as if the caller said "git-foo". */
-		prog[3] = '-';
-
-	for (cmd = cmd_list ; cmd->name ; cmd++) {
-		int len = strlen(cmd->name);
-		char *arg;
-		if (strncmp(cmd->name, prog, len))
-			continue;
-		arg = NULL;
-		switch (prog[len]) {
-		case '\0':
-			arg = NULL;
-			break;
-		case ' ':
-			arg = prog + len + 1;
-			break;
-		default:
-			continue;
-		}
-		exit(cmd->exec(cmd->name, arg));
-	}
-	die("unrecognized command '%s'", prog);
+    char *prog;
+    struct commands *cmd;
+    int devnull_fd;
+
+    /* Open syslog. */
+    openlog(GIT_SYSLOG_IDENT, GIT_SYSLOG_OPTION, GIT_SYSLOG_FACILITY);
+
+    /*
+     * Always open file descriptors 0/1/2 to avoid clobbering files
+     * in die().  It also avoids not messing up when the pipes are
+     * dup'ed onto stdin/stdout/stderr in the child processes we spawn.
+     */
+    devnull_fd = open("/dev/null", O_RDWR);
+    while (devnull_fd >= 0 && devnull_fd <= 2)
+        devnull_fd = dup(devnull_fd);
+    if (devnull_fd == -1)
+        die_errno("opening /dev/null failed");
+    close (devnull_fd);
+
+    /*
+     * Special hack to pretend to be a CVS server
+     */
+    if (argc == 2 && !strcmp(argv[1], "cvs server"))
+        argv--;
+
+    /*
+     * We do not accept anything but "-c" followed by "cmd arg",
+     * where "cmd" is a very limited subset of git commands.
+     */
+    else if (argc != 3 || strcmp(argv[1], "-c")) {
+        syslog(LOG_WARNING, "Invalid parameter '%s'", argv[1]);
+        die("What do you think I am? A shell?");
+    }
+
+    prog = argv[2];
+    if (!strncmp(prog, "git", 3) && isspace(prog[3]))
+        /* Accept "git foo" as if the caller said "git-foo". */
+        prog[3] = '-';
+
+    for (cmd = cmd_list ; cmd->name ; cmd++) {
+        int len = strlen(cmd->name);
+        char *arg;
+        if (strncmp(cmd->name, prog, len))
+            continue;
+        arg = NULL;
+        switch (prog[len]) {
+        case '\0':
+            arg = NULL;
+            break;
+        case ' ':
+            arg = prog + len + 1;
+            break;
+        default:
+            continue;
+        }
+        exit(cmd->exec(cmd->name, arg));
+    }
+
+    syslog(LOG_WARNING, "Somebody tried to execute an unallowed command '%s'", prog);
+    die("unrecognized command '%s'", prog);
 }
+
-- 
1.6.4.4


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: Patch which adds syslog support to git-shell

[Erik Faye-Lund]

Please read Documentation/SubmittingPatches. We prefer inline patches,
as they are easier to review.

On Wed, Dec 23, 2009 at 6:32 PM, Gerhard Gappmeier
<gerhard.gappmeier@ascolab.com> wrote:
> Hi
>
> I'm not sure if this is the right list, but here is my first GIT patch.
>

It's the right list :)

> I had a problem with git-shell and wanted to analyze it.
> Unfortunately it does not contain any trace capabilities.
> So I cloned git and added some basic syslog support.
> After that I recognized that the current git version just works ;-)
> but the syslog functionality is always a nice thing I think.
> So here is the patch.

Looking at your patch, I see there's a lot of white-space changes. Stuff like:

> -	const char *cvsserver_argv[3] = {
> -		"cvsserver", "server", NULL
> -	};
> +    const char *cvsserver_argv[3] = {
> +        "cvsserver", "server", NULL
> +    };

just makes this harder to review. Besides, we use tabs for indentation in git.

Also, I think it would be better to use set_die_routine() from usage.h
than to change all the die call-sites. This is what git-daemon does:

--->8---
	if (log_syslog) {
		openlog("git-daemon", LOG_PID, LOG_DAEMON);
		set_die_routine(daemon_die);
	}
--->8---
Look at daemon.c for the implementation of daemon_die().

+/* Syslog defines */
+#define GIT_SYSLOG_IDENT    "git-shell"
+#define GIT_SYSLOG_OPTION   0
+#define GIT_SYSLOG_FACILITY LOG_LOCAL0
+

Is this really needed? These are only used at one place. Just doing
+    openlog("git-shell", 0, LOG_LOCAL0);
would IMO be cleaner.

Anyway, this is all I bother to point out before I see an inlined,
white-space fixed patch.

> Merry X-Mas.

Happy holidays to you too :)

-- 
Erik "kusma" Faye-Lund

Re: Patch which adds syslog support to git-shell

[Erik Faye-Lund]

On Thu, Dec 24, 2009 at 12:38 PM, Erik Faye-Lund
<kusmabite@googlemail.com> wrote:
> Anyway, this is all I bother to point out before I see an inlined,
> white-space fixed patch.
>

Actually, I'll add one thing: using syslog in git-shell breaks the
Windows builds. I don't know about other platforms.

Currently, git-daemon is the only git-program that depends on syslog,
and it's excluded from Windows builds (but this is something that
might change soon).

-- 
Erik "kusma" Faye-Lund