[Asterisk-code-review] res pjsip: Patch for res pjsip * module load/reload crash (asterisk[master])

Tue Nov 20 17:42:37 CST 2018

sungtae kim has posted comments on this change. ( https://gerrit.asterisk.org/10617 )

Change subject: res_pjsip: Patch for res_pjsip_* module load/reload crash
......................................................................

Patch Set 10:

(14 comments)

No problem! Just fixed it. :)

https://gerrit.asterisk.org/#/c/10617/8/include/asterisk/res_pjsip_session.h
File include/asterisk/res_pjsip_session.h:

https://gerrit.asterisk.org/#/c/10617/8/include/asterisk/res_pjsip_session.h@262
PS8, Line 262: 	struct ast_module *module;
> This breaks the ABI of modules which provide session supplements. […]
Thank you for reviewing. Btw, I don't get it what it suppose to be.. May I get some reference about what you saying it?

https://gerrit.asterisk.org/#/c/10617/8/include/asterisk/res_pjsip_session.h@591
PS8, Line 591: 		ast_sip_session_register_supplement_with_module(AST_MODULE_SELF, supplement)
> This should use AST_MODULE_SELF in 16+. […]
Done

https://gerrit.asterisk.org/#/c/10617/7/include/asterisk/res_pjsip_session.h
File include/asterisk/res_pjsip_session.h:

https://gerrit.asterisk.org/#/c/10617/7/include/asterisk/res_pjsip_session.h@585
PS7, Line 585:  * \param module Referenced module(NULL safe)
> Document that it is safe for it to be NULL
Done

https://gerrit.asterisk.org/#/c/10617/8/res/res_pjsip/include/res_pjsip_private.h
File res/res_pjsip/include/res_pjsip_private.h:

https://gerrit.asterisk.org/#/c/10617/8/res/res_pjsip/include/res_pjsip_private.h@224
PS8, Line 224: int ast_res_pjsip_init_message_filter(void);
> If we find a way to cherry-pick this to 13 I'd prefer this pass 'struct ast_module *module'. […]
Done

https://gerrit.asterisk.org/#/c/10617/9/res/res_pjsip/pjsip_message_filter.c
File res/res_pjsip/pjsip_message_filter.c:

https://gerrit.asterisk.org/#/c/10617/9/res/res_pjsip/pjsip_message_filter.c@529
PS9, Line 529: 	ast_sip_session_register_supplement(&filter_session_supplement);
> In Asterisk 16+ we can just use ast_sip_session_register_supplement(&filter_session_supplement) from […]
Ah! Now I've got it! No problem. :)

https://gerrit.asterisk.org/#/c/10617/7/res/res_pjsip/pjsip_session.c
File res/res_pjsip/pjsip_session.c:

https://gerrit.asterisk.org/#/c/10617/7/res/res_pjsip/pjsip_session.c@41
PS7, Line 41: 	ast_assert(supplement != NULL);
            : 
            : 	s
> This should be an assert if you do want to add a check in.
Done

https://gerrit.asterisk.org/#/c/10617/7/res/res_pjsip/pjsip_session.c@45
PS7, Line 45: 	if (!supplement->response_priority) {
            : 		supplement->response_priority = AST_SIP_SESSION_BEFORE_MEDIA;
            : 	}
> There's no need for this if, it can always be done.
Done

https://gerrit.asterisk.org/#/c/10617/8/res/res_pjsip/pjsip_session.c
File res/res_pjsip/pjsip_session.c:

https://gerrit.asterisk.org/#/c/10617/8/res/res_pjsip/pjsip_session.c@41
PS8, Line 41: 	ast_assert(supplement != NULL);
            : 
            : 	s
> This should simply use `ast_assert(supplement != NULL);`.
Done

https://gerrit.asterisk.org/#/c/10617/8/res/res_pjsip/pjsip_session.c@104
PS8, Line 104: 
> ast_module_ref checks that the module is not NULL so no need to also check here.
Done

https://gerrit.asterisk.org/#/c/10617/5/res/res_pjsip_caller_id.c
File res/res_pjsip_caller_id.c:

https://gerrit.asterisk.org/#/c/10617/5/res/res_pjsip_caller_id.c@782
PS5, Line 782: 	ast_sip_session_register_supplement(&caller_id_supplement);
> Instead of having to do this for every module the ast_sip_session_register_supplement should become  […]
Agree. I've tried that at the beginning like this.

static struct ast_sip_session_supplement caller_id_supplement = {
	.module = ast_module_info->self,
	.method = "INVITE,UPDATE",
	.priority = AST_SIP_SUPPLEMENT_PRIORITY_CHANNEL - 1000,
	.incoming_request = caller_id_incoming_request,
	.incoming_response = caller_id_incoming_response,
	.outgoing_request = caller_id_outgoing_request,
	.outgoing_response = caller_id_outgoing_response,
};

But it caused compile error. Which is ast_module_info is not constant. How can I solve this..? Any idea?

Ah, MACRO! I've got it! You already mentioned it. Thanks! :)

https://gerrit.asterisk.org/#/c/10617/7/res/res_pjsip_session.c
File res/res_pjsip_session.c:

https://gerrit.asterisk.org/#/c/10617/7/res/res_pjsip_session.c@2249
PS7, Line 2249: 
> Remove added red
Done

https://gerrit.asterisk.org/#/c/10617/7/res/res_pjsip_session.c@3320
PS7, Line 3320: 	AST_LIST_TRAVERSE(&session->supplements, iter, next) {
> I don't think these debug messages would be useful after implementing this change.
Done

https://gerrit.asterisk.org/#/c/10617/8/res/res_pjsip_session.c
File res/res_pjsip_session.c:

https://gerrit.asterisk.org/#/c/10617/8/res/res_pjsip_session.c@3331
PS8, Line 3331: 	AST_LIST_TRAVERSE(&session->supplements, iter, next) {
              : 		if (iter->session_destroy) {
              : 			iter->session_destroy(session);
              : 		}
              : 	}
> It's impossible to reach this point with a NULL session so please remove the NULL check. […]
Done

https://gerrit.asterisk.org/#/c/10617/8/res/res_pjsip_session.c@3347
PS8, Line 3347: 	}
              : }
              : 
              : static void handle_incoming_response(struct ast_sip_session *session, pjsip_rx_data *rdata,
              : 		enum ast_sip_session_response_priority response_p
> Same
Done

-- 
To view, visit https://gerrit.asterisk.org/10617
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5b82be3a75d702cf1933d8d1417f44aa10ad1029
Gerrit-Change-Number: 10617
Gerrit-PatchSet: 10
Gerrit-Owner: sungtae kim <pchero21 at gmail.com>
Gerrit-Reviewer: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: Jenkins2 (1000185)
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: sungtae kim <pchero21 at gmail.com>
Gerrit-Comment-Date: Tue, 20 Nov 2018 23:42:37 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20181120/c96a4140/attachment-0001.html>