tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mark Thomas <ma...@apache.org>
Subject Re: svn commit: r1665888 - /tomcat/native/branches/1.1.x/native/src/poll.c
Date Wed, 11 Mar 2015 19:22:19 GMT
On 11/03/2015 18:13, Christopher Schultz wrote:
> Rainer,
> 
> On 3/11/15 12:38 PM, Rainer Jung wrote:
>> Am 11.03.2015 um 15:45 schrieb Mark Thomas:
>>> On 11/03/2015 14:44, markt@apache.org wrote:
>>>> Author: markt
>>>> Date: Wed Mar 11 14:44:23 2015
>>>> New Revision: 1665888
>>>>
>>>> URL: http://svn.apache.org/r1665888
>>>> Log:
>>>> Fix 57653. Crash when multiple events for same socket are returned
>>>> via separate apr_pollfd_t structures
>>>
>>> Review appreciated from folks that understand C better than I do (i.e.
>>> pretty much anybody). Hopefully it is clear from the comment what I am
>>> trying to do here and why.
>>
>> I guess it is more of a question of knowing the pollset and RING magic -
>> which I don't really qualify for - but it looks good to me.
> 
> +1
> 
>> Even if for
>> the two fd with the same socket their client_data parts might not be
>> identical, they should point to the same (identical) socket structure,
>> so NULLing "pe" in the socket during the first fd occurence should work
>> as a decision point for the second occurence.
> 
> +1
> 
> Is it a problem to call apr_pollset_remove for that file descriptor if
> it's not actually in the pollset? That is, are remove operations
> idempotent? Would it be considered an error (at a higher level) to try
> to remove the same fd twice?

You get a return code indicating that the fd wasn't in the pollset.

>> The only other case would be some fd which has a NUL s->pe from the
>> beginning, but then the existing code would have also crashed though
>> only after calling apr_pollset_remove(). So I guess that case is not
>> possible.
> 
> +1
> 
> What you (Mark) have done is just avoid a seg fault when s->pe is NULL.

Which should never happen apart from in the case that we are trying to
handle.

> What you haven't done (which might not actually be necessary) is solve
> any issue where this method shouldn't be called in the first place (i.e.
> state management).

I don't believe that is an issue.

We could have tried to merge multiple returns for the one fd but that
would have been a much more complicated patch. I'd rather deal with that
in the Java code.

Mark


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org


Mime
View raw message