[asterisk-dev] [Code Review] 4502: Improved and portable ast_log recursion avoidance

[Corey Farrell]

> On March 24, 2015, 5:08 p.m., rmudgett wrote:
> > /branches/13/main/logger.c, lines 1331-1332
> > <https://reviewboard.asterisk.org/r/4502/diff/2/?file=72787#file72787line1331>
> >
> >     I don't think this thread needs to be treated differently than any other thread.  The recursion check is thread specific to prevent double OOM messages on the same thread.

The idea was to prevent ast_log_safe from running in response to any log message.  Since everything that happens in logger_thread is the result of a log message, I was blocking ast_log_safe.  Not blocking the thread will still prevent an infinite recursion loop, so I've removed this block.


- Corey


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


On March 24, 2015, 4:31 p.m., Corey Farrell wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4502/
> -----------------------------------------------------------
> 
> (Updated March 24, 2015, 4:31 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-24155
>     https://issues.asterisk.org/jira/browse/ASTERISK-24155
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This introduces a new logger routine ast_log_safe.  This routine should be used for all error messages in code that can be run as a result of ast_log.  ast_log_safe does nothing if run recursively or from the logger thread.  All error logging in astobj2.c, strings.c and utils.h have been switched to ast_log_safe.  One ast_log from stringfields code in utils.c was also changed.
> 
> I've also added support for raw threadstorage.  This provides direct access to the void* pointer in threadstorage.  In ast_log_safe I use NULL to signify that this thread is not already running ast_log_safe, (void*)1 when it is already running.  This was done since it's critical that ast_log_safe do nothing that could log during recursion checking.
> 
> This review shows the version 13 patch.  Version 11 didn't have the backtrace check for MALLOC_FAILURE_MSG, and trunk uses 'ast_callid' instead of 'struct ast_callid *'.  Patches for each version are on JIRA.
> 
> The idea to use threadstorage to protect certain error logging came from the patch posted by Timo Teräs.
> 
> 
> Diffs
> -----
> 
>   /branches/13/main/utils.c 433360 
>   /branches/13/main/strings.c 433360 
>   /branches/13/main/logger.c 433360 
>   /branches/13/main/hashtab.c 433360 
>   /branches/13/main/astobj2.c 433360 
>   /branches/13/include/asterisk/utils.h 433360 
>   /branches/13/include/asterisk/threadstorage.h 433360 
>   /branches/13/include/asterisk/logger.h 433360 
> 
> Diff: https://reviewboard.asterisk.org/r/4502/diff/
> 
> 
> Testing
> -------
> 
> Verified with 'nm -g main/astobj2.o' that ast_log_safe was being used.
> 
> Tested by further modifying Asterisk with added calls to ast_log_safe().
> * In main() after fully booted.
> * In ast_log_safe() after setting in_safe_log.
> * From the logger thread.
> 
> Only the message after fully booted was shown in the logs, all others were ignored.
> 
> 
> Thanks,
> 
> Corey Farrell
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20150324/6c6598a0/attachment.html>