[Asterisk-code-review] Change in asterisk[master]: res_pjsip: Add external PJSIP resolver implementation using ...

[Matt Jordan (Code Review)]

Matt Jordan has posted comments on this change.

Change subject: res_pjsip: Add external PJSIP resolver implementation using core DNS API.
......................................................................


Patch Set 2:

(7 comments)

As a general question: Have we updated the CHANGES file yet with the new DNS resolver dependency information?

https://gerrit.asterisk.org/#/c/75/2/res/res_pjsip/pjsip_resolver.c
File res/res_pjsip/pjsip_resolver.c:

Line 143:  * \param port The port to use for any resulting records - if not specified the default for the transport is used
        :  *
Long line - wrap at 90 columns


Line 171: 	ast_debug(2, "[%p] Added target '%s' with record type '%d', transport '%s', and port '%d'\n", resolve, name, rr_type,
        : 		pjsip_transport_get_type_name(transport), target.port);
I'd wrap this debug statement at the end of the string, which is just over the max line length (90)


Line 226: 	if (!sip_transport_is_available(transport) &&
        : 		!sip_transport_is_available(transport + PJSIP_TRANSPORT_IPV6)) {
        : 		return -1;
        : 	}
It feels like a debug statement would be nice here as well. If I'm trying to debug why a particular DNS entry wasn't used, knowing that I don't have a transport configured to support it would be handy.


Line 266: 	/* This purposely steals the resolving list so we can add entries to the new one in the same loop and also have access
        : 	 * to the old.
        : 	 */
Wrap comment at 90 columns


Line 272: 	/* The order of queries is what defines the preference order for the records within this specific query set.
        : 	 * The preference order overall is defined as a result of drilling down from other records. Each
        : 	 * completed query set starts placing records at the beginning, moving others that may have already been present.
        : 	 */
Wrap comment at 90 columns


Line 301: 				/* PJSIP has a fixed maximum number of addresses that can exist, so limit ourselves to that */
        : 				if (address_count == PJSIP_MAX_RESOLVED_ADDRESSES) {
        : 					continue;
        : 				}
Why would we not break; out of the DNS records once we hit our max resolve addresses? I would expect that traversing further down into more NAPTR/SRV records would just result in them being ignored.


https://gerrit.asterisk.org/#/c/75/2/res/res_pjsip_session.c
File res/res_pjsip_session.c:

Line 1080: 
I'd revert this extraneous newline, since it doesn't have anything to do with this review.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I56cb03ce4f9d3d600776f36928e0b3e379b5d71e
Gerrit-PatchSet: 2
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>
Gerrit-HasComments: Yes