Re: [PATCH v6 5/6] Change copy_fd() to not close input fd

[Jeff King <peff@peff.net>]

On Tue, Aug 26, 2014 at 05:23:24PM +0200, Steffen Prohaska wrote:

> The caller opened the fd, so it should be responsible for closing it.
> 
> Signed-off-by: Steffen Prohaska <prohaska@zib.de>
> ---
>  copy.c     | 5 +----
>  lockfile.c | 3 +++
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/copy.c b/copy.c
> index a7f58fd..d0a1d82 100644
> --- a/copy.c
> +++ b/copy.c
> @@ -10,7 +10,6 @@ int copy_fd(int ifd, int ofd)
>  			break;
>  		if (len < 0) {
>  			int read_error = errno;
> -			close(ifd);
>  			return error("copy-fd: read returned %s",
>  				     strerror(read_error));
>  		}

This saved errno is not necessary anymore (the problem was that close()
clobbered the error in the original code). It can go away, and we can
even drop the curly braces.

> @@ -21,17 +20,14 @@ int copy_fd(int ifd, int ofd)
>  				len -= written;
>  			}
>  			else if (!written) {
> -				close(ifd);
>  				return error("copy-fd: write returned 0");
>  			} else {
>  				int write_error = errno;
> -				close(ifd);
>  				return error("copy-fd: write returned %s",
>  					     strerror(write_error));
>  			}
>  		}

Ditto here. Actually, isn't this whole write just a reimplementation of
write_in_full? The latter treats a return of 0 as ENOSPC rather than
using a custom message, but I think that is sane.

All together:

---
 copy.c | 28 +++++-----------------------
 1 file changed, 5 insertions(+), 23 deletions(-)

diff --git a/copy.c b/copy.c
index a7f58fd..53a9ece 100644
--- a/copy.c
+++ b/copy.c
@@ -4,34 +4,16 @@ int copy_fd(int ifd, int ofd)
 {
 	while (1) {
 		char buffer[8192];
-		char *buf = buffer;
 		ssize_t len = xread(ifd, buffer, sizeof(buffer));
 		if (!len)
 			break;
-		if (len < 0) {
-			int read_error = errno;
-			close(ifd);
+		if (len < 0)
 			return error("copy-fd: read returned %s",
-				     strerror(read_error));
-		}
-		while (len) {
-			int written = xwrite(ofd, buf, len);
-			if (written > 0) {
-				buf += written;
-				len -= written;
-			}
-			else if (!written) {
-				close(ifd);
-				return error("copy-fd: write returned 0");
-			} else {
-				int write_error = errno;
-				close(ifd);
-				return error("copy-fd: write returned %s",
-					     strerror(write_error));
-			}
-		}
+				     strerror(errno));
+		if (write_in_full(ofd, buffer, len) < 0)
+			return error("copy-fd: write returned %s",
+				     strerror(errno));
 	}
-	close(ifd);
 	return 0;
 }