[Asterisk-code-review] sorcery/res pjsip: Refactor for realtime performance (asterisk[13])

[Kevin Harwell]

Kevin Harwell has posted comments on this change.

Change subject: sorcery/res_pjsip:  Refactor for realtime performance
......................................................................


Patch Set 13: Code-Review-1

(8 comments)

https://gerrit.asterisk.org/#/c/2370/13/include/asterisk/config.h
File include/asterisk/config.h:

Line 355:  * \since 13.8
This will have to be bumped to 13.9 now.


Line 1236:  * \since 13.8
This one too.


Line 1271:  * \since 13.8
Another


Line 1275:  * \param exact_match If true, all variables in left must match all variables in right
         :  *        and vice versa.  This does exact value matches only.  Operators aren't supported.
         :  *
This could use a little rewording I think. For instance for non-exact when does this function return true? Or is this implying operators are supported for non-exact so ast_strings_match is ultimately used?


https://gerrit.asterisk.org/#/c/2370/13/include/asterisk/sorcery.h
File include/asterisk/sorcery.h:

Line 1166:  * \since 13.8.0
s/13.8.0/13.9.0


https://gerrit.asterisk.org/#/c/2370/13/include/asterisk/strings.h
File include/asterisk/strings.h:

Line 1340:  * \since 13.8
Another one to bump the version on.


https://gerrit.asterisk.org/#/c/2370/13/main/config.c
File main/config.c:

Line 776: 			name = ast_strdupa(field->name);
These types of allocations can be dangerous in a loop as they have the potential to "blow" the stack. If this seems like a possible concern in this scenario something different may need to be done here.


Line 793: 	if (exact_match) {
        : 		for (field = left; field; field = field->next) {
        : 			const struct ast_variable *new = ast_variable_find_variable_in_list(right, field->name);
        : 
        : 			if (!new || strcmp(new->value, field->value)) {
        : 				return 0;
        : 			}
        : 		}
        : 	}
Since this function appears to be doing set equality (order is not a consideration) then for exact match checking their equivalence (same number of elements) should suffice. Well given that every element in right is equal to that in left (which you checked above this).


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id2691e447db90892890036e663aaf907b2dc1c67
Gerrit-PatchSet: 13
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: George Joseph <george.joseph at fairview5.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: George Joseph <george.joseph at fairview5.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-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-HasComments: Yes