[Asterisk-code-review] chan sip: Don't unload module while TCP/TLS threads are stil... (asterisk[master])

[Corey Farrell]

Corey Farrell has posted comments on this change. ( https://gerrit.asterisk.org/4536 )

Change subject: chan_sip: Don't unload module while TCP/TLS threads are still running
......................................................................


Patch Set 3: Code-Review-1

(3 comments)

I have not commented on all the issues with this patch.  Please allow me to post a patch for this, I have a pretty good idea of how chan_sip unload can / can't be reordered.  I have a patch nearly completed but need a little more time to scrutinize it before I can post.

https://gerrit.asterisk.org/#/c/4536/3/channels/chan_sip.c
File channels/chan_sip.c:

Line 1783: 		if (event_state_compositors[i].compositor) {
The if statement is not needed, it is safe to call ao2_cleanup on a NULL value (so setting NULL is all that is needed.


Line 35200: 	if (!threadt) {
Unclear why this conditional is needed, load_module should never be run unless unload_module was completely successful (return 0).

I'll have to double-check the code in loader.c, but if this is needed I'm inclined to say that's a bug in the module loader.


PS3, Line 35533: 	default_tls_cfg.certfile = ast_strdup("");
               : 	default_tls_cfg.pvtfile = ast_strdup("");
               : 	default_tls_cfg.cipher = ast_strdup("");
               : 	default_tls_cfg.cafile = ast_strdup("");
               : 	default_tls_cfg.capath = ast_strdup("");
No can do.  Each of these are memory leaks.


-- 
To view, visit https://gerrit.asterisk.org/4536
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I23a8abf9e8f73b57ff7ada61984b8506aea6cbde
Gerrit-PatchSet: 3
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Michael Kuron <m.kuron at gmx.de>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: Michael Kuron <m.kuron at gmx.de>
Gerrit-HasComments: Yes