From: "Randall S. Becker" <rsbecker@nexbridge.com>
To: "'Junio C Hamano'" <gitster@pobox.com>
Cc: <git@vger.kernel.org>
Subject: RE: [Proposed Fix] daemon.c: not initializing revents
Date: Mon, 11 Feb 2019 16:44:48 -0500 [thread overview]
Message-ID: <003501d4c253$05f16fd0$11d44f70$@nexbridge.com> (raw)
In-Reply-To: <xmqqsgwugi21.fsf@gitster-ct.c.googlers.com>
On February 11, 2019 15:57, Junio C Hamano wrote:
> "Randall S. Becker" <rsbecker@nexbridge.com> writes:
>
> > Hi All,
> >
> > I found this while trying to track down a hang in t5562 - this isn't
> > the fix, but here it is something that could be considered a
> > code-inspection. If there have been random unexplained hangs when git
> > runs as a daemon, this might be the cause.
> >
> > According to many systems (other than Linux), the revents field is
> > supposed to be 0 on return to poll(). This was the cause of some
> > heart-ache a while back in compat/poll/poll.c.
>
> I am having a hard time grokking "supposed to be 0 on return to", but do
you
> mean "the caller must clear .revents field before calling poll()"?
>
> http://pubs.opengroup.org/onlinepubs/9699919799/functions/poll.html
> has this
>
> In each pollfd structure, poll() shall clear the revents member,
> except that where the application requested a report on a condition
> by setting one of the bits of events listed above, poll() shall set
> the corresponding bit in revents if the requested condition is
> true. In addition, poll() shall set the POLLHUP, POLLERR, and
> POLLNVAL flag in revents if the condition is true, even if the
> application did not set the corresponding bit in events.
>
> and I am also having a hard time interpreting the "except that". If we
asked
> to report (e.g. we set POLLIN in the .events field), poll() does not have
to
> clear .revents but just set whatever bits it needs to set to report the
> condition in the field?
>
> If that is the case, it makes it a bug not to clear .revents before
calling poll;
> the sample code snippet on the same page in EXAMPLES section does not,
> though, so I am puzzled.
>
> In any case, no matter what POSIX says, if clearing .revents before
calling
> poll() helps on platforms in the real world, the patch is worth taking as
a fix, I
> would think.
That's what my intent was - my explanations are suffering from a little
work-induced sleep deprivation. Would you like this as a formal patch?
>
> > I am not certain whether that copy of poll() is used in daemon, but I
> > wanted to point out that the value is being returned to poll, outside
> > of compat/poll/poll.c and may present a potential for poll returning
> > an error on that FD due to random values that might be in revents.
> >
> > Please see 61b2a1acaae for a related change/justification.
> >
> > diff --git a/daemon.c b/daemon.c
> > index 9d2e0d20ef..1e275fc8b3 100644
> > --- a/daemon.c
> > +++ b/daemon.c
> > @@ -1194,6 +1194,7 @@ static int service_loop(struct socketlist
*socklist)
> > }
> > handle(incoming, &ss.sa, sslen);
> > }
> > + pfd[i].revents = 0;
> > }
> > }
> > }
Regards,
Randall
next prev parent reply other threads:[~2019-02-11 21:45 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <000901d4c0b1$1ea15160$5be3f420$@nexbridge.com>
2019-02-09 19:56 ` [Proposed Fix] daemon.c: not initializing revents Randall S. Becker
2019-02-11 20:56 ` Junio C Hamano
2019-02-11 21:44 ` Randall S. Becker [this message]
2019-02-12 2:59 ` Junio C Hamano
2019-02-12 13:58 ` Randall S. Becker
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='003501d4c253$05f16fd0$11d44f70$@nexbridge.com' \
--to=rsbecker@nexbridge.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).