{"thread":{"id":"22445","subject":"[PATCH] run-command.c: fix build warnings on Ubuntu","startedAt":"2010-01-29T22:38:19Z","lastAt":"2011-03-17T22:34:26Z","messageCount":8,"participants":["Michael Wookey","Markus Heidelberg","Jonathan Nieder","Junio C Hamano","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"133031","messageId":"d2e97e801001291438k21a652cakb05ec34fc8bee227@mail.gmail.com","threadId":"22445","inReplyTo":null,"subject":"[PATCH] run-command.c: fix build warnings on Ubuntu","fromName":"Michael Wookey","fromEmail":"michaelwookey@gmail.com","sentAt":"2010-01-29T22:38:19Z","receivedAt":"2010-01-29T22:38:19Z","isPatch":true,"sender":{"key":"michaelwookey@gmail.com","avatar":"https://avatars.githubusercontent.com/u/19476?v=4"},"body":"Building git on Ubuntu 9.10 warns that the return value of write(2)\nisn't checked. These warnings were introduced in commits:\n\n 2b541bf8 (\"start_command: detect execvp failures early\")\n a5487ddf (\"start_command: report child process setup errors to the\nparent's stderr\")\n\nGCC details:\n\n $ gcc --version\n gcc (Ubuntu 4.4.1-4ubuntu9) 4.4.1\n\nSilence the warnings by reading (but not making use of) the return value\nof write(2).\n\nSigned-off-by: Michael Wookey \n---\nAlthough this will fix the build warnings, I am unsure if there is a\nbetter way to achieve the same result. Using \"(void)write(...)\" still\ngives warnings and I am unaware of any annotations that will silence\ngcc.\n\n run-command.c | 10 ++++++----\n 1 files changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 2feb493..3206d61 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -67,19 +67,21 @@ static int child_notifier = -1;\n\n static void notify_parent(void)\n {\n-\twrite(child_notifier, \"\", 1);\n+\tssize_t unused;\n+\tunused = write(child_notifier, \"\", 1);\n }\n\n static NORETURN void die_child(const char *err, va_list params)\n {\n \tchar msg[4096];\n+\tssize_t unused;\n \tint len = vsnprintf(msg, sizeof(msg), err, params);\n \tif (len > sizeof(msg))\n \t\tlen = sizeof(msg);\n\n-\twrite(child_err, \"fatal: \", 7);\n-\twrite(child_err, msg, len);\n-\twrite(child_err, \"\\n\", 1);\n+\tunused = write(child_err, \"fatal: \", 7);\n+\tunused = write(child_err, msg, len);\n+\tunused = write(child_err, \"\\n\", 1);\n \texit(128);\n }\n\n-- \n1.7.0.rc0.48.gdace5\n"},{"id":"133121","messageId":"201001301743.27439.markus.heidelberg@web.de","threadId":"22445","inReplyTo":"d2e97e801001291438k21a652cakb05ec34fc8bee227@mail.gmail.com","subject":"Re: [PATCH] run-command.c: fix build warnings on Ubuntu","fromName":"Markus Heidelberg","fromEmail":"markus.heidelberg@web.de","sentAt":"2010-01-30T16:43:27Z","receivedAt":"2010-01-30T16:43:27Z","isPatch":true,"sender":{"key":"markus.heidelberg@web.de","avatar":"https://avatars.githubusercontent.com/u/6334512?v=4"},"body":"Michael Wookey, 2010-01-29:\n> Building git on Ubuntu 9.10 warns that the return value of write(2)\n> isn't checked.\n> \n> GCC details:\n> \n> $ gcc --version\n> gcc (Ubuntu 4.4.1-4ubuntu9) 4.4.1\n> \n> Silence the warnings by reading (but not making use of) the return value\n> of write(2).\n\nSince a few weeks I get several warnings about fwrite(), currently 28\ntimes this:\nwarning: ignoring return value of ‘fwrite’, declared with attribute warn_unused_result\n\ngcc (Gentoo 4.3.4 p1.0, pie-10.1.5) 4.3.4\n\nNot sure if it should be muted, that are really many places.\n\nMarkus\n"},{"id":"163418","messageId":"20110316035135.GA30348@elie","threadId":"22445","inReplyTo":"d2e97e801001291438k21a652cakb05ec34fc8bee227@mail.gmail.com","subject":"[PATCH] run-command: prettify -D_FORTIFY_SOURCE workaround","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-03-16T03:51:52Z","receivedAt":"2011-03-16T03:51:52Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Current gcc + glibc with -D_FORTIFY_SOURCE try very aggressively to\nprotect against a programming style which uses write(...) without\nchecking the return value for errors. Even the usual hint of casting\nto (void) does not suppress the warning.\n\nSometimes when there is an output error, especially right before exit,\nthere really is nothing to be done. The obvious solution, adopted in\nv1.7.0.3~20^2 (run-command.c: fix build warnings on Ubuntu,\n2010-01-30), is to save the return value to a dummy variable:\n\n\tssize_t dummy;\n\tdummy = write(...);\n\nBut that (1) is ugly and (2) triggers -Wunused-but-set-variable\nwarnings with gcc-4.6 -Wall, so we are not much better off than when\nwe started.\n\nInstead, use an \"if\" statement with an empty body to make the intent\nclear.\n\n\tif (write(...))\n\t\t; /* yes, yes, there was an error. */\n\nSigned-off-by: Jonathan Nieder \n---\nHi,\n\nMichael Wookey wrote:\n\n> Although this will fix the build warnings, I am unsure if there is a\n> better way to achieve the same result. Using \"(void)write(...)\" still\n> gives warnings and I am unaware of any annotations that will silence\n> gcc.\n\nIt's been a long time (and meanwhile the patch has been working;\nthanks!). How about something like this?\n\n run-command.c | 12 ++++++------\n 1 files changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 3206d61..5b68907 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -67,21 +67,21 @@ static int child_notifier = -1;\n \n static void notify_parent(void)\n {\n-\tssize_t unused;\n-\tunused = write(child_notifier, \"\", 1);\n+\tif (write(child_notifier, \"\", 1))\n+\t\t; /* ok. */\n }\n \n static NORETURN void die_child(const char *err, va_list params)\n {\n \tchar msg[4096];\n-\tssize_t unused;\n \tint len = vsnprintf(msg, sizeof(msg), err, params);\n \tif (len > sizeof(msg))\n \t\tlen = sizeof(msg);\n \n-\tunused = write(child_err, \"fatal: \", 7);\n-\tunused = write(child_err, msg, len);\n-\tunused = write(child_err, \"\\n\", 1);\n+\tif (write(child_err, \"fatal: \", 7) ||\n+\t write(child_err, msg, len) ||\n+\t write(child_err, \"\\n\", 1))\n+\t\t; /* ok. */\n \texit(128);\n }\n \n-- \n1.7.4.1\n"},{"id":"163423","messageId":"7v7hbzaan9.fsf@alter.siamese.dyndns.org","threadId":"22445","inReplyTo":"20110316035135.GA30348@elie","subject":"Re: [PATCH] run-command: prettify -D_FORTIFY_SOURCE workaround","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-16T05:37:30Z","receivedAt":"2011-03-16T05:37:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Instead, use an \"if\" statement with an empty body to make the intent\n> clear.\n>\n> \tif (write(...))\n> \t\t; /* yes, yes, there was an error. */\n\nYuck --- and that is not meant against your workaround, but against the\ncompiler bogosity. The above is reasonable (for some definition of the\nword) and the comment makes the yuckiness tolerable by being somewhat\namusing.\n\nBut your comment in the actual patch is not amusing at all.\n\nIt certainly is _not_ \"ok\" to see errors from write(2); we are _ignoring_\nthe error because at that point in the codepath there isn't any better\nalternative. The unusual \"if ()\" whose condition is solely for its side\neffect, with an empty body, is a strong enough sign to any reader that\nthere is something fishy going on, and it would be helpful to the reader\nto hint _why_ such an unusual construct is there. It would be much better\nfor the longer term maintainability to say at least \"gcc\" in the comment,\ni.e.\n\n\tif (write(...))\n \t; /* we know we are ignoring the error, mr gcc! */\n\nor something.\n\nThanks for another amusing patch.\n"},{"id":"163435","messageId":"20110316073239.GJ5988@elie","threadId":"22445","inReplyTo":"7v7hbzaan9.fsf@alter.siamese.dyndns.org","subject":"[PATCH v2] run-command: prettify -D_FORTIFY_SOURCE workaround","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-03-16T07:32:39Z","receivedAt":"2011-03-16T07:32:39Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Current gcc + glibc with -D_FORTIFY_SOURCE try very aggressively to\nprotect against a programming style which uses write(...) without\nchecking the return value for errors. Even the usual hint of casting\nto (void) does not suppress the warning.\n\nSometimes when there is an output error, especially right before exit,\nthere really is nothing to be done. The obvious solution, adopted in\nv1.7.0.3~20^2 (run-command.c: fix build warnings on Ubuntu,\n2010-01-30), is to save the return value to a dummy variable:\n\n\tssize_t dummy;\n\tdummy = write(...);\n\nBut that (1) is ugly and (2) triggers -Wunused-but-set-variable\nwarnings with gcc-4.6 -Wall, so we are not much better off than when\nwe started.\n\nInstead, use an \"if\" statement with an empty body to make the intent\nclear.\n\n\tif (write(...))\n\t\t; /* yes, yes, there was an error. */\n\nSigned-off-by: Jonathan Nieder \nImproved-by: Junio C Hamano \n---\nJunio C Hamano wrote:\n\n> The unusual \"if ()\" whose condition is solely for its side\n> effect, with an empty body, is a strong enough sign to any reader that\n> there is something fishy going on, and it would be helpful to the reader\n> to hint _why_ such an unusual construct is there. It would be much better\n> for the longer term maintainability to say at least \"gcc\" in the comment,\n> i.e.\n> \n> \tif (write(...))\n> \t; /* we know we are ignoring the error, mr gcc! */\n\nVery true. Some comments to that effect below.\n\n run-command.c | 17 +++++++++++------\n 1 files changed, 11 insertions(+), 6 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 3206d61..ecd9d1c 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -67,21 +67,26 @@ static int child_notifier = -1;\n \n static void notify_parent(void)\n {\n-\tssize_t unused;\n-\tunused = write(child_notifier, \"\", 1);\n+\t/*\n+\t * execvp failed. If possible, we'd like to let start_command\n+\t * know, so failures like ENOENT can be handled right away; but\n+\t * otherwise, finish_command will still report the error.\n+\t */\n+\tif (write(child_notifier, \"\", 1))\n+\t\t; /* yes, dear gcc -D_FORTIFY_SOURCE, there was an error. */\n }\n \n static NORETURN void die_child(const char *err, va_list params)\n {\n \tchar msg[4096];\n-\tssize_t unused;\n \tint len = vsnprintf(msg, sizeof(msg), err, params);\n \tif (len > sizeof(msg))\n \t\tlen = sizeof(msg);\n \n-\tunused = write(child_err, \"fatal: \", 7);\n-\tunused = write(child_err, msg, len);\n-\tunused = write(child_err, \"\\n\", 1);\n+\tif (write(child_err, \"fatal: \", 7) ||\n+\t write(child_err, msg, len) ||\n+\t write(child_err, \"\\n\", 1))\n+\t\t; /* yes, gcc -D_FORTIFY_SOURCE, we know there was an error. */\n \texit(128);\n }\n \n-- \n1.7.4.1\n"},{"id":"163440","messageId":"4D80801A.1000208@viscovery.net","threadId":"22445","inReplyTo":"7v7hbzaan9.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] run-command: prettify -D_FORTIFY_SOURCE workaround","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2011-03-16T09:17:14Z","receivedAt":"2011-03-16T09:17:14Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 3/16/2011 6:37, schrieb Junio C Hamano:\n> It certainly is _not_ \"ok\" to see errors from write(2); we are _ignoring_\n> the error because at that point in the codepath there isn't any better\n> alternative. The unusual \"if ()\" whose condition is solely for its side\n> effect, with an empty body, is a strong enough sign to any reader that\n> there is something fishy going on, and it would be helpful to the reader\n> to hint _why_ such an unusual construct is there. It would be much better\n> for the longer term maintainability to say at least \"gcc\" in the comment,\n> i.e.\n> \n> \tif (write(...))\n> \t; /* we know we are ignoring the error, mr gcc! */\n\nAnd what about compilers that warn:\n\n\t';' : empty controlled statement found; is this the intent?\n\nThat's from MSVC. Perhaps:\n\n\tif (write(...))\n\t\t(void)0; /* we know we are ignoring the error, mr gcc! */\n\n-- Hannes\n"},{"id":"163442","messageId":"20110316092526.GA7886@elie","threadId":"22445","inReplyTo":"4D80801A.1000208@viscovery.net","subject":"Re: [PATCH] run-command: prettify -D_FORTIFY_SOURCE workaround","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-03-16T09:25:26Z","receivedAt":"2011-03-16T09:25:26Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Johannes Sixt wrote:\n\n> And what about compilers that warn:\n> \n> \t';' : empty controlled statement found; is this the intent?\n>\n> That's from MSVC. Perhaps:\n>\n> \tif (write(...))\n> \t\t(void)0; /* we know we are ignoring the error, mr gcc! */\n\nMm, thanks for pointing it out.\n\nYour suggestion is part of a bigger change that imho should go in a\nseparate patch:\n\n\t$ git grep -F -e '\t; /*' origin/master | wc -l\n\t65\n\nI would prefer to see such a patch do\n\n\tif (write(...)) {\n\t\t/* ... explanation goes here ... */\n\t}\n\nor something like\n\n\t#define do_nothing() do { /* nothing */ } while (0)\n\n\tif (write(...))\n\t\tdo_nothing();\t/* ... explanation ... */\n\nbut that is a small detail.\n"},{"id":"163601","messageId":"7vipvh1iml.fsf@alter.siamese.dyndns.org","threadId":"22445","inReplyTo":"20110316073239.GJ5988@elie","subject":"Re: [PATCH v2] run-command: prettify -D_FORTIFY_SOURCE workaround","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-17T22:34:26Z","receivedAt":"2011-03-17T22:34:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> static NORETURN void die_child(const char *err, va_list params)\n> {\n> ...\n> -\tunused = write(child_err, \"fatal: \", 7);\n> -\tunused = write(child_err, msg, len);\n> -\tunused = write(child_err, \"\\n\", 1);\n> +\tif (write(child_err, \"fatal: \", 7) ||\n> +\t write(child_err, msg, len) ||\n> +\t write(child_err, \"\\n\", 1))\n> +\t\t; /* yes, gcc -D_FORTIFY_SOURCE, we know there was an error. */\n\nStrictly speaking, this changes behaviour by stopping at the first failure\nfrom write(2), but I don't think we care.\n\nThanks.\n"}]}