Re: [PATCH 1/3] connect: treat generic proxy processes like ssh processes
- From
Johannes Sixt <j6t@kdbg.org>
- Date
- May 16, 2011, 19:57 UTC
- Message-ID
- <4DD181C6.4020104@kdbg.org>
- In-Reply-To
- <20110516064607.GA19078@sigill.intra.peff.net>
Am 16.05.2011 08:46, schrieb Jeff King:
Show 20 quoted lines
> The git_connect function returns two ends of a pipe for > talking with a remote, plus a struct child_process > representing the other end of the pipe. If we have a direct > socket connection, then this points to a special "no_fork" > child process. > > The code path for doing git-over-pipes or git-over-ssh sets > up this child process to point to the child git command or > the ssh process. When we call finish_connect eventually, we > check wait() on the command and report its return value. > > The code path for git://, on the other hand, always sets it > to no_fork. In the case of a direct TCP connection, this > makes sense; we have no child process. But in the case of a > proxy command (configured by core.gitproxy), we do have a > child process, but we throw away its pid, and therefore > ignore its return code. > > Instead, let's keep that information in the proxy case, and > respect its return code, which can help catch some errors
This patch looks strikingly familiar. I had written an almost identical change more than 3 years ago and forgot about it, though the justification I noted in the commit was more to properly shutdown the proxy process rather than to abandon it and let it be collected by init(8). Your justification is much better.
There's one problem with your implementation, though:
Show 25 quoted lines
> -static void git_proxy_connect(int fd[2], char *host)
> +static struct child_process *git_proxy_connect(int fd[2], char *host)
> {
> const char *port = STR(DEFAULT_GIT_PORT);
> char *colon, *end;
> const char *argv[4];
> - struct child_process proxy;
> + struct child_process *proxy;
>
> if (host[0] == '[') {
> end = strchr(host + 1, ']');
> @@ -431,14 +431,15 @@ static void git_proxy_connect(int fd[2], char *host)
> argv[1] = host;
> argv[2] = port;
> argv[3] = NULL;
> - memset(&proxy, 0, sizeof(proxy));
> - proxy.argv = argv;
> - proxy.in = -1;
> - proxy.out = -1;
> - if (start_command(&proxy))
> + proxy = xcalloc(1, sizeof(*proxy));
> + proxy->argv = argv;
> + proxy->in = -1;
> + proxy->out = -1;
> + if (start_command(proxy))At this point, proxy->argv would point to automatic storage; but we need argv[0] in finish_command() for error reporting. In my implementation, I xmalloced the pointer array and leaked it. (And that's probably the reason that I never submitted the patch.) I wouldn't dare to make argv just static because this limits us to have just one open connection at a time established via git_proxy_connect(). Dunno...
Below is the interdiff that turns your patch into mine, mostly for exposition: The two hunks in git_connect() are just cosmetic differences. But the first hunk should be squashed into your patch to fix a potential crash in an error situation (e.g., when the proxy dies from a signal) at the cost of a small memory leak.
diff --git a/connect.c b/connect.c index a28e084..8668913 100644 --- a/connect.c +++ b/connect.c @@ -399,9 +399,10 @@ static struct child_process *git_proxy_connect(int fd[2], char *host) { const char *port = STR(DEFAULT_GIT_PORT); - const char *argv[4]; + const char **argv; struct child_process *proxy; get_host_and_port(&host, &port); + argv = xmalloc(4*sizeof(*argv)); argv[0] = git_proxy_command; argv[1] = host; @@ -457,5 +458,5 @@ struct child_process *git_connect(int fd[2], const char *url_orig, char *end; int c; - struct child_process *conn; + struct child_process *conn = &no_fork; enum protocol protocol = PROTO_LOCAL; int free_path = 0; @@ -543,8 +544,6 @@ struct child_process *git_connect(int fd[2], const char *url_orig, if (git_use_proxy(host)) conn = git_proxy_connect(fd, host); - else { + else git_tcp_connect(fd, host, flags); - conn = &no_fork; - } /* * Separate original protocol components prog and path