[Asterisk-code-review] Change in asterisk[13]: Detect potential forwarding loops based on count.

[Mark Michelson (Code Review)]

Mark Michelson has posted comments on this change.

Change subject: Detect potential forwarding loops based on count.
......................................................................


Patch Set 2:

(4 comments)

https://gerrit.asterisk.org/#/c/121/2/main/max_forwards.c
File main/max_forwards.c:

Line 4:  * Copyright (C) 2015, mfgium, Inc.
> mfgium!
Uh, didn't you get the memo? ;)


Line 140: mf_datastore = ast_channel_datastore_find(chan, &max_forwards_info, NULL);
        : 	if (!mf_datastore) {
        : 		return ast_max_forwards_set(chan, DEFAULT_MAX_FORWARDS);
        : 	}
> Why not go ahead and decrement the default when you initially set it when t
Sure thing!


https://gerrit.asterisk.org/#/c/121/2/res/res_pjsip/pjsip_global_headers.c
File res/res_pjsip/pjsip_global_headers.c:

Line 90: 		pjsip_hdr *hdr;
       : 		pj_str_t hdr_name;
       : 		/* Only add the header if it's not already present. It's a good bet that
       : 		 * if someone already added this header, they're much smarter than us.
       : 		 */
       : 		pj_cstr(&hdr_name, iter->name);
       : 		hdr = pjsip_msg_find_hdr_by_name(tdata->msg, &hdr_name, NULL);
       : 		if (!hdr) {
       : 			ast_sip_add_header(tdata, iter->name, iter->value);
       : 		}
> Is this a bug fix from something else?
Yes, when I initially started messing with this, I was trying to do integration with the SIP Max-Forwards header. After messing with it, I realized it didn't make any sense to do that as part of this work, so I removed the new files I had added to deal with that. I did not revert this change though.


https://gerrit.asterisk.org/#/c/121/2/res/res_pjsip_diversion.c
File res/res_pjsip_diversion.c:

Line 277: 	old_hdr = pjsip_msg_find_hdr_by_name(tdata->msg, &diversion_name, NULL);
        : 	if (old_hdr) {
        : 		pj_list_erase(old_hdr);
        : 	}
> Bug fix?
In the test where I have a SIP forwarding loop, if this code is not present, then eventually there can be dozens of Diversion headers sent in the 181 response to the caller. Too many of these result in PJSIP failing due to E_MSGTOOLONG.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia7e4b7cd3bccfbd34d9a859838356931bba56c23
Gerrit-PatchSet: 2
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-HasComments: Yes