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

> 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;
> }
>