{"thread":{"id":"8747","subject":"[PATCH] detect dup failure","startedAt":"2007-06-27T11:59:47Z","lastAt":"2007-06-28T05:02:25Z","messageCount":15,"participants":["Jim Meyering","Alex Riesen","Geert Bosch","Linus Torvalds","Josh Triplett","Shawn O. Pearce"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"45904","messageId":"87wsxpobf0.fsf@rho.meyering.net","threadId":"8747","inReplyTo":null,"subject":"[PATCH] detect dup failure","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2007-06-27T11:59:47Z","receivedAt":"2007-06-27T11:59:47Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Without this, if you ever run out of file descriptors, dup will\nfail (silently), fdopen will return NULL, and fprintf will\ntry to dereference NULL (i.e., usually segfault).\n\nSigned-off-by: Jim Meyering \n---\n builtin-log.c | 8 ++++++--\n 1 files changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin-log.c b/builtin-log.c\nindex 073a2a1..ca54387 100644\n--- a/builtin-log.c\n+++ b/builtin-log.c\n@@ -588,8 +588,12 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \tif (ignore_if_in_upstream)\n \t\tget_patch_ids(&rev, &ids, prefix);\n\n-\tif (!use_stdout)\n-\t\trealstdout = fdopen(dup(1), \"w\");\n+\tif (!use_stdout) {\n+\t\tint fd = dup(1);\n+\t\tif (fd < 0)\n+\t\t\tdie(\"failed to duplicate standard output\");\n+\t\trealstdout = fdopen(fd, \"w\");\n+\t}\n\n \tprepare_revision_walk(&rev);\n \twhile ((commit = get_revision(&rev)) != NULL) {\n"},{"id":"45908","messageId":"81b0412b0706270548p6f694fd6x5f47cbefa16c08ac@mail.gmail.com","threadId":"8747","inReplyTo":"87wsxpobf0.fsf@rho.meyering.net","subject":"Re: [PATCH] detect dup failure","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-06-27T12:48:52Z","receivedAt":"2007-06-27T12:48:52Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On 6/27/07, Jim Meyering wrote:\n> Without this, if you ever run out of file descriptors, dup will\n> fail (silently), fdopen will return NULL, and fprintf will\n> try to dereference NULL (i.e., usually segfault).\n\nBut if you check the result of fdopen for NULL instead\nyou'll cover the dup failure _and_ out-of-memory in one\ngo. You'll loose the errno (probably), but you don't seem\nto use it here anyway.\n"},{"id":"45909","messageId":"87r6nxo8iq.fsf_-_@rho.meyering.net","threadId":"8747","inReplyTo":"81b0412b0706270548p6f694fd6x5f47cbefa16c08ac@mail.gmail.com","subject":"[PATCH] git-log: detect dup and fdopen failure","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2007-06-27T13:02:21Z","receivedAt":"2007-06-27T13:02:21Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"\"Alex Riesen\" wrote:\n> On 6/27/07, Jim Meyering wrote:\n>> Without this, if you ever run out of file descriptors, dup will\n>> fail (silently), fdopen will return NULL, and fprintf will\n>> try to dereference NULL (i.e., usually segfault).\n>\n> But if you check the result of fdopen for NULL instead\n> you'll cover the dup failure _and_ out-of-memory in one\n> go. You'll loose the errno (probably), but you don't seem\n> to use it here anyway.\n\nGood catch. Thanks!\nI didn't see that fdopen could fail with ENOMEM.\nThat'll teach me to trust the man page. I see POSIX does mention it.\n\nHere's a better patch:\n\nSigned-off-by: Jim Meyering \n---\n builtin-log.c | 8 ++++++--\n 1 files changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin-log.c b/builtin-log.c\nindex 073a2a1..7b0d6f4 100644\n--- a/builtin-log.c\n+++ b/builtin-log.c\n@@ -588,8 +588,12 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \tif (ignore_if_in_upstream)\n \t\tget_patch_ids(&rev, &ids, prefix);\n\n-\tif (!use_stdout)\n-\t\trealstdout = fdopen(dup(1), \"w\");\n+\tif (!use_stdout) {\n+\t\tint fd = dup(1);\n+\t\tif (fd < 0 || (realstdout = fdopen(fd, \"w\")) == NULL)\n+\t\t\tdie(\"failed to duplicate standard output: %s\",\n+\t\t\t strerror(errno));\n+\t}\n\n \tprepare_revision_walk(&rev);\n \twhile ((commit = get_revision(&rev)) != NULL) {\n"},{"id":"45911","messageId":"81b0412b0706270618p850df2dy41b2b797d82e3276@mail.gmail.com","threadId":"8747","inReplyTo":"87r6nxo8iq.fsf_-_@rho.meyering.net","subject":"Re: [PATCH] git-log: detect dup and fdopen failure","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-06-27T13:18:47Z","receivedAt":"2007-06-27T13:18:47Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On 6/27/07, Jim Meyering wrote:\n> I didn't see that fdopen could fail with ENOMEM.\n> That'll teach me to trust the man page. I see POSIX does mention it.\n\nI wouldn't trust Linux man pages nor POSIX, if I were you.\nCheck if this works in some exotic but common\nenvironments (like MacOSX, Cygwin or HP-UX).\n\n(And yes, they probably are broken, and no, you can't fix them,\nand no, people are not going to stop using them).\n\n> + if (!use_stdout) {\n> + int fd = dup(1);\n> + if (fd < 0 || (realstdout = fdopen(fd, \"w\")) == NULL)\n> + die(\"failed to duplicate standard output: %s\",\n> + strerror(errno));\n> + }\n\nKinda stuffed in here. What's wrong with plain\n\n realstdout = fdopen(dup(1), \"w\");\n if (!realstdout)\n die(\"%s\", strerror(errno));\n\n(Yes, I do think that \"duplicate standard output\" is useless,\nexcept for debugging. Exactly as strerror is, but that is shorter).\n"},{"id":"45913","messageId":"87d4zho74h.fsf@rho.meyering.net","threadId":"8747","inReplyTo":"81b0412b0706270618p850df2dy41b2b797d82e3276@mail.gmail.com","subject":"Re: [PATCH] git-log: detect dup and fdopen failure","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2007-06-27T13:32:30Z","receivedAt":"2007-06-27T13:32:30Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"\"Alex Riesen\" wrote:\n\n> On 6/27/07, Jim Meyering wrote:\n>> I didn't see that fdopen could fail with ENOMEM.\n>> That'll teach me to trust the man page. I see POSIX does mention it.\n\ns/trust/use/ :)\n\n> I wouldn't trust Linux man pages nor POSIX, if I were you.\n\nHa. Normally I don't rely solely on the man pages.\n\n> Check if this works in some exotic but common\n> environments (like MacOSX, Cygwin or HP-UX).\n\nWhat do you mean?\nI know that dup and fdopen work in those environments.\n\n> (And yes, they probably are broken, and no, you can't fix them,\n> and no, people are not going to stop using them).\n>\n>> + if (!use_stdout) {\n>> + int fd = dup(1);\n>> + if (fd < 0 || (realstdout = fdopen(fd, \"w\")) == NULL)\n>> + die(\"failed to duplicate standard output: %s\",\n>> + strerror(errno));\n>> + }\n>\n> Kinda stuffed in here. What's wrong with plain\n\nIt's ok, but for the fact that when dup fails, all you get\nis the uninformative EINVAL from fdopen.\n\n> realstdout = fdopen(dup(1), \"w\");\n> if (!realstdout)\n> die(\"%s\", strerror(errno));\n>\n> (Yes, I do think that \"duplicate standard output\" is useless,\n> except for debugging. Exactly as strerror is, but that is shorter).\n\nI like to include *something* in the diagnostic so that when someone\nsees it and reports it developers have an easier time finding where it\ncomes from in the code. Especially with something as unlikely (and hard\nto reproduce) as this, that might be useful.\n"},{"id":"45915","messageId":"EF53B249-8430-4700-81AE-B97FD49FB955@adacore.com","threadId":"8747","inReplyTo":"87r6nxo8iq.fsf_-_@rho.meyering.net","subject":"Re: [PATCH] git-log: detect dup and fdopen failure","fromName":"Geert Bosch","fromEmail":"bosch@adacore.com","sentAt":"2007-06-27T13:54:57Z","receivedAt":"2007-06-27T13:54:57Z","isPatch":true,"sender":{"key":"bosch@adacore.com","avatar":null},"body":"On Jun 27, 2007, at 09:02, Jim Meyering wrote:\n> -\tif (!use_stdout)\n> -\t\trealstdout = fdopen(dup(1), \"w\");\n> +\tif (!use_stdout) {\n> +\t\tint fd = dup(1);\n> +\t\tif (fd < 0 || (realstdout = fdopen(fd, \"w\")) == NULL)\n> +\t\t\tdie(\"failed to duplicate standard output: %s\",\n> +\t\t\t strerror(errno));\n> +\t}\n\nThis makes the code unreadable! A great way to ruin\nperfectly fine code is to add tons of error checking.\nThe error checking is likely wrong (detects non-errors,\nor fails to detect real ones), and for sure makes code\nuntestable and unreadable.\n\nIf we really case about catching such errors, write\nthe code as:\n\tif (!use_stdout)\n\t\trealstdout = xfdopen(dup(1), \"w\");\nwhere xfdopen is a wrapper around fdopen that dies in\ncase of an error. This follows a practice we use elsewhere,\nand only adds one character to the code and only affects\nreadability very slightly.\n\n> Without this, if you ever run out of file descriptors, dup will\n> fail (silently), fdopen will return NULL, and fprintf will\n> try to dereference NULL (i.e., usually segfault).\n\nAs it is unlikely the failure mode will ever occur in practice,\nany way of aborting is fine. Even SIGSEGV would do: it would be\ntrivial to find that we were leaking file descriptors or are out\nof memory. Oh, wait, that means we don't need any checking code\nat all...\n\n -Geert\n"},{"id":"45917","messageId":"81b0412b0706270704y58c9e05bh6ec10118d7ee406c@mail.gmail.com","threadId":"8747","inReplyTo":"87d4zho74h.fsf@rho.meyering.net","subject":"Re: [PATCH] git-log: detect dup and fdopen failure","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-06-27T14:04:28Z","receivedAt":"2007-06-27T14:04:28Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On 6/27/07, Jim Meyering wrote:\n> \"Alex Riesen\" wrote:\n> > Check if this works in some exotic but common\n> > environments (like MacOSX, Cygwin or HP-UX).\n>\n> What do you mean?\n> I know that dup and fdopen work in those environments.\n\nExactly as you described? Are you sure fdopen on Cygwin\nsets ENOMEM?\n\n> >\n> > Kinda stuffed in here. What's wrong with plain\n>\n> It's ok, but for the fact that when dup fails, all you get\n> is the uninformative EINVAL from fdopen.\n\nEBADF on Cygwin, 0 on Mingw32. Can't even imagine what msvc\n(which actually isn't a developers tool, but very common) will return.\n\n> > (Yes, I do think that \"duplicate standard output\" is useless,\n> > except for debugging. Exactly as strerror is, but that is shorter).\n>\n> I like to include *something* in the diagnostic so that when someone\n> sees it and reports it developers have an easier time finding where it\n> comes from in the code. Especially with something as unlikely (and hard\n> to reproduce) as this, that might be useful.\n\nThen put file name and the line number in!\n"},{"id":"45918","messageId":"87zm2lmr01.fsf@rho.meyering.net","threadId":"8747","inReplyTo":"EF53B249-8430-4700-81AE-B97FD49FB955@adacore.com","subject":"Re: [PATCH] git-log: detect dup and fdopen failure","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2007-06-27T14:06:06Z","receivedAt":"2007-06-27T14:06:06Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Geert Bosch wrote:\n\n> On Jun 27, 2007, at 09:02, Jim Meyering wrote:\n>> -\tif (!use_stdout)\n>> -\t\trealstdout = fdopen(dup(1), \"w\");\n>> +\tif (!use_stdout) {\n>> +\t\tint fd = dup(1);\n>> +\t\tif (fd < 0 || (realstdout = fdopen(fd, \"w\")) == NULL)\n>> +\t\t\tdie(\"failed to duplicate standard output: %s\",\n>> +\t\t\t strerror(errno));\n>> +\t}\n>\n> This makes the code unreadable! A great way to ruin\n> perfectly fine code is to add tons of error checking.\n> The error checking is likely wrong (detects non-errors,\n> or fails to detect real ones), and for sure makes code\n> untestable and unreadable.\n>\n> If we really case about catching such errors, write\n> the code as:\n> \tif (!use_stdout)\n> \t\trealstdout = xfdopen(dup(1), \"w\");\n\nOf course. That is much more readable.\nThough, perhaps you meant this?\n\n> \t\trealstdout = xfdopen(xdup(1), \"w\");\n\nIf so, I'll be happy to prepare a patch to do that instead.\nIMHO, we should never ignore syscall errors, and when preparing a\npatch for a project like git (to which I haven't contributed much yet),\nI prefer to keep the initial patch small and localized.\n\n> where xfdopen is a wrapper around fdopen that dies in\n> case of an error. This follows a practice we use elsewhere,\n> and only adds one character to the code and only affects\n> readability very slightly.\n>\n>> Without this, if you ever run out of file descriptors, dup will\n>> fail (silently), fdopen will return NULL, and fprintf will\n>> try to dereference NULL (i.e., usually segfault).\n>\n> As it is unlikely the failure mode will ever occur in practice,\n> any way of aborting is fine. Even SIGSEGV would do: it would be\n> trivial to find that we were leaking file descriptors or are out\n> of memory. Oh, wait, that means we don't need any checking code\n> at all...\n\nAren't you presuming the problem is easily reproducible\n(and encountered by someone capable of investigating/reproducing),\nor maybe that the abort left a usable core dump behind?\n\nIn my experience, the hardest bugs to track down are those\nthat are very rare and hard to reproduce.\n"},{"id":"45919","messageId":"87tzstmqmi.fsf@rho.meyering.net","threadId":"8747","inReplyTo":"81b0412b0706270704y58c9e05bh6ec10118d7ee406c@mail.gmail.com","subject":"Re: [PATCH] git-log: detect dup and fdopen failure","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2007-06-27T14:14:13Z","receivedAt":"2007-06-27T14:14:13Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"\"Alex Riesen\" wrote:\n\n> On 6/27/07, Jim Meyering wrote:\n>> \"Alex Riesen\" wrote:\n>> > Check if this works in some exotic but common\n>> > environments (like MacOSX, Cygwin or HP-UX).\n>>\n>> What do you mean?\n>> I know that dup and fdopen work in those environments.\n>\n> Exactly as you described?\n\nNo, but why does that matter?\n\n> Are you sure fdopen on Cygwin\n> sets ENOMEM?\n\nAre you suggesting not to use errno because\nit won't contain useful information on Mingw32?\n\n>> It's ok, but for the fact that when dup fails, all you get\n>> is the uninformative EINVAL from fdopen.\n>\n> EBADF on Cygwin, 0 on Mingw32. Can't even imagine what msvc\n\nEBADF is fine. Setting errno to 0 upon error is a bug.\nDon't cater to buggy systems.\n\n> (which actually isn't a developers tool, but very common) will return.\n>\n>> > (Yes, I do think that \"duplicate standard output\" is useless,\n>> > except for debugging. Exactly as strerror is, but that is shorter).\n>>\n>> I like to include *something* in the diagnostic so that when someone\n>> sees it and reports it developers have an easier time finding where it\n>> comes from in the code. Especially with something as unlikely (and hard\n>> to reproduce) as this, that might be useful.\n>\n> Then put file name and the line number in!\n\nSurely you're playing devil's advocate, now...\nIf you go that route, you might as well use assert() for both conditions,\nand we all know *that* is silly.\n"},{"id":"45920","messageId":"87odj1mpy2.fsf@rho.meyering.net","threadId":"8747","inReplyTo":"EF53B249-8430-4700-81AE-B97FD49FB955@adacore.com","subject":"Re: [PATCH] git-log: detect dup and fdopen failure","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2007-06-27T14:28:53Z","receivedAt":"2007-06-27T14:28:53Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Geert Bosch wrote:\n> If we really case about catching such errors, write\n> the code as:\n> \tif (!use_stdout)\n> \t\trealstdout = xfdopen(dup(1), \"w\");\n\nThanks for the suggestion.\nHere's a patch doing just that (and same for dup/xdup):\n\nSubject: [PATCH] git-log: detect dup and fdopen failure\n git-compat-util.h (xdup, xfdopen): Define functions.\n\n\nSigned-off-by: Jim Meyering \n---\n builtin-log.c | 2 +-\n git-compat-util.h | 16 ++++++++++++++++\n 2 files changed, 17 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-log.c b/builtin-log.c\nindex 073a2a1..a4186ea 100644\n--- a/builtin-log.c\n+++ b/builtin-log.c\n@@ -589,7 +589,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\tget_patch_ids(&rev, &ids, prefix);\n\n \tif (!use_stdout)\n-\t\trealstdout = fdopen(dup(1), \"w\");\n+\t\trealstdout = xfdopen(xdup(1), \"w\");\n\n \tprepare_revision_walk(&rev);\n \twhile ((commit = get_revision(&rev)) != NULL) {\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex b2ab3f8..362e040 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -287,6 +287,22 @@ static inline ssize_t xwrite(int fd, const void *buf, size_t len)\n \t}\n }\n\n+static inline int xdup(int fd)\n+{\n+\tint ret = dup(fd);\n+\tif (ret < 0)\n+\t\tdie(\"dup failed: %s\", strerror(errno));\n+\treturn ret;\n+}\n+\n+static inline FILE *xfdopen(int fd, const char *mode)\n+{\n+\tFILE *stream = fdopen(fd, mode);\n+\tif (stream == NULL)\n+\t\tdie(\"Out of memory? fdopen failed: %s\", strerror(errno));\n+\treturn stream;\n+}\n+\n static inline size_t xsize_t(off_t len)\n {\n \treturn (size_t)len;\n"},{"id":"45925","messageId":"81b0412b0706270857w175f2b2ey4c30bac9c0836929@mail.gmail.com","threadId":"8747","inReplyTo":"87tzstmqmi.fsf@rho.meyering.net","subject":"Re: [PATCH] git-log: detect dup and fdopen failure","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-06-27T15:57:27Z","receivedAt":"2007-06-27T15:57:27Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On 6/27/07, Jim Meyering wrote:\n> \"Alex Riesen\" wrote:\n>\n> > On 6/27/07, Jim Meyering wrote:\n> >> \"Alex Riesen\" wrote:\n> >> > Check if this works in some exotic but common\n> >> > environments (like MacOSX, Cygwin or HP-UX).\n> >>\n> >> What do you mean?\n> >> I know that dup and fdopen work in those environments.\n> >\n> > Exactly as you described?\n>\n> No, but why does that matter?\n>\n\nYour code will need additional tweaking on this\nsystems.\n\n> > Are you sure fdopen on Cygwin\n> > sets ENOMEM?\n>\n> Are you suggesting not to use errno because\n> it won't contain useful information on Mingw32?\n\nYes. Mingw32 is just one example.\n\n> >> It's ok, but for the fact that when dup fails, all you get\n> >> is the uninformative EINVAL from fdopen.\n> >\n> > EBADF on Cygwin, 0 on Mingw32. Can't even imagine what msvc\n>\n> EBADF is fine. Setting errno to 0 upon error is a bug.\n> Don't cater to buggy systems.\n\nIt is my system at work. What should I do, throw it out of window?\n\n> >\n> > Then put file name and the line number in!\n>\n> Surely you're playing devil's advocate, now...\n\nNo, I'm just thinking aloud about whether I should start preparing\na reverting commit for your changes just to be able to continue\nusing Git on that mismanaged, stupid, slow and very real\nsystem I have to work with. It is a Win2k laptop, in a Novell\nnetwork and with Cygwin on (actually 2 cygwins at least, which\ncygwin does not support), and without a chance to _EVER_ use\nanything else. And I happen to depend on Git, because the\nalternative is, as someone noted, \"badly mismanaged\" Perforce.\nAnd I believe I am not that bad off, there are other systems on\nwhich you didn't test either. \"Silly\", as you say.\n"},{"id":"45927","messageId":"873b0dmk4l.fsf@rho.meyering.net","threadId":"8747","inReplyTo":"81b0412b0706270857w175f2b2ey4c30bac9c0836929@mail.gmail.com","subject":"Re: [PATCH] git-log: detect dup and fdopen failure","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2007-06-27T16:34:34Z","receivedAt":"2007-06-27T16:34:34Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"\"Alex Riesen\" wrote:\n> On 6/27/07, Jim Meyering wrote:\n>> \"Alex Riesen\" wrote:\n>>\n>> > On 6/27/07, Jim Meyering wrote:\n>> >> \"Alex Riesen\" wrote:\n>> >> > Check if this works in some exotic but common\n>> >> > environments (like MacOSX, Cygwin or HP-UX).\n>> >>\n>> >> What do you mean?\n>> >> I know that dup and fdopen work in those environments.\n>> >\n>> > Exactly as you described?\n>>\n>> No, but why does that matter?\n>\n> Your code will need additional tweaking on this\n> systems.\n\nWhy bother? The only undesirable side effect you'd see on a buggy\nsystem would be a meaningless strerror(errno) in that very unusual\nerror situation: when git-format-patch (without --stdout)\nruns out of file descriptors or memory.\n\n>> > Are you sure fdopen on Cygwin\n>> > sets ENOMEM?\n>>\n>> Are you suggesting not to use errno because\n>> it won't contain useful information on Mingw32?\n>\n> Yes. Mingw32 is just one example.\n>\n>> >> It's ok, but for the fact that when dup fails, all you get\n>> >> is the uninformative EINVAL from fdopen.\n>> >\n>> > EBADF on Cygwin, 0 on Mingw32. Can't even imagine what msvc\n>>\n>> EBADF is fine. Setting errno to 0 upon error is a bug.\n>> Don't cater to buggy systems.\n>\n> It is my system at work. What should I do, throw it out of window?\n\nPerhaps you misunderstood?\n\nMy saying \"don't cater to buggy systems\", means don't pollute or dumb-down\napplication code to accommodate them. Instead, code to some best-case\nstandard (Linux, POSIX, whatever) and if necessary, use wrappers that\nwork around the deficiencies of less-well-endowed systems. For example,\ncoreutils assumes that functions like fchdir and openat work properly.\nEven though some really crufty systems have no fchdir syscall, and many\nrelatively modern systems lack openat, fstatat, etc., all of the coreutils\nprograms work fine in spite of that: they rely on gnulib's emulation\nof those underlying functions. This lets applications keep free of\nthe dirty underlying portability details and concentrate on algorithms.\nThen there are the all of the wrappers for functions like stat, lstat,\nrename, etc. that misbehave on certain losing (but still in use) systems.\nI've been \"accommodating\" deficient systems for a very long time, so\nknow from experience that it can be done cleanly.\n\nIf this little corner of portability matters to you, it wouldn't be\nhard to write dup and/or fdopen wrappers that do the right thing on\nwhatever buggy system you find you have to use. Incidentally, one nice\n(essential) thing about the gnulib framework is that full-featured systems\nusually incur *absolutely no* overhead, because they don't even use the\nwrapper functions.\n\n>> > Then put file name and the line number in!\n>>\n>> Surely you're playing devil's advocate, now...\n>\n> No, I'm just thinking aloud about whether I should start preparing\n> a reverting commit for your changes just to be able to continue\n> using Git on that mismanaged, stupid, slow and very real\n\nNo need to go to such an extreme...\nUnless that particular diagnostic really matters to you.\n\n> system I have to work with. It is a Win2k laptop, in a Novell\n> network and with Cygwin on (actually 2 cygwins at least, which\n> cygwin does not support), and without a chance to _EVER_ use\n> anything else.\n\nYou have my sympathy :-/\n"},{"id":"45929","messageId":"alpine.LFD.0.98.0706270935340.8675@woody.linux-foundation.org","threadId":"8747","inReplyTo":"87odj1mpy2.fsf@rho.meyering.net","subject":"Re: [PATCH] git-log: detect dup and fdopen failure","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-06-27T16:49:39Z","receivedAt":"2007-06-27T16:49:39Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Wed, 27 Jun 2007, Jim Meyering wrote:\n> \n> Subject: [PATCH] git-log: detect dup and fdopen failure\n> git-compat-util.h (xdup, xfdopen): Define functions.\n> \n> Signed-off-by: Jim Meyering \n\nAcked-by: Linus Torvalds \n\nThat said, the whole \"prepend 'x' to the function name\" thing is obviously \nhow git does things, but maybe it would be more readable in the long term \nto make the prefix be 'safe_' instead of 'x', or something like that?\n\nThe 'x' thing is the fairly traditional thing to do for malloc(), and \nthat's where it comes from, of course. In git (and other places: google \ncode shows that other projects have ended up with the same kind of things, \nwith busybox being one example I found), it then got extended to \nxread/xwrite, but now it's getting extended so much that I'd worry a bit \nthat in the long run the easy-to-miss 'x' part really would be better off \nwritten out a bit more.\n\nNo really strong opinion, just throwing it out for comment.\n\nIt was brought on by the fact that I mentally parsed \"xfdopen()\" as \"xf\" + \n\"dopen\" for some reason (but maybe that's just me).\n\n\t\t\tLinus\n"},{"id":"45948","messageId":"4683211E.2010704@freedesktop.org","threadId":"8747","inReplyTo":"alpine.LFD.0.98.0706270935340.8675@woody.linux-foundation.org","subject":"Re: [PATCH] git-log: detect dup and fdopen failure","fromName":"Josh Triplett","fromEmail":"josh@freedesktop.org","sentAt":"2007-06-28T02:46:54Z","receivedAt":"2007-06-28T02:46:54Z","isPatch":true,"sender":{"key":"josh@joshtriplett.org","avatar":"https://avatars.githubusercontent.com/u/162737?v=4"},"body":"Linus Torvalds wrote:\n> On Wed, 27 Jun 2007, Jim Meyering wrote:\n>> Subject: [PATCH] git-log: detect dup and fdopen failure\n>> git-compat-util.h (xdup, xfdopen): Define functions.\n>>\n>> Signed-off-by: Jim Meyering \n> \n> Acked-by: Linus Torvalds \n> \n> That said, the whole \"prepend 'x' to the function name\" thing is obviously \n> how git does things, but maybe it would be more readable in the long term \n> to make the prefix be 'safe_' instead of 'x', or something like that?\n\nIf you want a more verbose name than xfoo, I personally like the foo_or_die\nconvention, which reminds you explicitly that the function might kill the\nprogram. safe_ might convey exactly the opposite impression.\n\n- Josh Triplett\n\n"},{"id":"45953","messageId":"20070628050225.GI32223@spearce.org","threadId":"8747","inReplyTo":"4683211E.2010704@freedesktop.org","subject":"Re: [PATCH] git-log: detect dup and fdopen failure","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-06-28T05:02:25Z","receivedAt":"2007-06-28T05:02:25Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Josh Triplett wrote:\n> Linus Torvalds wrote:\n> > That said, the whole \"prepend 'x' to the function name\" thing is obviously \n> > how git does things, but maybe it would be more readable in the long term \n> > to make the prefix be 'safe_' instead of 'x', or something like that?\n> \n> If you want a more verbose name than xfoo, I personally like the foo_or_die\n> convention, which reminds you explicitly that the function might kill the\n> program. safe_ might convey exactly the opposite impression.\n\nI think the _gently suffix is already preferred for the\nnon-die()'ing variant of a function within Git. For example see\nsetup_git_directory_gently() or lookup_commit_reference_gently().\n\nOf course pkt-line.c goes and defines safe_read/safe_write.\nAnd lets not forget about safe_create_dir() and\nsafe_create_leading_directories().\n\nI guess we're about half-way in both directions. Next function\ndeclared with safe_ prefix or _gently suffix will tip the scale in\none direction or the other. ;-)\n\n-- \nShawn.\n"}]}