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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 26, 2014, 17:10 UTC
Message-ID: <xmqqd2bnjhx1.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1409066605-4851-6-git-send-email-prohaska@zib.de>

Steffen Prohaska <prohaska@zib.de> writes:

> The caller opened the fd, so it should be responsible for closing it.

Hmmmm, I am not sure what the benefit of such a dogmatism. This function consumes all that is useful in the fd, and there is nothing interesting that can be do further on it, no?

Ah, wait. The caller could choose to rewind and reuse the contents, and it is very selfish of this function to unilaterally declare that once you give an fd to it you cannot do anything with it laster.

So I think this is a good change, but the justification is not quite. It is not "the caller should be responsible"; it is more about "the callee did not open it, does not own it, and should allow the caller, if it chooses, reuse it by seeking after the callee is done."

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

Let's follow the "<area>: <description>" convention here, too, e.g.

    copy_fd(): allow callers to work on input fd after it returns

or something.

Thanks.

Show 58 quoted lines

>  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));
>  		}
> @@ -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));
>  			}
>  		}
>  	}
> -	close(ifd);
>  	return 0;
>  }
>  
> @@ -60,6 +56,7 @@ int copy_file(const char *dst, const char *src, int mode)
>  		return fdo;
>  	}
>  	status = copy_fd(fdi, fdo);
> +	close(fdi);
>  	if (close(fdo) != 0)
>  		return error("%s: close error: %s", dst, strerror(errno));
>  
> diff --git a/lockfile.c b/lockfile.c
> index 2564a7f..2448d30 100644
> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -224,8 +224,11 @@ int hold_lock_file_for_append(struct lock_file *lk, const char *path, int flags)
>  	} else if (copy_fd(orig_fd, fd)) {
>  		if (flags & LOCK_DIE_ON_ERROR)
>  			exit(128);
> +		close(orig_fd);
>  		close(fd);
>  		return -1;
> +	} else {
> +		close(orig_fd);
>  	}
>  	return fd;
>  }