feature: sip publish events support #3605
Conversation
|
Thanks for your contribution, @digiboridev! Please make sure you sign our CLA, as it's a required step before we can merge this. |
|
Sorry, can't review this right now, it's way too long for a quick glance and notes. But if it's tied to #3604, see my comments there about Call-IDs. |
|
One thing that will definitely be needed is text in the documentation for this new request, though. |
I propose to refactor this PR to use same mechanism of storing publisher that subscriptions currently have (using event_type and g_int64_hash). So it will be more appropriate to merge with current code, and then be able work on multi-subscription/pubslishers separately. |
Ok, will do it |
lminiero
left a comment
lminiero
left a comment
There was a problem hiding this comment.
Did a first quick review, you can find some notes inline.
| } | ||
| const char *to = json_string_value(json_object_get(root, "to")); | ||
| if(to == NULL) | ||
| to = session->account.identity; |
There was a problem hiding this comment.
Is the To you put there yourself or someone else? Should it be required instead of optional?
There was a problem hiding this comment.
Yes i think is ok to be optional, for presence event for example you can omit "to" for send it to your identity.
src/plugins/janus_sip.c
Outdated
| /* TTL */ | ||
| int ttl = publish_ttl; | ||
| json_t *pub_ttl = json_object_get(root, "publish_ttl"); | ||
| if(pub_ttl && json_is_integer(pub_ttl)) |
There was a problem hiding this comment.
There's no need to check the type when we have validation. You already have the
{"publish_ttl", JANUS_JSON_INTEGER, 0},
to take care of that. I see you do the same thing for other properties you defined, so please fix it for those too.
src/plugins/janus_sip.c
Outdated
| /* If call-id does not exist in request, create a random one */ | ||
| if(callid == NULL) { | ||
| JANUS_LOG(LOG_WARN, "Invalid call_id provided, generating a random one\n"); | ||
| callid = g_malloc0(24); |
There was a problem hiding this comment.
This is a memory leak. You're not freeing this memory when you're done. Adding a g_free would be broken anyway, since if you get it from json_string_value then it should not be freed that way. Probably better to use a generic string, e.g., char new_callid[24], call janus_sip_random_string on that, and have callid be a pointer to that.
src/plugins/janus_sip.c
Outdated
| janus_sip_parse_custom_headers(root, (char *)&custom_headers, sizeof(custom_headers)); | ||
| char *contact_header = janus_sip_session_contact_header_retrieve(session); | ||
| char *proxy = session->helper && session->master ? | ||
| session->master->account.outbound_proxy : session->account.outbound_proxy; |
There was a problem hiding this comment.
Code style: broken indentation (unneeded extra tab).
src/plugins/janus_sip.c
Outdated
| json_t *headers = janus_sip_get_incoming_headers(sip, session); | ||
| json_object_set_new(resultj, "headers", headers); | ||
| } | ||
| json_object_set_new(resultj, "etag", json_string(sip->sip_etag->g_string)); |
There was a problem hiding this comment.
This is missing a check on whether sip->sip_etag and sip->sip_etag->g_string exist, as you do for sip_call_id above. It will crash if it doesn't.
| JANUS_LOG(LOG_VERB, "\t%s\n", auth); | ||
| nua_authenticate(nh, NUTAG_AUTH(auth), TAG_END()); | ||
| } else if(status >= 400) { | ||
| JANUS_LOG(LOG_WARN, "[%s] PUBLISH failed: %d %s\n", session->account.username, status, phrase ? phrase : ""); |
There was a problem hiding this comment.
Is this a tempoary log line? We don't have indicators of failure for any of the other requests.
There was a problem hiding this comment.
Sorry, I don't understand what you mean.
There was a problem hiding this comment.
I meant that we don't print LOG_WARN errors when other requests fail, so I don't see why we should for PUBLISH.
There was a problem hiding this comment.
but SUBSCRIBE has, on line 6806
| JANUS_LOG(LOG_WARN, "Invalid call_id provided, generating a random one\n"); | ||
| char new_callid[24]; | ||
| janus_sip_random_string(24, new_callid); | ||
| callid = new_callid; |
There was a problem hiding this comment.
This still doesn't work, I think, because the array is scoped to this code block, and will disappear when we exit it. The array should be defined outside of the code block to be accessible when you need it.
There was a problem hiding this comment.
please check the change
3 participants
Just adds sip PUBLISH events support.
Logicaly linked to this pr because have the same mechanism for storing publishers.