[Asterisk-code-review] res pjsip: Fix infinite recursion when loading transports f... (asterisk[13])

[Joshua Colp]

Joshua Colp has posted comments on this change.

Change subject: res_pjsip:  Fix infinite recursion when loading transports from realtime
......................................................................


Patch Set 4: Code-Review-1

(9 comments)

https://gerrit.asterisk.org/#/c/2129/4/res/res_pjsip.c
File res/res_pjsip.c:

Line 2488: 	RAII_VAR(struct ast_sip_transport *, transport, NULL, ao2_cleanup);
Do we even need to retrieve this anymore?


https://gerrit.asterisk.org/#/c/2129/4/res/res_pjsip/config_transport.c
File res/res_pjsip/config_transport.c:

Line 36: static AO2_GLOBAL_OBJ_STATIC(current_states);
A global object is overkill here and not really useful. They are really only useful when you have an immutable object being stored and swapped out. If you are merely manipulating the underlying container then a normal container is best,


Line 38: struct internal_state {
Why have a configuration object and 2 state objects? Outbound registration has to do it because of threading interaction between the PJSIP outbound registration client and the threading model of Asterisk, in the case of transports there's much less of a problem.


Line 230: 	internal_state->transport = transport;
You can do the ao2_bump here for transport, it returns the object.


Line 287: 	ao2_lock(states);
You don't seem to unlock this at all.


Line 289: 	old_state = ao2_find(states, transport_id, OBJ_SEARCH_KEY);
Pass OBJ_NOLOCK here so it's not recursively locked.


Line 295: 				ast_log(LOG_WARNING, "Changes to transport '%s' are being ignored\n", transport_id);
Be more detailed with what is being ignored.


Line 299: 		transport->state = old_state->state;
        : 		copy_state_to_transport(transport);
        : 		ao2_replace(old_state->transport, transport);
Does the transport need to be protected at all in other threads?


Line 439: 	ao2_link(states, new_state);
Pass OBJ_NOLOCK to the options variant to prevent recursive locking.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic7a836ea8e786e8def51fe3f8cce855ea54f5f19
Gerrit-PatchSet: 4
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: George Joseph <george.joseph at fairview5.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-HasComments: Yes