On Aug 26, 2014, at 8:29 PM, Jeff King <peff@xxxxxxxx> wrote:
> 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@xxxxxx>
>> ---
>> 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:
Makes all sense, and seems sane to me, too.
Junio, I saw that you have the changes on pu with 'SQUASH???...'. Will you
squash it, or shall I send another complete update of the patch series?
Steffen
> ---
> 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;
> }
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html