Kgenjac/save sip reason state on callback #3210
Conversation
…into kgenjac/save-sip-reason-state-on-callback
|
Not sure what specific problem this is trying to fix, but this will definitely cause memory leaks when |
|
Oh yes, I overlooked that, thank you for pointing that out. The problem is that we need to read the reason header from final SIP responses, but SIP object is only present on If it is a satisfiable solution, I think freeing the memory and setting these variables to NULL before a new |
|
I'm not sure that would be enough: your code doesn't do any g_free before the g_strdup either, meaning that a state followed by a terminated or viceversa would still cause the same leak. Maybe a simpler fix is to only set those variables where you do (so removing the allocation in cancel/bye) and ensuring there's a free before the strdup, even though I feel it could be semantically inaccurate, and there would be a ton of reallocations any time there's an incoming message (who all have a reason, even e.g., 200 OK). Or maybe you should figure out why you're not getting the info in your case: I understand state/terminated don't have the sip object, but the call before probably does, are you getting to those states not via cancel/bye but somehow else? Maybe it's there that the reason string should also be added to? If you are going there via cancel/bye, I'd argue that you do have the reason: you need to intercept bye/cancel events too. |
|
After some exploring, I found that we could use SIP reason received on If this seems okay, I would discard the previous changes and update the PR with the new ones. |
|
That still saves the response code even for provisional and successful responses too. Shouldn't that code only be in the error branch? e.g., error codes >= 400? |
…into kgenjac/save-sip-reason-state-on-callback
|
Yes, I updated the PR to save the reason only on error codes > 400, I also added it for 401 and 407 cases and synced with master. |
| @@ -1504,6 +1504,7 @@ static void janus_sip_media_reset(janus_sip_session *session) { | |||
| gpointer janus_sip_sofia_thread(gpointer user_data); | |||
| /* Sofia callbacks */ | |||
| void janus_sip_sofia_callback(nua_event_t event, int status, char const *phrase, nua_t *nua, nua_magic_t *magic, nua_handle_t *nh, nua_hmagic_t *hmagic, sip_t const *sip, tagi_t tags[]); | |||
| void janus_sip_save_reason(sip_t const *sip, janus_sip_session *session); | |||
There was a problem hiding this comment.
Good idea about the helper method! This should probably be static, but I see other functions aren't either, so that's something I can fix myself in a later commit.
| return; | ||
|
|
||
| if (sip->sip_reason && sip->sip_reason->re_text) { | ||
| session->hangup_reason_header = g_strdup(sip->sip_reason->re_text); |
There was a problem hiding this comment.
I'd add a
g_free(session->hangup_reason_header);
before the g_strdup, so that we avoid memory leaks when a new reason replaces one received before. This was an issue we had already, apparently, so it may be a goot opportunity to fix this for good. The same should be done for the other two g_strdup before.
There was a problem hiding this comment.
Ah actually I see they're freed in nua_i_state so it may not be needed.
There was a problem hiding this comment.
Yes, I saw that it is freed in nua_i_state so I didn't add it in the method.
|
Considering my additional comment, I think it's safe to merge as it. We can then track if memory leaks occur anyway. |
|
Thank you for merging! |
|
On second thought I think I'll add the g_free anyway, just to stay on the safe side. |
Hello,
We didn't have variables from the sip reason header on failure events such as
nua_i_terminatedor onnua_i_stateevent where sip is NULL, so this PR adds an option to save the state of these variables.