Re: [PATCH] use appropriate typedefs

David Rientjes [off-list ref] writes:

Since mkpath() is vararg, doesn't this make it necessary to cast
its parameter several lines down?

This is good.  Thanks.

Hmph.  You might have made finish_connect() to return pid_t so
making "ret" of that type is consistent with that change, but it
also gets return value from copy_fd() -- which is "did we error
out?"  This part smells funny...

This function wants to wait for the given process and return
zero on success otherwise the caller takes it as a sign for
failure.  Most existing callers do not check the return value,
which should be cleaned up, but we always call it with specific
pid, not wildcard values like 0 or -1, so returning pid_t to say
which child exited does not add value to the interface.  Please
leave its function signature (and one of the callers,
remote_tar() you changed above) as it is.

Having said that, I suspect the existing implementation is quite
buggy.  It says:

	for (;;) {
		ret = waitpid(pid, NULL, 0);
	        if (!ret)
                	break;
		if (errno != EINTR)
                	break;
	}
        return ret;

But it probably should read:

	for (;;) {
		pid_t ret = waitpid(pid, NULL, 0);
		if (ret < 0 && errno == EINTR)
			continue;
	        if (ret == pid)
			return 0;
		return -1;
	}

I do not remember what I was smoking when I wrote that code, but
I suspect somehow I incorrectly thought waitpid() would return
zero for success.

I then dig the history and find out that it was not me but Linus
who did this with commit f719259 on July 4th 2005.  Maybe this
was a program under influence, judging from the date of the
commit ;-)?

These all look good, thanks.

On Tue, 15 Aug 2006, Junio C Hamano wrote:

quoted

diff --git a/builtin-apply.c b/builtin-apply.c
index be2c715..2862eb1 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c

