Re: [PATCH 1/2] prefer xwrite instead of write
Junio C Hamano wrote: > Jonathan Nieder writes: >> Shouldn't this use write_in_full() to avoid a silently truncated result? (*) > > Meaning this? If so, I think it makes sense. [...] > - if (xwrite(fd, out.buf, out.len) < 0) > + if (write_in_full(fd, out.buf, out.len) != out.len) Yes. Either '< 0' or '!= out.len' would work fine here, since write_in_full is defined to always either write the full 'count' bytes or return an error. Thanks, Jonathan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] prefer xwrite instead of write
Jonathan Nieder writes: > Junio C Hamano wrote: >> Jonathan Nieder writes: > >>> Shouldn't this use write_in_full() to avoid a silently truncated result? (*) >> >> Meaning this? If so, I think it makes sense. > [...] >> -if (xwrite(fd, out.buf, out.len) < 0) >> +if (write_in_full(fd, out.buf, out.len) != out.len) > > Yes. Either '< 0' or '!= out.len' would work fine here, since > write_in_full is defined to always either write the full 'count' > bytes or return an error. An unrelated tangent but we may want to fix majority of callers that do not seem to know that ;-) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] prefer xwrite instead of write
On Fri, Jan 17, 2014 at 8:07 PM, Junio C Hamano wrote:
> Jonathan Nieder writes:
>
>> Hi,
>>
>> Erik Faye-Lund wrote:
>>
>>> --- a/builtin/merge.c
>>> +++ b/builtin/merge.c
>>> @@ -367,7 +367,7 @@ static void squash_message(struct commit *commit,
>>> struct commit_list *remotehead
>>> sha1_to_hex(commit->object.sha1));
>>> pretty_print_commit(&ctx, commit, &out);
>>> }
>>> -if (write(fd, out.buf, out.len) < 0)
>>> +if (xwrite(fd, out.buf, out.len) < 0)
>>> die_errno(_("Writing SQUASH_MSG"));
>>
>> Shouldn't this use write_in_full() to avoid a silently truncated result? (*)
>
> Meaning this? If so, I think it makes sense.
>
Yeah, I think that's better. Thanks, both!
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] prefer xwrite instead of write
Jonathan Nieder writes:
> Hi,
>
> Erik Faye-Lund wrote:
>
>> --- a/builtin/merge.c
>> +++ b/builtin/merge.c
>> @@ -367,7 +367,7 @@ static void squash_message(struct commit *commit, struct
>> commit_list *remotehead
>> sha1_to_hex(commit->object.sha1));
>> pretty_print_commit(&ctx, commit, &out);
>> }
>> -if (write(fd, out.buf, out.len) < 0)
>> +if (xwrite(fd, out.buf, out.len) < 0)
>> die_errno(_("Writing SQUASH_MSG"));
>
> Shouldn't this use write_in_full() to avoid a silently truncated result? (*)
Meaning this? If so, I think it makes sense.
builtin/merge.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/merge.c b/builtin/merge.c
index 6e108d2..a6a38ee 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -367,7 +367,7 @@ static void squash_message(struct commit *commit, struct
commit_list *remotehead
sha1_to_hex(commit->object.sha1));
pretty_print_commit(&ctx, commit, &out);
}
- if (xwrite(fd, out.buf, out.len) < 0)
+ if (write_in_full(fd, out.buf, out.len) != out.len)
die_errno(_("Writing SQUASH_MSG"));
if (close(fd))
die_errno(_("Finishing SQUASH_MSG"));
>
> [...]
>> --- a/streaming.c
>> +++ b/streaming.c
>> @@ -538,7 +538,7 @@ int stream_blob_to_fd(int fd, unsigned const char *sha1,
>> struct stream_filter *f
>> goto close_and_exit;
>> }
>> if (kept && (lseek(fd, kept - 1, SEEK_CUR) == (off_t) -1 ||
>> - write(fd, "", 1) != 1))
>> + xwrite(fd, "", 1) != 1))
>
> Yeah, if we get EINTR then it's worth retrying.
>
> [...]
>> --- a/transport-helper.c
>> +++ b/transport-helper.c
>> @@ -1129,9 +1129,8 @@ static int udt_do_write(struct unidirectional_transfer
>> *t)
>> return 0; /* Nothing to write. */
>>
>> transfer_debug("%s is writable", t->dest_name);
>> -bytes = write(t->dest, t->buf, t->bufuse);
>> -if (bytes < 0 && errno != EWOULDBLOCK && errno != EAGAIN &&
>> -errno != EINTR) {
>> +bytes = xwrite(t->dest, t->buf, t->bufuse);
>> +if (bytes < 0 && errno != EWOULDBLOCK) {
>
> Here the write is limited by BUFFERSIZE, and returning to the outer
> loop to try another read when the write returns EAGAIN, like the
> original code does, seems philosophically like the right thing to do.
>
> Luckily we don't use O_NONBLOCK anywhere, so the change shouldn't
> matter in practice. So although it doesn't do any good, using xwrite
> here for consistency should be fine.
>
> So my only worry is the (*) above. With that change,
> Reviewed-by: Jonathan Nieder
>
> --
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] prefer xwrite instead of write
Hi,
Erik Faye-Lund wrote:
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -367,7 +367,7 @@ static void squash_message(struct commit *commit, struct
> commit_list *remotehead
> sha1_to_hex(commit->object.sha1));
> pretty_print_commit(&ctx, commit, &out);
> }
> - if (write(fd, out.buf, out.len) < 0)
> + if (xwrite(fd, out.buf, out.len) < 0)
> die_errno(_("Writing SQUASH_MSG"));
Shouldn't this use write_in_full() to avoid a silently truncated result? (*)
[...]
> --- a/streaming.c
> +++ b/streaming.c
> @@ -538,7 +538,7 @@ int stream_blob_to_fd(int fd, unsigned const char *sha1,
> struct stream_filter *f
> goto close_and_exit;
> }
> if (kept && (lseek(fd, kept - 1, SEEK_CUR) == (off_t) -1 ||
> - write(fd, "", 1) != 1))
> + xwrite(fd, "", 1) != 1))
Yeah, if we get EINTR then it's worth retrying.
[...]
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -1129,9 +1129,8 @@ static int udt_do_write(struct unidirectional_transfer
> *t)
> return 0; /* Nothing to write. */
>
> transfer_debug("%s is writable", t->dest_name);
> - bytes = write(t->dest, t->buf, t->bufuse);
> - if (bytes < 0 && errno != EWOULDBLOCK && errno != EAGAIN &&
> - errno != EINTR) {
> + bytes = xwrite(t->dest, t->buf, t->bufuse);
> + if (bytes < 0 && errno != EWOULDBLOCK) {
Here the write is limited by BUFFERSIZE, and returning to the outer
loop to try another read when the write returns EAGAIN, like the
original code does, seems philosophically like the right thing to do.
Luckily we don't use O_NONBLOCK anywhere, so the change shouldn't
matter in practice. So although it doesn't do any good, using xwrite
here for consistency should be fine.
So my only worry is the (*) above. With that change,
Reviewed-by: Jonathan Nieder
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
