Re: [PATCH 1/5] Change git_connect() to return a struct child_process instead of a pid_t.
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 30, 2007, 20:43 UTC
- Message-ID
- <7vbqbjg9zz.fsf@gitster.siamese.dyndns.org>
- In-Reply-To
- <1191183001-5368-2-git-send-email-johannes.sixt@telecom.at>
Johannes Sixt <johannes.sixt@telecom.at> writes:
Show 5 quoted lines
> This prepares the API of git_connect() and finish_connect() to operate on > a struct child_process. Currently, we just use that object as a placeholder > for the pid that we used to return. A follow-up patch will change the > implementation of git_connect() and finish_connect() to make full use > of the object.
Good description, except removal of checks for negative return of the calling sites raised my eyebrow and had me spend a few more minutes to review than necessary (see below).
Show 10 quoted lines
> diff --git a/builtin-archive.c b/builtin-archive.c
> index 04385de..76db6cf 100644
> --- a/builtin-archive.c
> +++ b/builtin-archive.c
> @@ -30,7 +30,7 @@ static int run_remote_archiver(const char *remote, int argc,
> {
> char *url, buf[LARGE_PACKET_MAX];
> int fd[2], i, len, rv;
> - pid_t pid;
> + struct child_process *chld;Is "child" a reserved keyword or something that we need to avoid its use as an identifier?
Show 12 quoted lines
> const char *exec = "git-upload-archive"; > int exec_at = 0; > > @@ -46,9 +46,7 @@ static int run_remote_archiver(const char *remote, int argc, > } > > url = xstrdup(remote); > - pid = git_connect(fd, url, exec, 0); > - if (pid < 0) > - return pid; > + chld = git_connect(fd, url, exec, 0); >
Interesting. This and other callers of git_connect() were prepared to see git_connect() to report errors with negative PID but the callee simply died --- so this change is not regressing by removing an early error return. It would be better to have something like this in the commit log message:
Old code had early-return-on-error checks at the calling sites of git_connect(), but the callee simply died on errors without returning negative values. This patch removes such bogosity.
Show 11 quoted lines
> diff --git a/connect.c b/connect.c > index 3d5c4ab..f6dcab9 100644 > --- a/connect.c > +++ b/connect.c > @@ -468,21 +468,22 @@ char *get_port(char *host) > } > > /* > - * This returns 0 if the transport protocol does not need fork(2), > + * This returns NULL if the transport protocol does not need fork(2), > * or a process id if it does. Once done, finish the connection
It does not return a process ID anymore, so this comment needs to be updated. Instead, you now return a struct child_process that is newly allocated, and needs to be deallocated somehow. At the end of finish_connect() might be a good place to do so.
Show 25 quoted lines
> * with finish_connect() with the value returned from this function
> - * (it is safe to call finish_connect() with 0 to support the former
> + * (it is safe to call finish_connect() with NULL to support the former
> * case).
> *
> - * Does not return a negative value on error; it just dies.
> + * If it returns, the connect is successful; it just dies on errors.
> */
> -pid_t git_connect(int fd[2], char *url, const char *prog, int flags)
> +struct child_process *git_connect(int fd[2], char *url,
> + const char *prog, int flags)
> {
> ...
> -int finish_connect(pid_t pid)
> +int finish_connect(struct child_process *chld)
> {
> - if (pid == 0)
> + if (chld == NULL)
> return 0;
>
> - while (waitpid(pid, NULL, 0) < 0) {
> + while (waitpid(chld->pid, NULL, 0) < 0) {
> if (errno != EINTR)
> return -1;
> }But it seems you don't, leaking the struct. I see this is fixed in the next patch in the series, but it would be nicer to have the free from the very beginning.