Johannes Sixt <johannes.sixt@xxxxxxxxxx> writes:
> 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).
> 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?
> 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.
> 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.
> * 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.
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
