[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 6: Code-Review-1

(9 comments)

Feel free to decide which comments to ignore or address.  I don't mind if the sip_options_contact_statuses is deferred to another patch (this is already quite large), but I think it is worth addressing.

The AST_VECTOR_APPEND I mentioned should have error checking, that's the main reason for my -1.

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

https://gerrit.asterisk.org/#/c/7710/6/res/res_pjsip/pjsip_options.c@338
PS6, Line 338: 	return status_map[status];
Do we want:
ast_assert(status < ARRAY_LEN(status_map));


https://gerrit.asterisk.org/#/c/7710/6/res/res_pjsip/pjsip_options.c@343
PS6, Line 343: 	return short_status_map[status];
Do we want:
ast_assert(status < ARRAY_LEN(short_status_map));


https://gerrit.asterisk.org/#/c/7710/6/res/res_pjsip/pjsip_options.c@472
PS6, Line 472: 	if (!sip_options_contact_statuses) {
This is concerning.  Why can't we fix the module initialization order to always create the container before it's possible to use it?

What about during shutdown, is it possible for this block to recreate sip_options_contact_statuses after it's already been destroyed, or access the sip_options_contact_statuses while it's being free'd but before it's been NULL'ed?


https://gerrit.asterisk.org/#/c/7710/6/res/res_pjsip/pjsip_options.c@1363
PS6, Line 1363: 				ast_log(LOG_ERROR, "Unable to schedule qualify for contacts of AOR '%s'\n",
Is it not appropriate to return -1 here?


https://gerrit.asterisk.org/#/c/7710/6/res/res_pjsip/pjsip_options.c@1530
PS6, Line 1530: 	AST_VECTOR_APPEND(&task_data->aor_options->compositors,
AST_VECTOR_APPEND could fail, leaking a reference to task_data->endpoint_state_compositor.


https://gerrit.asterisk.org/#/c/7710/6/res/res_pjsip/pjsip_options.c@1750
PS6, Line 1750: 			return 0;
Error message or return -1?


https://gerrit.asterisk.org/#/c/7710/6/res/res_pjsip/pjsip_options.c@1960
PS6, Line 1960: 			return 0;
Error message or return -1?


https://gerrit.asterisk.org/#/c/7710/6/res/res_pjsip/pjsip_options.c@2098
PS6, Line 2098: 				ast_log(LOG_ERROR, "Unable to schedule qualify for contacts of AOR '%s'\n",
return -1?


https://gerrit.asterisk.org/#/c/7710/6/res/res_pjsip/pjsip_options.c@2212
PS6, Line 2212: 		ao2_cleanup(task_data->aor_options);
Nit: task_data->aor_options is NULL.



-- 
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: 6
Gerrit-Owner: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Fri, 27 Apr 2018 15:55:47 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180427/b55fbf69/attachment.html>