@@ -2097,7 +2097,7 @@ static void create_one_file(char *path, 
 	}
 
 	if (errno == EEXIST) {
-		unsigned int nr = getpid();
+		pid_t nr = getpid();

Since mkpath() is vararg, doesn't this make it necessary to cast
its parameter several lines down?

No, it is not necessary in the sense that any of these changes in this patch are 
necessary.  But since getpid() returns pid_t, every assignment should be cast as 
such.  pid_t can be typed as a short.

quoted

diff --git a/builtin-tar-tree.c b/builtin-tar-tree.c
index 215892b..6fed919 100644
--- a/builtin-tar-tree.c
+++ b/builtin-tar-tree.c

@@ -361,8 +361,8 @@ static const char *exec = "git-upload-ta
 
 static int remote_tar(int argc, const char **argv)
 {
-	int fd[2], ret, len;
-	pid_t pid;
+	int fd[2], len;
+	pid_t pid, ret;

Hmph.  You might have made finish_connect() to return pid_t so
making "ret" of that type is consistent with that change, but it
also gets return value from copy_fd() -- which is "did we error
out?"  This part smells funny...

I did make finish_connect return pid_t, so it is consistent.  Changing the use 
of ret is beyond the scope of a patch that changes types to typedefs.

quoted

diff --git a/connect.c b/connect.c
index 4422a0d..a4c02d1 100644
--- a/connect.c
+++ b/connect.c

@@ -735,9 +735,9 @@ int git_connect(int fd[2], char *url, co
 	return pid;
 }
 
-int finish_connect(pid_t pid)
+pid_t finish_connect(pid_t pid)
 {
-	int ret;
+	pid_t ret;

This function wants to wait for the given process and return
zero on success otherwise the caller takes it as a sign for
failure.  Most existing callers do not check the return value,
which should be cleaned up, but we always call it with specific
pid, not wildcard values like 0 or -1, so returning pid_t to say
which child exited does not add value to the interface.  Please
leave its function signature (and one of the callers,
remote_tar() you changed above) as it is.

Please cite where this function is specified to return zero on success and not 
the return value of waitpid which, after all, is the only assignment to the 
return value.  waitpid only returns when the status of the child is available or 
an error has occurred as a result of an interrupt.  The correct interface, in my 
opinion, for this function is to return what waitpid returns and allow it to 
indicate the pid of the child or interrupt to the caller.  The signature now 
suggests that.  If Linus did indeed write this, he did so to spin until the 
status of the child was known.

		David

On Tue, 15 Aug 2006, David Rientjes wrote:

Please cite where this function is specified to return zero on success and not 
the return value of waitpid which, after all, is the only assignment to the 
return value.  waitpid only returns when the status of the child is available or 
an error has occurred as a result of an interrupt.  The correct interface, in my 
opinion, for this function is to return what waitpid returns and allow it to 
indicate the pid of the child or interrupt to the caller.  The signature now 
suggests that.  If Linus did indeed write this, he did so to spin until the 
status of the child was known.

Forget this, the function is correct as is because EINTR is only returned on 
signal interrupt and ret is set to -1 (by the waitpid spec) implicitly.

Please replace the original patch with the following.


Replaces types with appropriate typedefs.

		David

Signed-off-by: David Rientjes <rientjes@google.com>
---
 builtin-apply.c |    2 +-
 fetch-clone.c   |    3 +--
 merge-index.c   |    3 ++-
 run-command.c   |    8 ++++----
 unpack-trees.c  |    2 +-
 5 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/builtin-apply.c b/builtin-apply.c
index 9cf477c..56c5394 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c

@@ -2097,7 +2097,7 @@ static void create_one_file(char *path, 
 	}
 
 	if (errno == EEXIST) {
-		unsigned int nr = getpid();
+		pid_t nr = getpid();
 
 		for (;;) {
 			const char *newpath;

diff --git a/fetch-clone.c b/fetch-clone.c
index 5e84c46..c5cf477 100644
--- a/fetch-clone.c
+++ b/fetch-clone.c

@@ -44,9 +44,8 @@ static int finish_pack(const char *pack_
 
 	for (;;) {
 		int status, code;
-		int retval = waitpid(pid, &status, 0);
 
-		if (retval < 0) {
+		if (waitpid(pid, &status, 0) < 0) {
 			if (errno == EINTR)
 				continue;
 			error("waitpid failed (%s)", strerror(errno));

diff --git a/merge-index.c b/merge-index.c
index 0498a6f..a9c8cc1 100644
--- a/merge-index.c
+++ b/merge-index.c

@@ -11,7 +11,8 @@ static int err;
 
 static void run_program(void)
 {
-	int pid = fork(), status;
+	pid_t pid = fork();
+	int status;
 
 	if (pid < 0)
 		die("unable to fork");

diff --git a/run-command.c b/run-command.c
index ca67ee9..3bacc1b 100644
--- a/run-command.c
+++ b/run-command.c

@@ -25,15 +25,15 @@ int run_command_v_opt(int argc, const ch
 	}
 	for (;;) {
 		int status, code;
-		int retval = waitpid(pid, &status, 0);
+		pid_t waiting = waitpid(pid, &status, 0);
 
-		if (retval < 0) {
+		if (waiting < 0) {
 			if (errno == EINTR)
 				continue;
-			error("waitpid failed (%s)", strerror(retval));
+			error("waitpid failed (%s)", strerror(waiting));
 			return -ERR_RUN_COMMAND_WAITPID;
 		}
-		if (retval != pid)
+		if (waiting != pid)
 			return -ERR_RUN_COMMAND_WAITPID_WRONG_PID;
 		if (WIFSIGNALED(status))
 			return -ERR_RUN_COMMAND_WAITPID_SIGNAL;

diff --git a/unpack-trees.c b/unpack-trees.c
index a20639b..e496d8c 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c

@@ -278,7 +278,7 @@ static void unlink_entry(char *name)
 	}
 }
 
-static volatile int progress_update = 0;
+static volatile sig_atomic_t progress_update = 0;
 
 static void progress_interval(int signum)
 {

-- 
1.4.2.g460c-dirty

David Rientjes [off-list ref] writes:

On Tue, 15 Aug 2006, David Rientjes wrote:

Please replace the original patch with the following.

(earlier)

quoted

Since mkpath() is vararg, doesn't this make it necessary to cast
its parameter several lines down?

No, it is not necessary in the sense that any of these changes
in this patch are necessary.  But since getpid() returns
pid_t, every assignment should be cast as such.  pid_t can be
typed as a short.

If pid_t is a short then wouldn't it be promoted to "unsigned
int" just fine, except for the failure case (but this is
getpid() we are dealing with here)?  More problematic is the
case where pid_t is wider than unsigned int, in which case we
can end up truncating the return value from getpid().

But for this particular case, I do not think it matters; the
code uses getpid() to seed the loop to obtain an unused
"~number" suffix; it could have used random(3) or time(2)
instead.  As long as:

	newpath = mkpath("%s~%u", path, nr);

does a reasonable thing, we are Ok.

I think, however, if you change the type of nr to pid_t, you
would need to cast it like this, because mkpath is defined to be
"char *mkpath(const char *, ...)":

	newpath = mkpath("%s~%u", path, (unsigned int) nr);

Or use whatever matching integral type and format letter pairs;
we seem to like "%lu" format and "(unsigned long)" in other
parts of our code.

Makes sense -- if pid_t is wider than int we would be in
trouble.

Same.

Same.

-			error("waitpid failed (%s)", strerror(retval));
+			error("waitpid failed (%s)", strerror(waiting));

Shouldn't this be "strerror(errno)"?  The original gets it wrong
already.

This matches the other one in builtin-pack-objects.c and makes
sense.

Thanks.