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

[George Joseph]

George Joseph has posted comments on this change.

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


Patch Set 4:

(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?
We still need transport->type further down.


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 onl
Too much cut and paste.


Line 38: struct internal_state {
> Why have a configuration object and 2 state objects? Outbound registration 
The separate state object has to be keyed by something and I didn't want to add a string field in it so I used the same wrapper technique as outbound registration.  I could have put a reference to transport in state but that would have created a circular reference.

I'll add the key to the structure and get rid of the internal state.


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


Line 287: 	ao2_lock(states);
> You don't seem to unlock this at all.
The states_cleanup RAII destructor does the ao2_unlock and ao2_unref.


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


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


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?
Nobody saves the transport object anywhere. They all get it as needed.  Since transport_apply is locked on the states container, which gets called for every retrieval from realtime (unfortunately), it's effectively protected.


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


-- 
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: George Joseph <george.joseph at fairview5.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-HasComments: Yes