[asterisk-dev] Time for a bug fix phase?

[Steve Davies]

2008/5/28 Russell Bryant <russell at digium.com>:
> Steve Davies wrote:
>> OEJ seems to be proposing a larger fix which rationalises NOTIFY
>> transmissions, which is probably a "Very Good Thing(tm)", but IMHO
>> does not reduce the need to apply "notify.1_4_12.patch" to fix the
>> underlying problem.
>
> IIRC, the attached patch is very much not correct, and that is why it has not
> been committed.  Just because it is one line, doesn't mean it is safe or
> correct.  :)  Anyway, we should look again to be sure ...

I agree that small is not always good, but the initial change was
based on painfully following the flow of the code, and determining
that it made absolutely no sense to set a variable (ignore=1) in an IF
block that is only ever referred to in an associated ELSE block, which
it can never reach - This IIRC is true of this code.

If the ELSE remains, then there is a code path that not only never
uses a variable (ignore) that you just set for some purpose, but also
never processes the packet that is in your hand, causing retries due
to a dropped ACK.

Then see what happens if the ELSE is not an ELSE, and if "ignore" is
both set/unset, and it all looks sane - The called handler understands
that if ignore=1, we just remove the NOTIFY from the list, and destroy
its retransmit timer.

It looks like a perfectly sane patch to me. It fixes the issue, and
has not caused any other noticable issues for the number of people
using it who have given feedback.

I would be happy to have any specific flaws pointed out, as I use this
code in production, and I would be the first to try to fix it again :)

Regards,
Steve