{"thread":{"id":"35680","subject":"[PATCH 1/2] prefer xwrite instead of write","startedAt":"2014-01-17T14:17:09Z","lastAt":"2014-01-17T20:07:15Z","messageCount":7,"participants":["Erik Faye-Lund","Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"233288","messageId":"1389968230-1224-1-git-send-email-kusmabite@gmail.com","threadId":"35680","inReplyTo":null,"subject":"[PATCH 1/2] prefer xwrite instead of write","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2014-01-17T14:17:09Z","receivedAt":"2014-01-17T14:17:09Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"Our xwrite wrapper already deals with a few potential hazards, and\nare as such more robust. Prefer it instead of write to get the\nrobustness benefits everywhere.\n\nSigned-off-by: Erik Faye-Lund <kusmabite@gmail.com>\n---\nWith this patch, we don't call write directly any more; all calls\ngoes via the xwrite-wrapper.\n\n builtin/merge.c    | 2 +-\n streaming.c        | 2 +-\n transport-helper.c | 5 ++---\n 3 files changed, 4 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 4941a6c..9383c31 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -367,7 +367,7 @@ static void squash_message(struct commit *commit, struct commit_list *remotehead\n \t\t\tsha1_to_hex(commit->object.sha1));\n \t\tpretty_print_commit(&ctx, commit, &out);\n \t}\n-\tif (write(fd, out.buf, out.len) < 0)\n+\tif (xwrite(fd, out.buf, out.len) < 0)\n \t\tdie_errno(_(\"Writing SQUASH_MSG\"));\n \tif (close(fd))\n \t\tdie_errno(_(\"Finishing SQUASH_MSG\"));\ndiff --git a/streaming.c b/streaming.c\nindex 9659f18..d7c9f32 100644\n--- a/streaming.c\n+++ b/streaming.c\n@@ -538,7 +538,7 @@ int stream_blob_to_fd(int fd, unsigned const char *sha1, struct stream_filter *f\n \t\t\tgoto close_and_exit;\n \t}\n \tif (kept && (lseek(fd, kept - 1, SEEK_CUR) == (off_t) -1 ||\n-\t\t     write(fd, \"\", 1) != 1))\n+\t\t     xwrite(fd, \"\", 1) != 1))\n \t\tgoto close_and_exit;\n \tresult = 0;\n \ndiff --git a/transport-helper.c b/transport-helper.c\nindex 2010674..bf64ad7 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -1129,9 +1129,8 @@ static int udt_do_write(struct unidirectional_transfer *t)\n \t\treturn 0;\t/* Nothing to write. */\n \n \ttransfer_debug(\"%s is writable\", t->dest_name);\n-\tbytes = write(t->dest, t->buf, t->bufuse);\n-\tif (bytes < 0 && errno != EWOULDBLOCK && errno != EAGAIN &&\n-\t\terrno != EINTR) {\n+\tbytes = xwrite(t->dest, t->buf, t->bufuse);\n+\tif (bytes < 0 && errno != EWOULDBLOCK) {\n \t\terror(\"write(%s) failed: %s\", t->dest_name, strerror(errno));\n \t\treturn -1;\n \t} else if (bytes > 0) {\n-- \n1.8.4.msysgit.0\n\n-- \n-- \n*** Please reply-to-all at all times ***\n*** (do not pretend to know who is subscribed and who is not) ***\n*** Please avoid top-posting. ***\nThe msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.\n\nYou received this message because you are subscribed to the Google\nGroups \"msysGit\" group.\nTo post to this group, send email to msysgit@googlegroups.com\nTo unsubscribe from this group, send email to\nmsysgit+unsubscribe@googlegroups.com\nFor more options, and view previous threads, visit this group at\nhttp://groups.google.com/group/msysgit?hl=en_US?hl=en\n\n--- \nYou received this message because you are subscribed to the Google Groups \"msysGit\" group.\nTo unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.\nFor more options, visit https://groups.google.com/groups/opt_out.\n"},{"id":"233289","messageId":"1389968230-1224-2-git-send-email-kusmabite@gmail.com","threadId":"35680","inReplyTo":"1389968230-1224-1-git-send-email-kusmabite@gmail.com","subject":"[PATCH 2/2] mingw: remove mingw_write","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2014-01-17T14:17:10Z","receivedAt":"2014-01-17T14:17:10Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"Since 0b6806b9 (\"xread, xwrite: limit size of IO to 8MB\"), this\nwrapper is no longer needed, as read and write are already split\ninto small chunks.\n\nSigned-off-by: Erik Faye-Lund <kusmabite@gmail.com>\n---\n compat/mingw.c | 17 -----------------\n compat/mingw.h |  3 ---\n 2 files changed, 20 deletions(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex fecb98b..e9892f8 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -304,23 +304,6 @@ int mingw_open (const char *filename, int oflags, ...)\n \treturn fd;\n }\n \n-#undef write\n-ssize_t mingw_write(int fd, const void *buf, size_t count)\n-{\n-\t/*\n-\t * While write() calls to a file on a local disk are translated\n-\t * into WriteFile() calls with a maximum size of 64KB on Windows\n-\t * XP and 256KB on Vista, no such cap is placed on writes to\n-\t * files over the network on Windows XP.  Unfortunately, there\n-\t * seems to be a limit of 32MB-28KB on X64 and 64MB-32KB on x86;\n-\t * bigger writes fail on Windows XP.\n-\t * So we cap to a nice 31MB here to avoid write failures over\n-\t * the net without changing the number of WriteFile() calls in\n-\t * the local case.\n-\t */\n-\treturn write(fd, buf, min(count, 31 * 1024 * 1024));\n-}\n-\n static BOOL WINAPI ctrl_ignore(DWORD type)\n {\n \treturn TRUE;\ndiff --git a/compat/mingw.h b/compat/mingw.h\nindex 92cd728..e033e72 100644\n--- a/compat/mingw.h\n+++ b/compat/mingw.h\n@@ -180,9 +180,6 @@ int mingw_rmdir(const char *path);\n int mingw_open (const char *filename, int oflags, ...);\n #define open mingw_open\n \n-ssize_t mingw_write(int fd, const void *buf, size_t count);\n-#define write mingw_write\n-\n int mingw_fgetc(FILE *stream);\n #define fgetc mingw_fgetc\n \n-- \n1.8.4.msysgit.0\n\n-- \n-- \n*** Please reply-to-all at all times ***\n*** (do not pretend to know who is subscribed and who is not) ***\n*** Please avoid top-posting. ***\nThe msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.\n\nYou received this message because you are subscribed to the Google\nGroups \"msysGit\" group.\nTo post to this group, send email to msysgit@googlegroups.com\nTo unsubscribe from this group, send email to\nmsysgit+unsubscribe@googlegroups.com\nFor more options, and view previous threads, visit this group at\nhttp://groups.google.com/group/msysgit?hl=en_US?hl=en\n\n--- \nYou received this message because you are subscribed to the Google Groups \"msysGit\" group.\nTo unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.\nFor more options, visit https://groups.google.com/groups/opt_out.\n"},{"id":"233296","messageId":"20140117182148.GY18964@google.com","threadId":"35680","inReplyTo":"1389968230-1224-1-git-send-email-kusmabite@gmail.com","subject":"Re: [PATCH 1/2] prefer xwrite instead of write","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-01-17T18:21:48Z","receivedAt":"2014-01-17T18:21:48Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nErik Faye-Lund wrote:\n\n> --- a/builtin/merge.c\n> +++ b/builtin/merge.c\n> @@ -367,7 +367,7 @@ static void squash_message(struct commit *commit, struct commit_list *remotehead\n>  \t\t\tsha1_to_hex(commit->object.sha1));\n>  \t\tpretty_print_commit(&ctx, commit, &out);\n>  \t}\n> -\tif (write(fd, out.buf, out.len) < 0)\n> +\tif (xwrite(fd, out.buf, out.len) < 0)\n>  \t\tdie_errno(_(\"Writing SQUASH_MSG\"));\n\nShouldn't this use write_in_full() to avoid a silently truncated result? (*)\n\n[...]\n> --- a/streaming.c\n> +++ b/streaming.c\n> @@ -538,7 +538,7 @@ int stream_blob_to_fd(int fd, unsigned const char *sha1, struct stream_filter *f\n>  \t\t\tgoto close_and_exit;\n>  \t}\n>  \tif (kept && (lseek(fd, kept - 1, SEEK_CUR) == (off_t) -1 ||\n> -\t\t     write(fd, \"\", 1) != 1))\n> +\t\t     xwrite(fd, \"\", 1) != 1))\n\nYeah, if we get EINTR then it's worth retrying.\n\n[...]\n> --- a/transport-helper.c\n> +++ b/transport-helper.c\n> @@ -1129,9 +1129,8 @@ static int udt_do_write(struct unidirectional_transfer *t)\n>  \t\treturn 0;\t/* Nothing to write. */\n>  \n>  \ttransfer_debug(\"%s is writable\", t->dest_name);\n> -\tbytes = write(t->dest, t->buf, t->bufuse);\n> -\tif (bytes < 0 && errno != EWOULDBLOCK && errno != EAGAIN &&\n> -\t\terrno != EINTR) {\n> +\tbytes = xwrite(t->dest, t->buf, t->bufuse);\n> +\tif (bytes < 0 && errno != EWOULDBLOCK) {\n\nHere the write is limited by BUFFERSIZE, and returning to the outer\nloop to try another read when the write returns EAGAIN, like the\noriginal code does, seems philosophically like the right thing to do.\n\nLuckily we don't use O_NONBLOCK anywhere, so the change shouldn't\nmatter in practice.  So although it doesn't do any good, using xwrite\nhere for consistency should be fine.\n\nSo my only worry is the (*) above.  With that change,\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\n-- \n-- \n*** Please reply-to-all at all times ***\n*** (do not pretend to know who is subscribed and who is not) ***\n*** Please avoid top-posting. ***\nThe msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.\n\nYou received this message because you are subscribed to the Google\nGroups \"msysGit\" group.\nTo post to this group, send email to msysgit@googlegroups.com\nTo unsubscribe from this group, send email to\nmsysgit+unsubscribe@googlegroups.com\nFor more options, and view previous threads, visit this group at\nhttp://groups.google.com/group/msysgit?hl=en_US?hl=en\n\n--- \nYou received this message because you are subscribed to the Google Groups \"msysGit\" group.\nTo unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.\nFor more options, visit https://groups.google.com/groups/opt_out.\n"},{"id":"233300","messageId":"xmqq4n52xw95.fsf@gitster.dls.corp.google.com","threadId":"35680","inReplyTo":"20140117182148.GY18964@google.com","subject":"Re: [PATCH 1/2] prefer xwrite instead of write","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-17T19:07:34Z","receivedAt":"2014-01-17T19:07:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> Hi,\n>\n> Erik Faye-Lund wrote:\n>\n>> --- a/builtin/merge.c\n>> +++ b/builtin/merge.c\n>> @@ -367,7 +367,7 @@ static void squash_message(struct commit *commit, struct commit_list *remotehead\n>>  \t\t\tsha1_to_hex(commit->object.sha1));\n>>  \t\tpretty_print_commit(&ctx, commit, &out);\n>>  \t}\n>> -\tif (write(fd, out.buf, out.len) < 0)\n>> +\tif (xwrite(fd, out.buf, out.len) < 0)\n>>  \t\tdie_errno(_(\"Writing SQUASH_MSG\"));\n>\n> Shouldn't this use write_in_full() to avoid a silently truncated result? (*)\n\nMeaning this?  If so, I think it makes sense.\n\n builtin/merge.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 6e108d2..a6a38ee 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -367,7 +367,7 @@ static void squash_message(struct commit *commit, struct commit_list *remotehead\n \t\t\tsha1_to_hex(commit->object.sha1));\n \t\tpretty_print_commit(&ctx, commit, &out);\n \t}\n-\tif (xwrite(fd, out.buf, out.len) < 0)\n+\tif (write_in_full(fd, out.buf, out.len) != out.len)\n \t\tdie_errno(_(\"Writing SQUASH_MSG\"));\n \tif (close(fd))\n \t\tdie_errno(_(\"Finishing SQUASH_MSG\"));\n\n\n\n>\n> [...]\n>> --- a/streaming.c\n>> +++ b/streaming.c\n>> @@ -538,7 +538,7 @@ int stream_blob_to_fd(int fd, unsigned const char *sha1, struct stream_filter *f\n>>  \t\t\tgoto close_and_exit;\n>>  \t}\n>>  \tif (kept && (lseek(fd, kept - 1, SEEK_CUR) == (off_t) -1 ||\n>> -\t\t     write(fd, \"\", 1) != 1))\n>> +\t\t     xwrite(fd, \"\", 1) != 1))\n>\n> Yeah, if we get EINTR then it's worth retrying.\n>\n> [...]\n>> --- a/transport-helper.c\n>> +++ b/transport-helper.c\n>> @@ -1129,9 +1129,8 @@ static int udt_do_write(struct unidirectional_transfer *t)\n>>  \t\treturn 0;\t/* Nothing to write. */\n>>  \n>>  \ttransfer_debug(\"%s is writable\", t->dest_name);\n>> -\tbytes = write(t->dest, t->buf, t->bufuse);\n>> -\tif (bytes < 0 && errno != EWOULDBLOCK && errno != EAGAIN &&\n>> -\t\terrno != EINTR) {\n>> +\tbytes = xwrite(t->dest, t->buf, t->bufuse);\n>> +\tif (bytes < 0 && errno != EWOULDBLOCK) {\n>\n> Here the write is limited by BUFFERSIZE, and returning to the outer\n> loop to try another read when the write returns EAGAIN, like the\n> original code does, seems philosophically like the right thing to do.\n>\n> Luckily we don't use O_NONBLOCK anywhere, so the change shouldn't\n> matter in practice.  So although it doesn't do any good, using xwrite\n> here for consistency should be fine.\n>\n> So my only worry is the (*) above.  With that change,\n> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n>\n> -- \n\n-- \n-- \n*** Please reply-to-all at all times ***\n*** (do not pretend to know who is subscribed and who is not) ***\n*** Please avoid top-posting. ***\nThe msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.\n\nYou received this message because you are subscribed to the Google\nGroups \"msysGit\" group.\nTo post to this group, send email to msysgit@googlegroups.com\nTo unsubscribe from this group, send email to\nmsysgit+unsubscribe@googlegroups.com\nFor more options, and view previous threads, visit this group at\nhttp://groups.google.com/group/msysgit?hl=en_US?hl=en\n\n--- \nYou received this message because you are subscribed to the Google Groups \"msysGit\" group.\nTo unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.\nFor more options, visit https://groups.google.com/groups/opt_out.\n"},{"id":"233301","messageId":"CABPQNSYQhctwvJgW4N-=56Oiiop6MyyYNyLKQF6WYps7dpC5Ug@mail.gmail.com","threadId":"35680","inReplyTo":"xmqq4n52xw95.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/2] prefer xwrite instead of write","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2014-01-17T19:08:51Z","receivedAt":"2014-01-17T19:08:51Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Fri, Jan 17, 2014 at 8:07 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Jonathan Nieder <jrnieder@gmail.com> writes:\n>\n>> Hi,\n>>\n>> Erik Faye-Lund wrote:\n>>\n>>> --- a/builtin/merge.c\n>>> +++ b/builtin/merge.c\n>>> @@ -367,7 +367,7 @@ static void squash_message(struct commit *commit, struct commit_list *remotehead\n>>>                      sha1_to_hex(commit->object.sha1));\n>>>              pretty_print_commit(&ctx, commit, &out);\n>>>      }\n>>> -    if (write(fd, out.buf, out.len) < 0)\n>>> +    if (xwrite(fd, out.buf, out.len) < 0)\n>>>              die_errno(_(\"Writing SQUASH_MSG\"));\n>>\n>> Shouldn't this use write_in_full() to avoid a silently truncated result? (*)\n>\n> Meaning this?  If so, I think it makes sense.\n>\n\nYeah, I think that's better. Thanks, both!\n\n-- \n-- \n*** Please reply-to-all at all times ***\n*** (do not pretend to know who is subscribed and who is not) ***\n*** Please avoid top-posting. ***\nThe msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.\n\nYou received this message because you are subscribed to the Google\nGroups \"msysGit\" group.\nTo post to this group, send email to msysgit@googlegroups.com\nTo unsubscribe from this group, send email to\nmsysgit+unsubscribe@googlegroups.com\nFor more options, and view previous threads, visit this group at\nhttp://groups.google.com/group/msysgit?hl=en_US?hl=en\n\n--- \nYou received this message because you are subscribed to the Google Groups \"msysGit\" group.\nTo unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.\nFor more options, visit https://groups.google.com/groups/opt_out.\n"},{"id":"233312","messageId":"20140117200236.GA18964@google.com","threadId":"35680","inReplyTo":"xmqq4n52xw95.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/2] prefer xwrite instead of write","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-01-17T20:02:36Z","receivedAt":"2014-01-17T20:02:36Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n> Jonathan Nieder <jrnieder@gmail.com> writes:\n\n>> Shouldn't this use write_in_full() to avoid a silently truncated result? (*)\n>\n> Meaning this?  If so, I think it makes sense.\n[...]\n> -\tif (xwrite(fd, out.buf, out.len) < 0)\n> +\tif (write_in_full(fd, out.buf, out.len) != out.len)\n\nYes.  Either '< 0' or '!= out.len' would work fine here, since\nwrite_in_full is defined to always either write the full 'count'\nbytes or return an error.\n\nThanks,\nJonathan\n\n-- \n-- \n*** Please reply-to-all at all times ***\n*** (do not pretend to know who is subscribed and who is not) ***\n*** Please avoid top-posting. ***\nThe msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.\n\nYou received this message because you are subscribed to the Google\nGroups \"msysGit\" group.\nTo post to this group, send email to msysgit@googlegroups.com\nTo unsubscribe from this group, send email to\nmsysgit+unsubscribe@googlegroups.com\nFor more options, and view previous threads, visit this group at\nhttp://groups.google.com/group/msysgit?hl=en_US?hl=en\n\n--- \nYou received this message because you are subscribed to the Google Groups \"msysGit\" group.\nTo unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.\nFor more options, visit https://groups.google.com/groups/opt_out.\n"},{"id":"233314","messageId":"xmqqa9euwex8.fsf@gitster.dls.corp.google.com","threadId":"35680","inReplyTo":"20140117200236.GA18964@google.com","subject":"Re: [PATCH 1/2] prefer xwrite instead of write","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-17T20:07:15Z","receivedAt":"2014-01-17T20:07:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> Junio C Hamano wrote:\n>> Jonathan Nieder <jrnieder@gmail.com> writes:\n>\n>>> Shouldn't this use write_in_full() to avoid a silently truncated result? (*)\n>>\n>> Meaning this?  If so, I think it makes sense.\n> [...]\n>> -\tif (xwrite(fd, out.buf, out.len) < 0)\n>> +\tif (write_in_full(fd, out.buf, out.len) != out.len)\n>\n> Yes.  Either '< 0' or '!= out.len' would work fine here, since\n> write_in_full is defined to always either write the full 'count'\n> bytes or return an error.\n\nAn unrelated tangent but we may want to fix majority of callers that\ndo not seem to know that ;-)\n\n-- \n-- \n*** Please reply-to-all at all times ***\n*** (do not pretend to know who is subscribed and who is not) ***\n*** Please avoid top-posting. ***\nThe msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.\n\nYou received this message because you are subscribed to the Google\nGroups \"msysGit\" group.\nTo post to this group, send email to msysgit@googlegroups.com\nTo unsubscribe from this group, send email to\nmsysgit+unsubscribe@googlegroups.com\nFor more options, and view previous threads, visit this group at\nhttp://groups.google.com/group/msysgit?hl=en_US?hl=en\n\n--- \nYou received this message because you are subscribed to the Google Groups \"msysGit\" group.\nTo unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.\nFor more options, visit https://groups.google.com/groups/opt_out.\n"}]}