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

[rmudgett]

> On May 7, 2012, 10:41 a.m., rmudgett wrote:
> > /trunk/main/channel.c, lines 6297-6298
> > <https://reviewboard.asterisk.org/r/1900/diff/2/?file=27729#file27729line6297>
> >
> >     You do not need to check if the channel has a linkedid.  If it does not have one it has already been complained about when it failed to get the initial linkedid.
> >     
> >     With this code, you cannot give a channel a new linkedid if the channel does not already have one.  Therefor, the problem of a channel not having a linkedid cannot be corrected.
> >
> 
> Terry Wilson wrote:
>     > With this code, you cannot give a channel a new linkedid if the channel does not already have one.  Therefor, the problem of a channel not having a linkedid cannot be corrected.
>     
>     It isn't about correcting anything. It is just about warning in case something that *should* be impossible happens. For instance, if someone made a change to the alloc() function where all of the sudden it becomes possible to have a channel without a linkedid, then we don't crash when we hit the strcmp. Yes, bad things will probably still happen with the call, but at least we wouldn't crash. Or if someone calls the function with a NULL/empty linkedid, then fail the assertion or at least warn if it somehow makes it out of development .
>     
>     Chances are it is a condition that will never be hit. When I can, I like to avoid passing NULL to strcmp and crashing.

ast_channel_linkedid() cannot return NULL.  It is a string field.  String fields can never be NULL unless the string fields were never initialized.  For an ast_channel this is not going to happen unless someone really brakes channel allocation.

I did not say remove the check for the passed in linkedid.  I said the check for the channels existing linkedid is not needed.  It is also potentially harmful because you cannot correct a missing linkedid on a channel by giving it one later.


- rmudgett


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


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/0cba6965/attachment-0001.htm>