Conditions always true in tevent_standard.c

[Jeremy Allison]

On Mon, Jul 04, 2016 at 07:17:20PM +0300, Nikolai Kondrashov wrote:
> On 06/30/2016 10:24 PM, Jeremy Allison wrote:
> >On Wed, Jun 29, 2016 at 03:43:34PM +0300, Nikolai Kondrashov wrote:
> >>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.
> 
> I understand, and this sounds like a good convention in general.
> However, I think it makes these small functions and conditions harder to read.
> Perhaps an exception for small functions would be in order.
> 
> Regardless, it's your code and you decide, so here is an updated patch :)

LGTM. Can I get a second Team reviewer ?

> From fd0ad05d311878797e29aa65363b64a502bbc9c3 Mon Sep 17 00:00:00 2001
> From: Nikolai Kondrashov <Nikolai.Kondrashov at redhat.com>
> Date: Wed, 29 Jun 2016 15:05:08 +0300
> Subject: [PATCH] tevent: Clarify apparently useless conditions
> 
> Comment on two similar conditions in tevent_standard.c, which,
> otherwise, at a first glance, seem useless, i.e. always true.
> 
> The conditions checking glue->epoll_ops for being non-NULL, imply that
> it *can* be NULL. A casual reader would not generally expect a "member"
> function to modify its container's pointer in a container higher up, and
> would assume that glue->epoll_ops could be NULL before the call,
> resulting in a near-NULL pointer dereference.
> 
> However, in this case epoll_ops is indeed cleared in those "member"
> functions, in the case of an epoll interface failure, to signify
> fallback to poll interface.
> ---
>  lib/tevent/tevent_standard.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/lib/tevent/tevent_standard.c b/lib/tevent/tevent_standard.c
> index a050901..30e9b91 100644
> --- a/lib/tevent/tevent_standard.c
> +++ b/lib/tevent/tevent_standard.c
> @@ -112,6 +112,11 @@ static int std_event_loop_once(struct tevent_context *ev, const char *location)
>  	int ret;
>  
>  	ret = glue->epoll_ops->loop_once(ev, location);
> +	/*
> +	 * If the above hasn't panicked due to an epoll interface failure,
> +	 * std_fallback_to_poll() wasn't called, and hasn't cleared epoll_ops to
> +	 * signify fallback to poll_ops.
> +	 */
>  	if (glue->epoll_ops != NULL) {
>  		/* No fallback */
>  		return ret;
> @@ -138,6 +143,11 @@ static int std_event_loop_wait(struct tevent_context *ev, const char *location)
>  	int ret;
>  
>  	ret = glue->epoll_ops->loop_wait(ev, location);
> +	/*
> +	 * If the above hasn't panicked due to an epoll interface failure,
> +	 * std_fallback_to_poll() wasn't called, and hasn't cleared epoll_ops to
> +	 * signify fallback to poll_ops.
> +	 */
>  	if (glue->epoll_ops != NULL) {
>  		/* No fallback */
>  		return ret;
> -- 
> 2.8.1
>