Conditions always true in tevent_standard.c

[Uri Simchoni]

On 07/12/2016 02:59 AM, Jeremy Allison wrote:
> 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
>>
> 
> 
RB+ me and Pushed.

The issue with fixing the misuse of epoll_ops by adding a "panicked"
variable is making sure that adding 4 bytes to a structure that gets
accessed each loop doesn't affect performance.

Thanks,
Uri.