Conditions always true in tevent_standard.c

[Jeremy Allison]

On Wed, Jun 29, 2016 at 03:43:34PM +0300, Nikolai Kondrashov wrote:
> On 06/28/2016 08:30 PM, Jeremy Allison wrote:
> >On Tue, Jun 28, 2016 at 02:07:10PM +0300, Nikolai Kondrashov wrote:
> >>Hi everyone,
> >>
> >>I was reading about tevent and browsing the code, when I noticed these two
> >>conditions in tevent_standard.c:
> >>
> >>     ret = glue->epoll_ops->loop_once(ev, location);
> >>     if (glue->epoll_ops != NULL) {
> >>         /* No fallback */
> >>         return ret;
> >>     }
> >>
> >>and
> >>
> >>     ret = glue->epoll_ops->loop_wait(ev, location);
> >>     if (glue->epoll_ops != NULL) {
> >>         /* No fallback */
> >>         return ret;
> >>     }
> >>
> >>To me it seems they are never going to be false (the program would segfault
> >>before they are reached if glue->epoll_ops is NULL) and I suspect there's some
> >>logic bug here. However, unfortunately, I don't have time to figure out what
> >>exactly should be done about this, so I'll leave it up to you.
> >
> >No, it isn't a logic bug (I had to explain the same thing to a
> >Google engineer). The epoll backend can fall back on an epoll
> >syscall failure (early versions of Linux might have had problems
> >here) so check out the code in tevent_standard.c:
> >
> >#ifdef HAVE_EPOLL
> >                tevent_epoll_set_panic_fallback(ev, std_fallback_to_poll);
> >#endif
> 
> Yeah, I understood that much.
> 
> >>Still, if there's some purpose to that, then I think a comment is in order.
> >
> >Please submit a comment as a git-am patch, once you grok the code :-).
> 
> Ah, I see now. That was unexpected :)
> 
> Please find the patch attached. I also tried to rearrange the conditions to
> make the code easier to understand.

I like the comment. I don't like the rearrangement of conditions.

In Samba we prefer simplified if statement that cause early exit,
rather than complex if conditions. Unfortunately your patch makes
the if conditions complex.

I'd prefer something that just adds the comment before the
if == NULL check.