Re: [Libevent-users] Re: Dead or wrong code

[Nick Mathewson <nickm@xxxxxxxxxxxxx>]

On Tue, Jun 7, 2011 at 5:58 PM, Nir Soffer <nirsof@xxxxxxxxx> wrote:
>
> On Jun 7, 2011, at 8:48 PM, Gilad Benjamini wrote:
>
>>>> 955         buf->first = chain;
>>>> 956         if (chain) {
>>>> 957             chain->misalign += remaining;
>>>> 958             chain->off -= remaining;
>>>> 959         }
>>>
>>> Draining all the buffer trigger this code.
>>>
>>> Adding an assert in line 957 cause the tests to die, so this code is
>>> certainly not dead yet ;-)
>>
>> Perhaps I wasn't clear.
>
> No, its my fault :-)
>
>> I was referring to line 956. If the code was able to reach line 956, the
>> value of chain cannot be NULL and therefore there is no need for the test.
>> A NULL value for chain would have broken the code earlier.
>
> Indeed, chain is always non-null in line 956.
>
> This check was added in 03afa209 - I don't see why but maybe the author can
> explain why.

Hm.  I think the check is indeed needless.  If we get into the else
case starting on line 928 (of the current patches-2.0 HEAD
4461f1a09662), then either the amount that we want to drain is less
than the total buffer length, or the last data-containing chain in the
buffer is read-pinned, or both.

In the first case we will exit the loop because of the "remaining >=
chain->off" loop condition, and so 'chain' will point to a chain with
less than "remaining" bytes in it.  In the second case, we will exit
because of the CHAIN_PINNED_R() test on line 946; chain will be set
(and incidentally, remaining will be 0).  So I believe the test is
indeed needless.

I'm removing this check in the master branch but leaving it as-is in
2.0, on the grounds that it isn't actually causing undesirable
behavior and as such is a clean-up, not a critical bugfix.

yrs,
-- 
Nick
***********************************************************************
To unsubscribe, send an e-mail to majordomo@xxxxxxxxxxxxx with
unsubscribe libevent-users    in the body.