Re: [PATCH 3/3] Add sideband status report to git-archive protocol

[Franck Bui-Huu <vagabon.xyz@gmail.com>]

Junio C Hamano wrote:
> Using the refactored sideband code from existing upload-pack protocol,
> this lets the error condition and status output sent from the remote
> process to be shown locally.
> 
> Signed-off-by: Junio C Hamano <junkio@cox.net>
> ---

[snip]

> -}
> +	while (1) {
> +		struct pollfd pfd[2];
> +		char buf[16384];
> +		ssize_t sz;
> +		pid_t pid;
> +		int status;
> +
> +		pfd[0].fd = fd1[0];
> +		pfd[0].events = POLLIN;
> +		pfd[1].fd = fd2[0];
> +		pfd[1].events = POLLIN;
> +		if (poll(pfd, 2, -1) < 0) {
> +			if (errno != EINTR) {
> +				error("poll failed resuming: %s",
> +				      strerror(errno));
> +				sleep(1);
> +			}
> +			continue;
> +		}
> +		if (pfd[0].revents & (POLLIN|POLLHUP)) {
> +			/* Data stream ready */
> +			sz = read(pfd[0].fd, buf, sizeof(buf));
> +			send_sideband(1, 1, buf, sz, DEFAULT_PACKET_MAX);
> +		}
> +		if (pfd[1].revents & (POLLIN|POLLHUP)) {
> +			/* Status stream ready */
> +			sz = read(pfd[1].fd, buf, sizeof(buf));
> +			send_sideband(1, 2, buf, sz, DEFAULT_PACKET_MAX);
> +		}
>  
> +		if (((pfd[0].revents | pfd[1].revents) & POLLHUP) == 0)
> +			continue;
> +		/* did it die? */
> +		pid = waitpid(writer, &status, WNOHANG);
> +		if (!pid) {
> +			fprintf(stderr, "Hmph, HUP?\n");
> +			continue;
> +		}

I get a lot of "Hmph, HUP?" messages when testing "git-archive
--remote" command. One guess: this can be due to the fact that when
the writer process exits, it first closes its fd but do not send a
SIGCHLD signal right after to its parent.

Therefore poll() can return POLLHUP flag to the parent process but
waitpid still returns 0 because the writer process has still not sent
SIGCHLD signal to its parent.

How about this patch ? If poll() doesn't only return the single POLLIN
flag, then either the pipe has been closed because the write process
died or something wrong happened. In all these cases we can wait for
the writer process to exit then die.

-- >8 --

diff --git a/builtin-upload-archive.c b/builtin-upload-archive.c
index 42cb9f8..2ebe9a0 100644
--- a/builtin-upload-archive.c
+++ b/builtin-upload-archive.c
@@ -114,7 +114,6 @@ int cmd_upload_archive(int argc, const c
 		struct pollfd pfd[2];
 		char buf[16384];
 		ssize_t sz;
-		pid_t pid;
 		int status;
 
 		pfd[0].fd = fd1[0];
@@ -140,13 +139,11 @@ int cmd_upload_archive(int argc, const c
 			send_sideband(1, 2, buf, sz, LARGE_PACKET_MAX);
 		}
 
-		if (((pfd[0].revents | pfd[1].revents) & POLLHUP) == 0)
-			continue;
-		/* did it die? */
-		pid = waitpid(writer, &status, WNOHANG);
-		if (!pid) {
-			fprintf(stderr, "Hmph, HUP?\n");
+		if ((pfd[0].revents | pfd[1].revents) == POLLIN)
 			continue;
+
+		if (waitpid(writer, &status, 0) < 0) {
+			die("waitpid failed: %s", strerror(errno));
 		}
 		if (!WIFEXITED(status) || WEXITSTATUS(status) > 0)
 			send_sideband(1, 3, deadchild, strlen(deadchild),