Re: [PATCH] copy.c: make copy_fd preserve meaningful errno

[Michael Haggerty <mhagger@alum.mit.edu>]

On 11/17/2014 11:14 PM, Stefan Beller wrote:
> From: Ronnie Sahlberg <sahlberg@google.com>
> 
> Update copy_fd to return a meaningful errno on failure and also 
> preserve the existing errno variable.
> 
> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> As announced in [1], I'm going to take over the 
> ref-transactions-reflog series by Ronnie Sahlberg.
> 
> This patch was sent previously to the list as part of
> that series[2], but it seems to be unrelated to me.
> 
> Thanks,
> Stefan
> 
> [1] http://www.mail-archive.com/git@vger.kernel.org/msg61051.html
> [2] http://www.spinics.net/lists/git/msg240784.html
>  copy.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/copy.c b/copy.c
> index f2970ec..a8d366e 100644
> --- a/copy.c
> +++ b/copy.c
> @@ -8,12 +8,17 @@ int copy_fd(int ifd, int ofd)
>  		if (!len)
>  			break;
>  		if (len < 0) {
> -			return error("copy-fd: read returned %s",
> -				     strerror(errno));
> +			int save_errno = errno;
> +			error("copy-fd: read returned %s", strerror(errno));
> +			errno = save_errno;
> +			return -1;
> +		}
> +		if (write_in_full(ofd, buffer, len) < 0) {
> +			int save_errno = errno;
> +			error("copy-fd: write returned %s", strerror(errno));
> +			errno = save_errno;
> +			return -1;
>  		}
> -		if (write_in_full(ofd, buffer, len) < 0)
> -			return error("copy-fd: write returned %s",
> -				     strerror(errno));
>  	}
>  	return 0;
>  }
> 

Couldn't we save ourselves a lot of this "save_errno" boilerplate by
making error() and warning() preserve errno? They don't do any
meaningful internal error checking anyway, so even if they overwrite
errno, that value is useless to callers (who undoubtedly wouldn't check
such a value anyway). Something like

 int error(const char *err, ...)
 {
 	va_list params;
+	int save_errno = errno;

 	va_start(params, err);
 	error_routine(err, params);
 	va_end(params);
+	errno = save_errno;
 	return -1;
 }

and the same for warning()?

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu