[Asterisk-code-review] pjsip: Rewrite OPTIONS support with new eyes. (asterisk[13])

[Corey Farrell]

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

Change subject: pjsip: Rewrite OPTIONS support with new eyes.
......................................................................


Patch Set 1: Code-Review-1

(10 comments)

I don't know enough about pjsip to review this for correct handling of OPTIONS, so this is just a generic review.

https://gerrit.asterisk.org/#/c/7710/1/res/res_pjsip/pjsip_configuration.c
File res/res_pjsip/pjsip_configuration.c:

https://gerrit.asterisk.org/#/c/7710/1/res/res_pjsip/pjsip_configuration.c@1140
PS1, Line 1140: 	return 0;
return blob ? 0 : -1;


https://gerrit.asterisk.org/#/c/7710/1/res/res_pjsip/pjsip_options.c
File res/res_pjsip/pjsip_options.c:

https://gerrit.asterisk.org/#/c/7710/1/res/res_pjsip/pjsip_options.c@470
PS1, Line 470: static void sip_options_aor_destroy(void *obj)
Nit: could you move this just before sip_options_aor_alloc?  Keeping them together makes it easier to compare now and in the future.


https://gerrit.asterisk.org/#/c/7710/1/res/res_pjsip/pjsip_options.c@475
PS1, Line 475: 	ao2_callback(aor_options->permanent_contacts, OBJ_NODATA | OBJ_UNLINK, sip_options_remove_contact, aor_options);
             : 	ao2_callback(aor_options->dynamic_contacts, OBJ_NODATA | OBJ_UNLINK, sip_options_remove_contact, aor_options);
Need to NULL check each of these containers.


https://gerrit.asterisk.org/#/c/7710/1/res/res_pjsip/pjsip_options.c@652
PS1, Line 652: 			ast_sip_persistent_endpoint_publish_contact_state(endpoint_state_compositor->name, contact_status);
Extra indentation.


https://gerrit.asterisk.org/#/c/7710/1/res/res_pjsip/pjsip_options.c@850
PS1, Line 850: 		return 0;
Leaks endpoint.


https://gerrit.asterisk.org/#/c/7710/1/res/res_pjsip/pjsip_options.c@874
PS1, Line 874: 	if (ast_sip_send_out_of_dialog_request(tdata, endpoint, (int)(aor_options->qualify_timeout * 1000), contact_callback_data, qualify_contact_cb) != PJ_SUCCESS) {
It doesn't look like ast_sip_send_out_of_dialog_request steals a reference to endpoint even in success.


https://gerrit.asterisk.org/#/c/7710/1/res/res_pjsip/pjsip_options.c@1055
PS1, Line 1055: 	existing_permanent_contacts = ao2_container_clone(aor_options->permanent_contacts, 0);
It seems like this could be expensive?  It can be optimized away if (!aor->permanent_contacts), in that case you can just OBJ_UNLINK and run sip_options_remove_contact directly on aor_options->permanent_contacts.

In addition for the if (aor->permanent_contacts) branch should we lock aor_options->permanent_contacts before the clone and unlock after we are done relinking contacts?


https://gerrit.asterisk.org/#/c/7710/1/res/res_pjsip/pjsip_options.c@1165
PS1, Line 1165: 		AST_VECTOR_GET(&task_data->aor_options->compositors, i);
Indent


https://gerrit.asterisk.org/#/c/7710/1/res/res_pjsip/pjsip_options.c@1278
PS1, Line 1278: 	AST_VECTOR_APPEND(&task_data->aor_options->compositors, ao2_bump(task_data->endpoint_state_compositor));
aor_options->compositors is safe to modify without a lock?


https://gerrit.asterisk.org/#/c/7710/1/res/res_pjsip/pjsip_options.c@1283
PS1, Line 1283: 		task_data->endpoint_state_compositor->available++;
No debug for this manipulation of available?



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

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-MessageType: comment
Gerrit-Change-Id: I6a5ebbfca9001dfe933eaeac4d3babd8d2e6f082
Gerrit-Change-Number: 7710
Gerrit-PatchSet: 1
Gerrit-Owner: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Comment-Date: Tue, 26 Dec 2017 07:05:24 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20171226/5f40f4fa/attachment.html>