[Asterisk-code-review] res_pjsip_outbound_registration.c: Use our own scheduler and other s... (asterisk[16])

[Kevin Harwell]

Kevin Harwell has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/15130 )

Change subject: res_pjsip_outbound_registration.c:  Use our own scheduler and other stuff
......................................................................


Patch Set 2: Code-Review-1

(4 comments)

Just thoughts on code reuse mainly.

https://gerrit.asterisk.org/c/asterisk/+/15130/2/res/res_pjsip_outbound_registration.c 
File res/res_pjsip_outbound_registration.c:

https://gerrit.asterisk.org/c/asterisk/+/15130/2/res/res_pjsip_outbound_registration.c@544 
PS2, Line 544: pjsip_regc_info info;
             : 
             : 	pjsip_regc_get_info(client_state->client, &info);
             : 
             : 	callback_invoked = ast_threadstorage_get(&register_callback_invoked, sizeof(int));
             : 	if (!callback_invoked) {
             : 		pjsip_tx_data_dec_ref(tdata);
             : 		ast_log(LOG_ERROR, "Failed to get threadstorage for registration to server '%.*s' from client '%.*s'\n",
             : 				(int) info.server_uri.slen, info.server_uri.ptr,
             : 				(int) info.client_uri.slen, info.client_uri.ptr);
Since info is really only used here in off nominal paths maybe only retrieve it when that happens?


https://gerrit.asterisk.org/c/asterisk/+/15130/2/res/res_pjsip_outbound_registration.c@613 
PS2, Line 613: 	pjsip_regc_get_info(client_state->client, &info);
             : 
             : 	if (pjsip_regc_register(client_state->client, PJ_FALSE, &tdata) != PJ_SUCCESS) {
             : 		ast_log(LOG_ERROR, "Failed to create registration to server '%.*s' from client '%.*s'\n",
             : 				(int) info.server_uri.slen, info.server_uri.ptr,
             : 				(int) info.client_uri.slen, info.client_uri.ptr);
Similar to above. Only retrieve when used by off nominal? Maybe move to own error log function since being used in this capacity multiple times?

Might could do similar for debug as well? Might not be worth it though?


https://gerrit.asterisk.org/c/asterisk/+/15130/2/res/res_pjsip_outbound_registration.c@1457 
PS2, Line 1457: 	pjsip_regc_get_info(state->client_state->client, &info);
              : 
              : 	ast_debug(1, "Performing register to server '%s' from client '%s'  Expiration: %d\n",
              : 		state->registration->server_uri, state->registration->client_uri, registration->expiration);
This can be surrounded with a DEBUG_ATLEAST


https://gerrit.asterisk.org/c/asterisk/+/15130/2/res/res_pjsip_outbound_registration.c@1644 
PS2, Line 1644: 	pjsip_regc_get_info(state->client_state->client, &info);
              : 
              : 	ast_debug(1, "Queueing register to server '%s' from client '%s'\n",
              : 		state->registration->server_uri, state->registration->client_uri);
This too can be surrounded by a DEBUG_ATLEAST



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

Gerrit-Project: asterisk
Gerrit-Branch: 16
Gerrit-Change-Id: I4534a0fc78c7cb69f23b7b449dda9748c90daca2
Gerrit-Change-Number: 15130
Gerrit-PatchSet: 2
Gerrit-Owner: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Comment-Date: Tue, 03 Nov 2020 23:27:53 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20201103/23c24c72/attachment.html>