On Tue, Jun 7, 2011 at 5:58 PM, Nir Soffer <nir...@gmail.com> 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 majord...@freehaven.net with
unsubscribe libevent-users in the body.
