{"thread":{"id":"50451","subject":"[Proposed Fix] daemon.c: not initializing revents","startedAt":"2019-02-09T19:57:07Z","lastAt":"2019-02-12T13:58:23Z","messageCount":5,"participants":["Randall S. Becker","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"368941","messageId":"000a01d4c0b1$9ef9ea70$dcedbf50$@nexbridge.com","threadId":"50451","inReplyTo":"000901d4c0b1$1ea15160$5be3f420$@nexbridge.com","subject":"[Proposed Fix] daemon.c: not initializing revents","fromName":"Randall S. Becker","fromEmail":"rsbecker@nexbridge.com","sentAt":"2019-02-09T19:56:55Z","receivedAt":"2019-02-09T19:57:07Z","isPatch":false,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"Hi All,\n\nI found this while trying to track down a hang in t5562 - this isn't the\nfix, but here it is something that could be considered a code-inspection. If\nthere have been random unexplained hangs when git runs as a daemon, this\nmight be the cause.\n\nAccording to many systems (other than Linux), the revents field is supposed\nto be 0 on return to poll(). This was the cause of some heart-ache a while\nback in compat/poll/poll.c. I am not certain whether that copy of poll() is\nused in daemon, but I wanted to point out that the value is being returned\nto poll, outside of compat/poll/poll.c and may present a potential for poll\nreturning an error on that FD due to random values that might be in revents.\n\nPlease see 61b2a1acaae for a related change/justification.\n\ndiff --git a/daemon.c b/daemon.c\nindex 9d2e0d20ef..1e275fc8b3 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -1194,6 +1194,7 @@ static int service_loop(struct socketlist *socklist)\n                                }\n                                handle(incoming, &ss.sa, sslen);\n                        }\n+                       pfd[i].revents = 0;\n                }\n        }\n }\n\nRegards,\nRandall\n\n"},{"id":"369034","messageId":"xmqqsgwugi21.fsf@gitster-ct.c.googlers.com","threadId":"50451","inReplyTo":"000a01d4c0b1$9ef9ea70$dcedbf50$@nexbridge.com","subject":"Re: [Proposed Fix] daemon.c: not initializing revents","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-02-11T20:56:54Z","receivedAt":"2019-02-11T20:57:00Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Randall S. Becker\" <rsbecker@nexbridge.com> writes:\n\n> Hi All,\n>\n> I found this while trying to track down a hang in t5562 - this isn't the\n> fix, but here it is something that could be considered a code-inspection. If\n> there have been random unexplained hangs when git runs as a daemon, this\n> might be the cause.\n>\n> According to many systems (other than Linux), the revents field is supposed\n> to be 0 on return to poll(). This was the cause of some heart-ache a while\n> back in compat/poll/poll.c.\n\nI am having a hard time grokking \"supposed to be 0 on return to\",\nbut do you mean \"the caller must clear .revents field before calling\npoll()\"?\n\nhttp://pubs.opengroup.org/onlinepubs/9699919799/functions/poll.html\nhas this \n\n    In each pollfd structure, poll() shall clear the revents member,\n    except that where the application requested a report on a condition\n    by setting one of the bits of events listed above, poll() shall set\n    the corresponding bit in revents if the requested condition is\n    true. In addition, poll() shall set the POLLHUP, POLLERR, and\n    POLLNVAL flag in revents if the condition is true, even if the\n    application did not set the corresponding bit in events.\n\nand I am also having a hard time interpreting the \"except that\".  If\nwe asked to report (e.g. we set POLLIN in the .events field), poll()\ndoes not have to clear .revents but just set whatever bits it needs\nto set to report the condition in the field?  \n\nIf that is the case, it makes it a bug not to clear .revents before\ncalling poll; the sample code snippet on the same page in EXAMPLES\nsection does not, though, so I am puzzled.\n\nIn any case, no matter what POSIX says, if clearing .revents before\ncalling poll() helps on platforms in the real world, the patch is\nworth taking as a fix, I would think.\n\n> I am not certain whether that copy of poll() is\n> used in daemon, but I wanted to point out that the value is being returned\n> to poll, outside of compat/poll/poll.c and may present a potential for poll\n> returning an error on that FD due to random values that might be in revents.\n>\n> Please see 61b2a1acaae for a related change/justification.\n>\n> diff --git a/daemon.c b/daemon.c\n> index 9d2e0d20ef..1e275fc8b3 100644\n> --- a/daemon.c\n> +++ b/daemon.c\n> @@ -1194,6 +1194,7 @@ static int service_loop(struct socketlist *socklist)\n>                                 }\n>                                 handle(incoming, &ss.sa, sslen);\n>                         }\n> +                       pfd[i].revents = 0;\n>                 }\n>         }\n>  }\n>\n> Regards,\n> Randall\n"},{"id":"369042","messageId":"003501d4c253$05f16fd0$11d44f70$@nexbridge.com","threadId":"50451","inReplyTo":"xmqqsgwugi21.fsf@gitster-ct.c.googlers.com","subject":"RE: [Proposed Fix] daemon.c: not initializing revents","fromName":"Randall S. Becker","fromEmail":"rsbecker@nexbridge.com","sentAt":"2019-02-11T21:44:48Z","receivedAt":"2019-02-11T21:45:02Z","isPatch":false,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On February 11, 2019 15:57, Junio C Hamano wrote:\n> \"Randall S. Becker\" <rsbecker@nexbridge.com> writes:\n> \n> > Hi All,\n> >\n> > I found this while trying to track down a hang in t5562 - this isn't\n> > the fix, but here it is something that could be considered a\n> > code-inspection. If there have been random unexplained hangs when git\n> > runs as a daemon, this might be the cause.\n> >\n> > According to many systems (other than Linux), the revents field is\n> > supposed to be 0 on return to poll(). This was the cause of some\n> > heart-ache a while back in compat/poll/poll.c.\n> \n> I am having a hard time grokking \"supposed to be 0 on return to\", but do\nyou\n> mean \"the caller must clear .revents field before calling poll()\"?\n> \n> http://pubs.opengroup.org/onlinepubs/9699919799/functions/poll.html\n> has this\n> \n>     In each pollfd structure, poll() shall clear the revents member,\n>     except that where the application requested a report on a condition\n>     by setting one of the bits of events listed above, poll() shall set\n>     the corresponding bit in revents if the requested condition is\n>     true. In addition, poll() shall set the POLLHUP, POLLERR, and\n>     POLLNVAL flag in revents if the condition is true, even if the\n>     application did not set the corresponding bit in events.\n> \n> and I am also having a hard time interpreting the \"except that\".  If we\nasked\n> to report (e.g. we set POLLIN in the .events field), poll() does not have\nto\n> clear .revents but just set whatever bits it needs to set to report the\n> condition in the field?\n> \n> If that is the case, it makes it a bug not to clear .revents before\ncalling poll;\n> the sample code snippet on the same page in EXAMPLES section does not,\n> though, so I am puzzled.\n> \n> In any case, no matter what POSIX says, if clearing .revents before\ncalling\n> poll() helps on platforms in the real world, the patch is worth taking as\na fix, I\n> would think.\n\nThat's what my intent was - my explanations are suffering from a little\nwork-induced sleep deprivation. Would you like this as a formal patch?\n\n> \n> > I am not certain whether that copy of poll() is used in daemon, but I\n> > wanted to point out that the value is being returned to poll, outside\n> > of compat/poll/poll.c and may present a potential for poll returning\n> > an error on that FD due to random values that might be in revents.\n> >\n> > Please see 61b2a1acaae for a related change/justification.\n> >\n> > diff --git a/daemon.c b/daemon.c\n> > index 9d2e0d20ef..1e275fc8b3 100644\n> > --- a/daemon.c\n> > +++ b/daemon.c\n> > @@ -1194,6 +1194,7 @@ static int service_loop(struct socketlist\n*socklist)\n> >                                 }\n> >                                 handle(incoming, &ss.sa, sslen);\n> >                         }\n> > +                       pfd[i].revents = 0;\n> >                 }\n> >         }\n> >  }\n\nRegards,\nRandall\n\n"},{"id":"369118","messageId":"xmqqd0nxg1a1.fsf@gitster-ct.c.googlers.com","threadId":"50451","inReplyTo":"003501d4c253$05f16fd0$11d44f70$@nexbridge.com","subject":"Re: [Proposed Fix] daemon.c: not initializing revents","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-02-12T02:59:18Z","receivedAt":"2019-02-12T02:59:23Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Randall S. Becker\" <rsbecker@nexbridge.com> writes:\n\n>> In any case, no matter what POSIX says, if clearing .revents before\n> calling\n>> poll() helps on platforms in the real world, the patch is worth taking as\n> a fix, I\n>> would think.\n>\n> That's what my intent was - my explanations are suffering from a little\n> work-induced sleep deprivation. Would you like this as a formal patch?\n\nThat depends ;-)\n\nAt this late in the cycle, I do not see much urgency for this patch\nto be in the upcoming release (after all, this code survived real\nworld for quite a long time, so it's only minority platforms like\nNonStop that haven't seen serious porting effort until recently that\nwould see improvement---and they have survived without reliably\nworking daemon for so long that they can wait for one more release).\n\nNow, the knowledge that we will have long enough time before the\nfinal version of the formal patch becomes necessary makes me wonder\nwhat the best use of that time to polish the patch would be.  Ideally\nwe'd like to see \"this definitely fixed (or 'worked around') such\nand such breakages on platform X, Y and Z\" instead of my \"Well, we\ncould read POSIX that way, so there may be some platforms that would\nrequire applications to do this, and an extra assignment here would\ncertainly not hurt\", which was the hand-waving I just did.\n\nI dunno.\n\n"},{"id":"369161","messageId":"000201d4c2da$feff72c0$fcfe5840$@nexbridge.com","threadId":"50451","inReplyTo":"xmqqd0nxg1a1.fsf@gitster-ct.c.googlers.com","subject":"RE: [Proposed Fix] daemon.c: not initializing revents","fromName":"Randall S. Becker","fromEmail":"rsbecker@nexbridge.com","sentAt":"2019-02-12T13:58:08Z","receivedAt":"2019-02-12T13:58:23Z","isPatch":false,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On February 11, 2019 21:59, Junio C Hamano wrote:\n> \"Randall S. Becker\" <rsbecker@nexbridge.com> writes:\n> \n> >> In any case, no matter what POSIX says, if clearing .revents before\n> > calling\n> >> poll() helps on platforms in the real world, the patch is worth\n> >> taking as\n> > a fix, I\n> >> would think.\n> >\n> > That's what my intent was - my explanations are suffering from a\n> > little work-induced sleep deprivation. Would you like this as a formal\n> patch?\n> \n> That depends ;-)\n> \n> At this late in the cycle, I do not see much urgency for this patch to be\nin the\n> upcoming release (after all, this code survived real world for quite a\nlong\n> time, so it's only minority platforms like NonStop that haven't seen\nserious\n> porting effort until recently that would see improvement---and they have\n> survived without reliably working daemon for so long that they can wait\nfor\n> one more release).\n> \n> Now, the knowledge that we will have long enough time before the final\n> version of the formal patch becomes necessary makes me wonder what the\n> best use of that time to polish the patch would be.  Ideally we'd like to\nsee\n> \"this definitely fixed (or 'worked around') such and such breakages on\n> platform X, Y and Z\" instead of my \"Well, we could read POSIX that way, so\n> there may be some platforms that would require applications to do this,\nand\n> an extra assignment here would certainly not hurt\", which was the hand-\n> waving I just did.\n> \n> I dunno.\n\nI hear that. I'd rather (not) be working on debugging breakages from other\nauthors that impact my platform. Honestly, I'd rather work at my $DAYJOB,\nalthough, some days...\n\nSince this topic isn't a breakage per-se (no tests seem to be impacted on\nway or another), I agree that this can wait and get through the normal cycle\nof events at some point in the future. Now, if I could only get some help on\nt5562 ;) that would be time well spent for rc0.\n\nCheers,\nRandall\n\n"}]}