[PATCH] dd doesn't return a failure status

[Kazuo TAKADA]

loic.grenie at gmail.com wrote:
{snip..}
>
>    It may be faster to do
>
>if ((w = write_and_stats(...)))
>  goto out_status;
>
>  and
>
>return w;
>
>  because write_and_stats returns 1 on failure. You can even tweak
>  it to return either EXIT_SUCCESS or EXIT_FAILURE and change the
>  if to:
>
>if ((w = write_and_stats(...)) == EXIT_SUCCESS)
>  goto out_status;
>
>  (which is the same but symbolically different).

I agree with your suggestion basically and I had already tried.
But, in other lines, the variable 'w' is set with the written size by
the function 'full_write_or_warn()'.

An exit status of zero must indicate success, and a nonzero value
indicates failure, therefore, 'return w' is not appropriate.

Well, if I take care of variable's meanings, it can be written as below.

----------------------------------------------------------------------
--- coreutils/dd.c.orig	2007-09-03 20:48:39.000000000 +0900
+++ coreutils/dd.c	2007-10-03 18:19:29.000000000 +0900
@@ -54,17 +54,17 @@
 	return n;
 }
 
-static bool write_and_stats(int fd, const void *buf, size_t len, size_t obs,
+static int write_and_stats(int fd, const void *buf, size_t len, size_t obs,
 	const char *filename)
 {
 	ssize_t n = full_write_or_warn(fd, buf, len, filename);
 	if (n < 0)
-		return 1;
+		return EXIT_FAILURE;
 	if (n == obs)
 		G.out_full++;
 	else if (n) /* > 0 */
 		G.out_part++;
-	return 0;
+	return EXIT_SUCCESS;
 }
 
 #if ENABLE_LFS
@@ -106,7 +106,8 @@
 #endif
 	};
 	size_t ibs = 512, obs = 512;
-	ssize_t n, w;
+	ssize_t n;
+	int retval = EXIT_SUCCESS;
 	char *ibuf, *obuf;
 	/* And these are all zeroed at once! */
 	struct {
@@ -303,18 +304,21 @@
 				tmp += d;
 				oc += d;
 				if (oc == obs) {
-					if (write_and_stats(ofd, obuf, obs, obs, outfile))
+					if ((retval = write_and_stats(ofd, obuf, obs, obs, outfile)))
 						goto out_status;
 					oc = 0;
 				}
 			}
-		} else if (write_and_stats(ofd, ibuf, n, obs, outfile))
+		} else if ((retval = write_and_stats(ofd, ibuf, n, obs, outfile)))
 			goto out_status;
 	}
 
 	if (ENABLE_FEATURE_DD_IBS_OBS && oc) {
-		w = full_write_or_warn(ofd, obuf, oc, outfile);
-		if (w < 0) goto out_status;
+		ssize_t w = full_write_or_warn(ofd, obuf, oc, outfile);
+		if (w < 0) {
+			retval = EXIT_FAILURE;
+			goto out_status;
+		}
 		if (w > 0)
 			G.out_part++;
 	}
@@ -330,5 +334,5 @@
  out_status:
 	dd_output_status(0);
 
-	return EXIT_SUCCESS;
+	return retval;
 }
----------------------------------------------------------------------

It may be clear on the point of the code's policy.
Thanks.

Kazuo TAKADA