[asterisk-dev] [Code Review]: Two new CEL-related fixes

[Terry Wilson]

> On May 4, 2012, 11:37 p.m., rmudgett wrote:
> > /trunk/main/cel.c, line 473
> > <https://reviewboard.asterisk.org/r/1900/diff/1/?file=27670#file27670line473>
> >
> >     This should exit early if the given linkedid is empty.

Done.


> On May 4, 2012, 11:37 p.m., rmudgett wrote:
> > /trunk/main/channel.c, line 651
> > <https://reviewboard.asterisk.org/r/1900/diff/1/?file=27671#file27671line651>
> >
> >     This should be:
> >     return ast_atomic_fetchadd_int(&chancount, 0);
> 
> rmudgett wrote:
>     Technically it should be using ast_atomic_fetchadd_int() to get a guaranteed consistent value but it isn't done anywhere else in Asterisk this way.

Done.


> On May 4, 2012, 11:37 p.m., rmudgett wrote:
> > /trunk/main/channel.c, lines 6297-6303
> > <https://reviewboard.asterisk.org/r/1900/diff/1/?file=27671#file27671line6297>
> >
> >     This code is not going to set a linkedid at all if the current linkedid is empty.
> >     
> >     You should just remove the ast_strlen_zero check.  With the ast_cel_linkedid_ref() doing nothing if linkedid is empty as indicated earlier.
> >

Linkedids should *never* be empty after allocation. I'm assuming the check for ast_strlen_zero is there so that strcmp doesn't crash if for some reason ast_channel_linkedid(chan) returns NULL. This *should* never happen. But if it does, we probably shouldn't crash in production if we can avoid it.

We could just test for NULL, but "empty" would also be an error. We should really be testing both linkedid and ast_channel_linkedid(chan) for NULL before doing anything if we really wanted to be "safe". How about throwing in an ERROR message and an ast_assert()?


- Terry


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/1900/#review6159
-----------------------------------------------------------


On May 7, 2012, 9:10 a.m., Terry Wilson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1900/
> -----------------------------------------------------------
> 
> (Updated May 7, 2012, 9:10 a.m.)
> 
> 
> Review request for Asterisk Developers, Mark Michelson and rmudgett.
> 
> 
> Summary
> -------
> 
> This patch fixes to situations that could cause the CEL LINKEDID_END event to be missed.
> 
> 1) During a core stop gracefully, modules are unloaded when ast_active_channels == 0. The LINKDEDID_END event fires during the channel destructor. This means that occasionally, the cel_* module will be unloaded before the channel is destroyed. It seemed generally useful to wait until the refcount of all channel == 0 before unloading, so I added a channel counter and used it in the shutdown code.
> 
> 2) During a masquerade, ast_channel_change_linkedid is called. It calls ast_cel_check_retire_linkedid which unrefs the linkedid in the linkedids container in cel.c. It didn't ref the new linkedid. Now it does. I also changed the logic a little, since it used to call ast_channel_linkedid_set() even when it hadn't changed (or was blank, which should be verboten).
> 
> (this patch is against trunk, but it will go in 1.8+)
> 
> 
> Diffs
> -----
> 
>   /trunk/include/asterisk/cel.h 365451 
>   /trunk/include/asterisk/channel.h 365451 
>   /trunk/main/asterisk.c 365451 
>   /trunk/main/cel.c 365451 
>   /trunk/main/channel.c 365451 
> 
> Diff: https://reviewboard.asterisk.org/r/1900/diff
> 
> 
> Testing
> -------
> 
> My CEL test passed 100 times or so and transferred calls no longer show an error.
> 
> 
> Thanks,
> 
> Terry
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120507/527e2a53/attachment.htm>