[Asterisk-code-review] backtrace: Refactor ast bt get symbols so it doesn't crash (asterisk[13])

[George Joseph]

George Joseph has posted comments on this change. ( https://gerrit.asterisk.org/10602 )

Change subject: backtrace:  Refactor ast_bt_get_symbols so it doesn't crash
......................................................................


Patch Set 6:

(5 comments)

https://gerrit.asterisk.org/#/c/10602/6/main/backtrace.c
File main/backtrace.c:

https://gerrit.asterisk.org/#/c/10602/6/main/backtrace.c@33
PS6, Line 33: #include "asterisk/utils.h"
            : #include "asterisk/strings.h"
> These includes are unused and should be removed.
Done


https://gerrit.asterisk.org/#/c/10602/6/main/backtrace.c@58
PS6, Line 58: #include "asterisk/vector.h"
> My vote is to prevent vector.h from including asterisk/lock.h.  We
 > should avoid as many Asterisk headers as possible.
 > /* Block include of asterisk/lock.h */
 > #define _ASTERISK_LOCK_H
 > 
 > On the other hand maybe vector.h shouldn't include asterisk/lock.h
 > at all.  Any source which uses locking versions of vectors should
 > directly include asterisk/lock.h.

I'm of the opinion that files, whether source or header, should include the things they need directly.  That way users of header files don't need to worry about what they might need to also include.  In any case, I disabled the lock.h inclusion.


https://gerrit.asterisk.org/#/c/10602/6/main/backtrace.c@111
PS6, Line 111: /*
             :  * We CANNOT call the ast_ equivalents for pthread_
             :  * functions here so we need to undo the redirections
             :  * created by lock.h so we can call the pthread_
             :  * functions directly.
             :  */
             : #undef pthread_mutex_lock
             : #undef pthread_mutex_unlock
             : #undef pthread_mutex_t
> I'd prefer this be moved to the top after including headers.  If we
 > remove the include of lock.h from vector.h this will become
 > unnecessary.

done


https://gerrit.asterisk.org/#/c/10602/6/main/backtrace.c@163
PS6, Line 163: 		snprintf(data->msg, MSG_BUFF_LEN, "%s %s %s:%u %s()",
> In this example output the first [%p] of each line is from
 > ast_log_backtrace, the second [%p] is from right here.  Probably
 > should just print the address from here instead of
 > ast_log_backtrace, I don't think ast_log_backtrace can compensate
 > for the inline frames that do not have corresponding entries in the
 > backtrace addresses.

I didn't even notice the duplicate pointers.  Removed from ast_log_backtrace.


https://gerrit.asterisk.org/#/c/10602/6/main/backtrace.c@305
PS6, Line 305: struct ast_vector_string *__ast_bt_get_symbols(void **addresses, size_t num_frames)
             : {
             : 	return __ast_bt_get_symbols_impl(addresses, num_frames);
             : }
> Why not just rename __ast_bt_get_symbols_impl in the two sections?
Done



-- 
To view, visit https://gerrit.asterisk.org/10602
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-MessageType: comment
Gerrit-Change-Id: I79d02862ddaa2423a0809caa4b3b85c128131621
Gerrit-Change-Number: 10602
Gerrit-PatchSet: 6
Gerrit-Owner: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Jenkins2 (1000185)
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Tue, 13 Nov 2018 14:23:14 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20181113/88f6e07f/attachment.html>