RE: [Proposed Fix] daemon.c: not initializing revents

On February 11, 2019 15:57, Junio C Hamano wrote:

"Randall S. Becker" [off-list ref] writes:

quoted

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?

quoted

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)

quoted

                                }
                                handle(incoming, &ss.sa, sslen);
                        }
+                       pfd[i].revents = 0;
                }
        }
 }

Regards,
Randall